Make Cargo-minimal and Cargo-recent lockfiles follow expected convention#1612
Conversation
Coverage Report for CI Build 27358652152Coverage remained the same at 85.188%Details
Uncovered ChangesNo uncovered changes found. Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
13a6c1c to
31dd86b
Compare
36266b3 to
3e77c6c
Compare
|
This should avoid cargo updates from occuring on stray CI jobs preventing failures like https://github.com/payjoin/rust-payjoin/actions/runs/27285126244/job/80590095693?pr=1629 in efc5b92 from happening |
DanGould
left a comment
There was a problem hiding this comment.
This is a welcome cleanup for a dangling issue
I ran into an issue running ./update-lock-files.sh locally where payjoin-test-utils had futures = "0.3" and payjoin-mailroom has futures = "0.3.21". cargo +nightly check -Z direct-minimal-versions pins each direct dep to
its own lowest acceptable version, picks 0.3.11 for test-utils, then hits mailroom's ≥0.3.21 floor — and since there's one futures node in the graph, the minima can't coexist → "all possible versions conflict."
error from run
```console dan@Excavator payjoin % bash ./contrib/update-lock-files.sh Updating crates.io index Updating git repository `https://github.com/chavic/uniffi-bindgen-cs.git` Updating git repository `https://github.com/Uniffi-Dart/uniffi-dart.git` error: failed to select a version for `futures`. ... required by package `payjoin-mailroom v0.1.1 (/Users/dan/f/dev/payjoin/payjoin-mailroom)` versions that meet the requirements `^0.3.21` are: 0.3.21all possible versions conflict with previously selected packages.
previously selected package futures v0.3.11
... which satisfies dependency futures = "^0.3" of package payjoin-test-utils v0.0.1 (/Users/dan/f/dev/payjoin/payjoin-test-utils)
... which satisfies path dependency payjoin-test-utils of package payjoin v1.0.0-rc.3 (/Users/dan/f/dev/payjoin/payjoin)
... which satisfies dependency payjoin = "^1.0.0-rc.3" of package payjoin-cli v0.2.0 (/Users/dan/f/dev/payjoin/payjoin-cli)
failed to select a version for futures which could resolve this conflict
| bitcoin = { version = "0.32.7", features = ["base64"] } | ||
| bitcoin = { version = "0.32.9", features = ["base64"] } | ||
| corepc-node = { version = "0.10.0", features = ["download", "29_0"] } | ||
| futures = { version = "0.3", default-features = false, features = ["std"] } |
There was a problem hiding this comment.
Bumping this fixes my update run locally
| futures = { version = "0.3", default-features = false, features = ["std"] } | |
| futures = { version = "0.3.21", default-features = false, features = ["std"] } |
There was a problem hiding this comment.
I have no idea how I didn't catch this, good find
| done | ||
| # Cargo-minimal.lock | ||
| # minimum direct dependency versions | ||
| rm -f Cargo.lock && cargo +nightly check --all-features -Z direct-minimal-versions |
There was a problem hiding this comment.
I think we want --all-targets here so that we make sure tests (and examples, if we have them) also compile with these minimum deps. I don't think dev-dependencies find their minimums without this.
| rm -f Cargo.lock && cargo +nightly check --all-features -Z direct-minimal-versions | |
| rm -f Cargo.lock && cargo +nightly check --all-features --all-targets -Z direct-minimal-versions |
3e77c6c to
cb10f73
Compare
cb10f73 to
712dcf1
Compare
Add a new lockfile backups script to make sure lints and tests are locked to Cargo-recent.lock
712dcf1 to
19d702d
Compare
| Lint-FFI: | ||
| runs-on: ubuntu-latest | ||
| steps: | ||
| - name: "Checkout repo" | ||
| uses: actions/checkout@v4 | ||
| - name: "Use cache" | ||
| uses: Swatinem/rust-cache@v2 | ||
| - name: "Install nix" | ||
| uses: DeterminateSystems/determinate-nix-action@main | ||
| - name: "Use nix cache" | ||
| uses: DeterminateSystems/magic-nix-cache-action@main | ||
| - name: "Run linting check" | ||
| run: cd payjoin-ffi && nix develop -c ./contrib/lint.sh | ||
|
|
There was a problem hiding this comment.
Do you know why this was separated in the first place? Chesterson's fence?
There was a problem hiding this comment.
I looked into the history for this very question. I think that we simply didn't want any failures to hit the main crate at the time as there is no other explanation was given to keep them separate. #846 included commits to actually make it pass the lint so I suspect that was partly why as there was perhaps some expectation of continued instability. @arminsabouri do you have a recollection about any additional motivation here?
There was a problem hiding this comment.
I know the workspaces used to be separated because there was no way to compile payjoin-ffi with the old MSRV, so that'd be one potential reason. Now everything is in the top level Cargo.toml workspace it can all be compiled and linted together.
DanGould
left a comment
There was a problem hiding this comment.
My only real concern here is 'Merge lint and lint-ffi into a single CI job' being here without rationale or understanding why they were separated in the first place. Because it's linting in CI I don't think it's load bearing and am ok moving forward but I do wonder why
The Cargo-minimal and Cargo-recent lockfiles were not actually pointing to the minimal and recent semvers of the dependencies and were instead just copies of the Cargo.lock lockfile.
This is somewhat related to the release but I don't think its all that critical just something that I thought should be improved.
Relates to #940 though I'm not sure it should close it
also related is #454
Pull Request Checklist
Please confirm the following before requesting review:
AI
in the body of this PR.