Skip to content

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

Merged
DanGould merged 3 commits intopayjoin:masterfrom
DanGould:cargo-minimal
Sep 19, 2024
Merged

Add Cargo-{minimal,recent}.lock to test deps#357
DanGould merged 3 commits intopayjoin:masterfrom
DanGould:cargo-minimal

Conversation

@DanGould
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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 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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

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