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.

Sign up for GitHub

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

Jump to bottom

Upgrade dev dependencies #967

Closed
wants to merge 3 commits into from
Closed

Upgrade dev dependencies #967

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
15bae28
Remove CI check for Rust 1.29
tcharding Apr 21, 2022
8f45723
test.sh: Remove 1.29 pinning
tcharding Apr 21, 2022
8e005f9
Upgrade dev dependencies
tcharding Apr 21, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 1 addition & 2 deletions .github/workflows/rust.yml
View file
Open in desktop
Expand Up @@ -25,10 +25,9 @@ jobs:
AS_DEPENDENCY: true
DO_NO_STD: true
DO_DOCS: true
- rust: 1.29.0
- rust: 1.41.1
env:
AS_DEPENDENCY: true
PIN_VERSIONS: true
steps:
- name: Checkout Crate
uses: actions/checkout@v2
Expand Down
4 changes: 1 addition & 3 deletions Cargo.toml
View file
Open in desktop
Expand Up @@ -45,12 +45,10 @@ serde = { version = "1", features = [ "derive" ], optional = true }
hashbrown = { version = "0.8", optional = true }

[dev-dependencies]
serde_json = "<1.0.45"
serde_json = "1"
serde_test = "1"
Copy link
Collaborator

Choose a reason for hiding this comment

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

AFAIK version specification like this (1 instead of 1.0.0) is considered an anti-pattern but I myself don't care much. It's equivalent to 1.0.0 and it'd only be a problem if it didn't work with minimal versions.

Copy link
Member

Choose a reason for hiding this comment

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

I also don't really care (or fully understand the distinction) but we might as well follow best practices.

Copy link
Contributor

@dpc dpc Apr 22, 2022 •
edited

Choose a reason for hiding this comment

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

1 allows deps resvoler to pull in any version. And oftentimes what you actually tested with is not 1.0.0 but 1.0.666 or someting, and when resolver for whatever reason pulls in 1.0.3 and things are not working, user will get a weird error in your crate, not the crate that actually caused the 1.0.3 to pulled in.

Because of that it is recommend to put in a version that you actually know is working and thus enforce it as a minimum one.

Copy link
Contributor

Choose a reason for hiding this comment

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

https://users.rust-lang.org/t/psa-please-specify-precise-dependency-versions-in-cargo-toml/71277

https://www.reddit.com/r/rust/comments/skckkl/psa_please_specify_precise_dependency_versions_in/

Copy link
Member

Choose a reason for hiding this comment

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

That cargo +nightly -Z minimal-versions update trick is pretty cool!

Copy link
Member Author

Choose a reason for hiding this comment

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

Cool, I had a bit of a play with it and was getting grief from rust-secp256k1, I chased that down to rand and it gets resolved during the MSRV and rand 0.8 bump PR. So I'll just leave this sitting here until we do secp. Cheers

Copy link
Collaborator

Choose a reason for hiding this comment

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

@tcharding was just thinking about opening an issue to audit. But maybe we could also add a test into CI? The problem is minimal-versions could cause breakage even if dependency of dependency is incorrect. OTOH we would get the chance to fix it for them by just setting the correct version. :)

The main difference between 1 and 1.0.0 is the explicitness. 1 may indicate "I didn't make effort to check which one works". However given how many versions serde has, I'd personally be suspicious even if there was 1.0.0. :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I'm not really sure whats the best thing to do about our version numbers. Any more suggestions from anyone on how to find a suitable version number?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Setting the correct minimal version is obviously the most useful (flexible) for consumers but may be a bit annoying to figure out depending on circumstances. I wouldn't mind trying at some point though.

secp256k1 = { version = "0.22.0", features = [ "recovery", "rand-std" ] }
bincode = "1.3.1"
# We need to pin ryu (transitive dep from serde_json) to stay compatible with Rust 1.22.0
ryu = "<1.0.5"

[[example]]
name = "bip32"
Expand Down
18 changes: 0 additions & 18 deletions contrib/test.sh
View file
Open in desktop
Expand Up @@ -8,19 +8,6 @@ then
alias cargo="cargo +$TOOLCHAIN"
fi

pin_common_verions() {
cargo generate-lockfile --verbose
cargo update -p cc --precise "1.0.41" --verbose
cargo update -p serde --precise "1.0.98" --verbose
cargo update -p serde_derive --precise "1.0.98" --verbose
}

# Pin `cc` for Rust 1.29
if [ "$PIN_VERSIONS" = true ]; then
pin_common_verions
cargo update -p byteorder --precise "1.3.4"
fi

if [ "$DO_COV" = true ]
then
export RUSTFLAGS="-C link-dead-code"
Expand Down Expand Up @@ -95,10 +82,5 @@ then
cd dep_test
echo 'bitcoin = { path = "..", features = ["use-serde"] }' >> Cargo.toml

# Pin `cc` for Rust 1.29
if [ -n "$PIN_VERSIONS" ]; then
pin_common_verions
fi

cargo test --verbose
fi