Let gix-testtools use gix-* workspace crates
#1992
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
`gix-testtools` depends on several other `gix-*` crates. Before version 0.16.1 (GitoxideLabs#1972), `gix-testtools` depended on prior breaking versions of those crates (as discussed in GitoxideLabs#1510 and GitoxideLabs#1886). Since then, it depends on the current versions. When depending on a strictly earlier version, it was necessary to omit `path =` in the `gix-testtools` manifest for its `gix-*` dependencies. Now that `gix-testtools` depends on current versions of those dependencies, it seems feasible to specify both `version` and `path`, as we do in other cases where one crate developed in this workspace depends on another crate developed in the workspace. Aside from improving general consistency (which is a weak rationale here, since the role of `gix-testtools` differs substantially from that of other `gix-*` crates, in terms of how we're ourselves using it), the broad benefits here are that: 1. Ambiguity in what crate is meant, when an operation is performed on a specific `gix-*` crate, is lessened, or maybe even eliminated. (GitoxideLabs#1989) 2. Because the code of the dependency comes from the workspace when applicable, i.e. when `gix-testtools` is itself being used in the workspace, it should allow new not-yet-published functionality to be leveraged in `gix-testtools`, without confusion or breakage. (GitoxideLabs#1886) Before this, some actions we'd prefer to do by `<cmd> -p <crate>` had to be done by `(cd <crate-dir>; <cmd>)`. This was needed to operate on `gix-*` crates in the workspace that are also dependencies, even transitively, of `gix-testtools`. This affected some commands in `justfile` recipes, some commands run in CI workflows (indirectly via `just`, or directly in script steps), and some operations carried out manually. This included `cargo nextest run` and `cargo check` on various crates. Here's an example (shown on Windows, but this problem was not specific to Windows) using `gix-date`, which is not listed in `tests/tools/Cargo.toml`, but which is a transitive dependency: C:\Users\ek\source\repos\gitoxide [main ≡]> cargo nextest run -p gix-date Blocking waiting for file lock on package cache error: There are multiple `gix-date` packages in your project, and the specification `gix-date` is ambiguous. Please re-run this command with one of the following specifications: path+file:///C:/Users/ek/source/repos/gitoxide/gix-date#0.10.1 registry+https://github.com/rust-lang/crates.io-index#[email protected] error: command `'\\?\C:\Users\ek\.rustup\toolchains\stable-x86_64-pc-windows-msvc\bin\cargo.exe' test --no-run --message-format json-render-diagnostics --package gix-date` exited with code 101 An important special case is that of editor/IDE integration, especially in VS Code. This couldn't run and (more significantly, in view of the benefit of integration) couldn't debug some of the tests. This happened because synthesized `cargo test -p ...` commands, used behind the scenes to launch the tests, were ambiguous. For further details, see GitoxideLabs#1989. Another benefit is that the lockfile and dependency tree are simpler, and the dependency tree is truly unified. That points to an important aspect of this change, which is more than a refactoring and will affect test behavior: - It shouldn't produce different behavior when `gix-testtools` is obtained from crates.io (i.e. when projects developed outside the `gitoxide` repository use `gix-testtools`), it can produce different behavior here, where `gix-testtools` will use changes to its `gix-*` dependencies (and accordingly their own dependencies, recursively) that are present in the workspace even if not present in the released version that matches `version =`. - That could be a good thing if it causes new changes to be exercised more and earlier. That might help find bugs. - This is also desirable in that it allows feature changes and bugfixes in `gix-*` crates to be used immediately in `gix-testtools`, before either those `gix-*` crates or `gix-testtools` are published with the changes (GitoxideLabs#1886). But... However: - It could be bad if it introduces an undesirable dependency ordering for fixing bugs and/or introducing regression tests. That is, in principle there could arise two (possibly related) bugs, A and B, where there is some reason to fix A before B, but where B must be fixed in order for the regression test for A to run (to validate that it can catch A), due to B breaking `gix-testtools` as used in the test for A or in other tests in the crate affected by A. Because this would presumably be known--an error would occur, likely when building the tests--it could be worked around by temporarily (or permanently) reverting this change if and when such a problem ever arises, or partially undoing it for the specific affected `gix-*` dependency of `gix-testtools`. - It could be bad if a bug affects a `gix-*` crate and its own tests in identical or complementary ways, and this is used to establish or check an expectation. That is, in principle there could arise a bug in a `gix-*` crate that `gix-testtools` uses, and that itself uses `gix-testtools` in its tests, that causes a test that should catch that bug (either initially or to verify a bugfix) to wrongly report that the code is working. This scenario is a case of the general problem that duplicated logic between code and its tests can cause a bug to appear (either in the same form or in different forms) in both, such that tests that should catch the bug don't catch it because they suffer from the same bug. In the hypothetical case imagined here, the duplication of logic would arise from the tests calling and using the very code that is under test. For the way we are currently using or likely ever to use `gix-testtools`, it seems like this would probably not happen. But it is hard to be completely sure. Unlike the previously described scenario, if this scenario did occur, it would likely not be noticed. Both those problem scenarios have corresponding scenarios that had already applied (and which the change here at least slightly *mitigates*): if the code with the bug has already been published. This fixes GitoxideLabs#1989 and makes progress toward GitoxideLabs#1886.
The previous commit notes: > Before this, some actions we'd prefer to do by `<cmd> -p <crate>` > had to be done by `(cd <crate-dir>; <cmd>)`. This was needed to > operate on `gix-*` crates in the workspace that are also > dependencies, even transitively, of `gix-testtools`. All occurrences of `cd ...` followed immediately by a `cargo` command in the `justfile` and in `ci.yml` were for that reason. This commit changes them to use `-p` instead. The changes in the `justfile` affect both manual runs and runs on CI through the `test` job. The changes directly in `ci.yml` affect the `wasm` jobs. This is intended to be a refactoring. No change is anticipated in what tests are run, their features, or their behavior. The rationale is twofold: - Simplify the commands. This is the form most other such commmands were already written in. These seem to have been written in the `cd` form only as a workaround for the now-fixed `-p` ambiguity. - Verify that the fix actually works and that there is nothing else breaking these checks when they are run from the top level of the workspace.
|
Thanks a lot! This PR is definitely wanted, and I marked it as draft to indicate that I hope to try it out locally to at least create an issue in CSR to be able to tackle the resulting issues at some time. |
|
Should I attempt a dry-run publish? Alternatively, is cargo-smart-release working with custom registries? I think you had mentioned that using a local registry for testing would be helpful. |
|
You could try a dry-run, but if that doesn't run into problem, doing a real run would be fine too - I'd expect it to run into trouble before it bumps into permissions errors. If it seems to work I will try myself. Which reminds me… probably it would be best to just merge this PR to assure I will track the issues that arise from it, and I will revert it when I have to. |
I worry that, if something goes unexpectedly, then it might somehow try to push tags, which I think would succeed, since I have the ability to do that. Though I could try to run it in an environment where
That makes sense, and perhaps should've been listed as a fourth bullet point in my description here. A possible variant could be to merge only the first commit, since the second commit makes changes that rely on it. But the second commit also serves to test the effect of the first commit, so I would suggest merging the PR as a whole and then just reverting the whole thing if need be. |
That's a very valid point and a great catch! Fortunately there is a cure, In any case, let's merge to get the ball rolling. |
Thanks--I should've checked for such options! |
|
In case it is of interest, at the current All changelogs looked reasonable when previewed. I don't know if there are other steps that are needed, or if the error is due to an anticipated problem, or unrelated. The changes in the generated commit were as shown here (full content), as produced by Edit: I haven't done much to look into this so far, because I wanted to post this in case it turns out to be relevant for crate releases to help get #2005 out. But I'm hoping it's enough to release a patch version of |
|
|
Could this have worked because I don't know how to investigate the behavior of any current or past Do you think it's okay to move forward with work on things like #1886 that rely on the changes here? |
|
That's probably the reason - if
To me it makes sense even if just to gather data and approach the problem in a more rational way than it was handled before. My thinking is that I should be able to workaround the problem after gather said data, making it nothing more than an inconvenience when publishing. It will, however, pave the way for a fix or automated workaround. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #1989
This is the PR I had promised in #1989 to demonstrate the effect of the fix proposed there. Based on the last paragraph of #1989 (comment), I am guessing that this is not suitable for merging at this time, or maybe even at all, and therefore that this is effectively just an experiment rather than something to be merged. I recommend:
(The "other tradeoffs to consider" that I had alluded to in the description of #1989 are detailed in the bulleted list under "However:" in the commit message for the first commit here, 7e057f2. However, if this should not be integrated due to cargo-smart-release possibly not being ready for it, then it may not be necessary to look at that.)