Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Cargo-{minimal,recent}.lock to test deps #357

Merged
merged 3 commits into from
Sep 19, 2024

Conversation

DanGould
Copy link
Contributor

@DanGould DanGould commented Sep 8, 2024

This replaces manual pins in the README and CI workflow.

Close #337
based on rust-bitcoin/rust-bitcoin#1764

contrib/test.sh Outdated
Comment on lines 36 to 40
cargo test --locked --package payjoin --verbose --all-features --lib
cargo test --locked --package payjoin --verbose --features=send,receive --test integration
cargo test --locked --package payjoin --verbose --no-default-features --features=send,receive,danger-local-https,v2 --test integration
cargo test --locked --package payjoin-cli --verbose --no-default-features --features=danger-local-https,v2 --test e2e
cargo test --locked --package payjoin-cli --verbose --features=danger-local-https
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It may be nice to keep this in a separate script so that it's still easy to quickly run all tests locally without having to do it for each lock file. Similar to how it's done in rust-bitcoin:

	cp "Cargo-$dep.lock" Cargo.lock
	for crate in ${CRATES}
	do
	    (
		cd "$crate"
		./contrib/test.sh
	    )
	done

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've done this now. rust-bitcoin has come a long way and now uses a completely separate rust-bitcoin-maintainer-tools repo to test with extreme granularity.

I think we want a way to run all the crates' tests without looping through all dependencies, so I'm working on that next.

contrib/test.sh Show resolved Hide resolved
contrib/test.sh Outdated
Comment on lines 21 to 33
# Pin dependencies as required if we are using MSRV toolchain.
if cargo --version | grep "1\.63"; then
cargo update -p cc --precise 1.0.105
cargo update -p clap_lex --precise 0.3.0
cargo update -p regex --precise 1.9.6
cargo update -p reqwest --precise 0.12.4
cargo update -p time --precise 0.3.20
cargo update -p tokio --precise 1.38.1
cargo update -p tokio-util --precise 0.7.11
cargo update -p url --precise 2.5.0
cargo update -p which --precise 4.4.0
cargo update -p zstd-sys --precise 2.0.8+zstd.1.5.5
fi
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't the whole point of Cargo-minimal.lock that we don't have to pin these MSRV versions manually?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, I saw that rust-bitcoin was still pinning like this in rust-bitcoin/rust-bitcoin#1764. The idea was that the Cargo-minimal.lock let downstream users get the pins without manually entering them, having them specified in one place instead of in both CI yaml and in the readme. But we could remove this entirely, we'd just lose some track of exactly what has been updated, but I suppose that would make the lock file the one true dependency reference, so I've removed these update lines

@DanGould DanGould force-pushed the cargo-minimal branch 3 times, most recently from 5c99bc3 to 6eb44b6 Compare September 14, 2024 20:21
This replaces manual pins in the README and CI workflow.
time@0.3.36 pin can be elided since the minimal.lock already has 0.3.20.
@DanGould
Copy link
Contributor Author

I went with chmod +x *test.shing because those files should be executable in any environment. Made more sense to me than changing the scripts to call bash test.sh for that reason.

@DanGould DanGould merged commit 6460c85 into payjoin:master Sep 19, 2024
4 checks passed
@DanGould DanGould deleted the cargo-minimal branch September 19, 2024 15:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Ship a Cargo-minimal.lock and Cargo-recent.lock instead of listing required pinned deps
2 participants