lib: merge testutils crate directly into jj_lib::testutils
#3878
Conversation
thoughtpolice
force-pushed
the
aseipp/push-vrxtoymnuvpv
branch
from
95410b1 to
73ef8a5
Compare
|
I forgot to mention:
I can commit myself to fixing both of these, for example by splitting out the code into a new |
Is it not possible to override the checks to be more permissive only at the |
|
I tried that but I think that there can only be one unique set of top-level |
|
Thank you for working on this. The Update: Actually, it seems like recent |
There was a problem hiding this comment.
I'm not sure if this is the right way forward. At some point, we might want to extract some library code to separate crate (to build proc-macro on top of it for example.) Then, there will be some project-level dependency cycle as we have right now: {libA/tests, libB/tests} (-> testutils) -> {libA, libB}.
Update: Actually, it seems like recent
rust-analyzeris better at filtering them out from the Problems pane, maybe, right now for me, but that might break if there's a breeze or the phase of the moon shifts.
There's some progress on this. I think rust-analyzer will (or can already?) handle fake jj-lib/tests -> testutils -> jj-lib dependency cycle.
My plan was actually to look into this as a next step, something like a I can of course abandon this change and start over with that in mind instead and simply move testutils out into such a crate at the start, but just moving everything into |
|
After thinking more and taking a walk I think I see what you mean; the testutils code also needs to be refactored in order to pull apart the implicit dependencies on |
That's great news! 🎉 I don't have an opinion on whether this refactor is good in the long term. After reading Yuya's comment, my mind also (like Austin's) went to the idea of making a minimal subset of jj-lib crate that could be merged with |
|
So, I went and spent some time more closely reading the If we came to a point where we wanted to split out utils code from lib into another crate in the chain — I don't think any of this code would qualify for that privilege anyway, i.e. it would still just remain in what is now the So, I think the real question isn't about whether the code here will end up in a different crate that's higher up in the crate dep chain. I don't think that's going to happen. So then it becomes: should this module be in |
thoughtpolice
force-pushed
the
aseipp/push-vrxtoymnuvpv
branch
from
73ef8a5 to
50d5d69
Compare
|
The latest version of the patch transitions |
My point is that the testutils (or crate containing testutils) will depend on lib crates, but these lib creates will also have integration tests that depend on the entire lib environment. Let's say
The last one isn't a real cycle, but old We can of course split The following post explains valid use cases. |
|
FYI, this is minimal change to remove scary dependency cycle within jj-lib unit tests. |
While reading the code while working on Buck-style builds, I realized that there is a strange cyclic dependency between `jj-lib` and `testutils`: - `jj-lib` has a dev-dep on `testutils` - `testutils` has a normal dep on `jj_lib` Cargo apparently has (some?) kind of logic to topologize this sort of graph, but it's awkward to understand or otherwise express, and has knock-on issues with `rust-analyzer` too because certain code is only built under the proper testing configuration, e.g. with `#[cfg(feature = "testing")]` This instead just merges all of the `testutils` code as a single `jj_lib::testutils` module, which breaks the cycle and also simplifies a few of the mod imports as well. To fix the remaining compile errors we have to explicitly use `jj_lib::testutils` first, which minimizes the overall diff. Some call sites are simple enough to just inline though. I do think that some of this can be pulled back out into a `jj-utils` package that `jj-lib` unconditionally depends on and contains other various functionality, too. Signed-off-by: Austin Seipp <[email protected]>
thoughtpolice
force-pushed
the
aseipp/push-vrxtoymnuvpv
branch
from
50d5d69 to
b29f4c5
Compare
|
FWIW, rust-analyzer team member @davidbarsky’s statements at various points on the Discord:
I think we shouldn’t expect rust-analyzer to get better at handling this, and there’s some vague indication that this might be contributing to issues getting rust-analyzer to work in Zed (though that’s not certain). Circular crate dependencies might be useful in some cases, but they also break Cargo’s model, so we can’t expect a good experience with them, and it seems like we’re likely to be playing whack‐a‐mole with individual problematic circular references if we continue to have the cycle. I think it makes most sense to go with this approach for now and worry about better factoring when crates are split. |
|
Sorry for not replying sooner to this @yuja, life gets in the way... Aside from the rust-analyzer stuff, I would like to respond to this:
I think this is the main sticking point is: what are you splitting up the crates for? I don't think splitting up things this way makes sense without a motivation. Actually, on that note, in my mind, I think of crate splitting in our case as having one major selling point:
Why is this the main selling point? Doesn't it help if users can only depend on a subset of crates? That's nice for them. Maybe it is, but I think it's an illusion of choice (more on that later). But here is my second point that can help expose my thought process:
Now, let's apply this thought process to the
The first answer is "in
Under this interpretation, Note that people would probably not even depend on In short, if we split up crates, I suspect it would mostly be a hierarchy of types and traits that are appropriately abstracted. But integration testing still happens in Note that there are also downsides to having a lot of crates. One I really dislike is having to maintain "equi-consistent" versions of many published crates. If we have 10 crates and a bugfix only touches 1 crate (and 2 transitively dependent crates), how do you cut a release? Do you do a full version bump and cut a new release for all 10 crates, or only bump each package's minor version and then release 1 new crate? Or 3 new crates if the bugfix requires a lower-bound? One advantage of This isn't related to this ticket but I'm pointing this out because, often users will get themselves into weird situations — let's say you forget a lower bound bump, so a cargo update doesn't update transitive dependencies, causing a build failure — and having a bunch of split up crates is often an illusion of choice, because it's a false choice; you almost always do want the components that were tested together at release time! |
While reading the code while working on Buck-style builds, I realized that there is a strange cyclic dependency between
jj-libandtestutils:jj-libhas a dev-dep ontestutilstestutilshas a normal dep onjj_libCargo apparently has (some?) kind of logic to topologize this sort of graph, but it's awkward to understand or otherwise express, and has knock-on issues with
rust-analyzertoo because certain code is only built under the proper testing configuration, e.g. with#[cfg(feature = "testing")]This instead just merges all of the
testutilscode as a singlejj_lib::testutilsmodule, which breaks the cycle and also simplifies a few of the mod imports as well. To fix the remaining compile errors we have to explicitly usejj_lib::testutilsfirst, which minimizes the overall diff. Some call sites are simple enough to just inline though.I do think that some of this can be pulled back out into a
jj-utilspackage thatjj-libunconditionally depends on and contains other various functionality, too.Checklist
If applicable:
CHANGELOG.md