Test with minimal dependency versions by Kixunil · Pull Request #1272 · rust-bitcoin/rust-bitcoin

Kixunil · 2022-09-14T11:24:35Z

It could happen that we unknowingly depend on a new version of a crate without updating Cargo.toml. This could couse resolution issues for downstream users. It's also unclear for outsiders to know with which dependencies did we test the crate.

This change commits two lock files: minimal and recent. minimal contains minimal depdendency versions, while recent contains dependency versions at the time of making the change.

Further, this adds CI jobs to test with both lock files, CI job for internals crate, removes old serde pinning and prints a warning if recent is no longer up to date. (We may have to override it somehow if any crate breaks MSRV.)

The documentation is also updated accordingly.

Closes #1230

Kixunil · 2022-09-14T11:54:22Z

~~WTF, this runs on my machine anyone has any idea?~~ Even bigger WTF, this fails on older Rust but it's not supposed to be nightly. Did Rust break the API?!

Kixunil · 2022-09-14T14:45:15Z

Oh, I see now, they kept in an alias. core2 broke MSRV and it was restored in technocreatives/core2#7
Released in core2 0.3.2

apoelstra

ACK ffdb2f489441020a9e57d73ee1dd7910b34992a0

Kixunil · 2022-09-14T17:45:28Z

Oh, is this actually correct? From my understanding of matrix GA option it should test with all combinations of {minimal,recent}{$rust_versions} but from logs it doesn't seem to be doing that.

apoelstra · 2022-09-14T17:47:45Z

I think https://github.com/rust-bitcoin/rust-bitcoin/actions/runs/3053748075/jobs/4924768120 is doing the right thing?

Kixunil · 2022-09-14T18:04:41Z

It seems to be only testing with 1.47. Am I missing something?

apoelstra · 2022-09-14T18:15:35Z

Oh, you're right -- I just looked for "is it actually doing the cp command somewhere". But yes, I think it's only doing 1.47.

Maybe somebody else here understands GH Actions but unfortunately you may just have to iterate a dozen times until you guess how to make it do what you want..

Kixunil · 2022-09-14T18:17:17Z

I will re-read the docs and retry and then start pinging people if it still doesn't work. :)

It could happen that we unknowingly depend on a new version of a crate without updating `Cargo.toml`. This could couse resolution issues for downstream users. It's also unclear for outsiders to know with which dependencies did we test the crate. This change commits two lock files: `minimal` and `recent`. `minimal` contains minimal depdendency versions, while `recent` contains dependency versions at the time of making the change. Further, this adds CI jobs to test with both lock files, CI job for `internals` crate, removes old `serde` pinning and prints a warning if `recent` is no longer up to date. (We may have to override it somehow if any crate breaks MSRV.) The documentation is also updated accordingly. Closes rust-bitcoin#1230

Kixunil · 2022-09-14T19:07:42Z

It looks like it works. I've moved duplicate checking to lint part. (I'd love to have a separate linting job but that's for another time.)

apoelstra

ACK 1e4487a

tcharding · 2022-09-14T23:45:14Z

README.md


+## External dependencies
+
+The crate integrates with a few external librarie, most notably `serde`. These


Suggested change

The crate integrates with a few external librarie, most notably `serde`. These

The crate integrates with a few external libraries, most notably `serde`. These

tcharding · 2022-09-14T23:57:55Z

contrib/test.sh

+		cd "$crate"
+		./contrib/test.sh
+	    )
+	done


Perhaps to catch future mistakes when editing the test scripts we could add

if ! diff Cargo-$dep.lock Cargo.lock then echo "::warning Cargo.lock has been updated by test run - check for missing --locked flag" exit 1 fi

tcharding · 2022-09-15T00:01:54Z

contrib/test.sh

+		then
+			echo Dependencies are up to date
+		else
+			echo "::warning file=Cargo-recent.lock::Dependencies could be updated"


Can I ask, why the double colons in ::warning? We could use colors

echo "\e[33mWarning:\e[0m ..."

This is a special signal for GitHub to show the warning at that file. Sadly one will only see it when explicitly looking at the file but that's still better than looking into logs.

Oh cool, cheers - I've never seen that before.

Me neither, I just thought "is there a way to warn in GH actions?" and succeeded googling it. :)

tcharding · 2022-09-15T00:04:39Z

internals/contrib/test.sh

+
+if [ "$DO_LINT" = true ]
+then
+    cargo clippy --frozen --all-features --all-targets -- -D warnings


Why use --frozen over --locked, is it because we know the order of CRATES so we know that cargo has already been run with --locked so doesn't need to hit the network again?

I previously thought --frozen does something else. I changed it to --locked but forgot in this case.

dpc · 2022-09-15T05:43:48Z

Any reason for all the manual file handling? I was just hoping to eventually get something like: https://github.com/fedimint/fedimint/pull/580/files working . Done automatically in the CI.

Kixunil · 2022-09-15T06:34:31Z

@dpc Mainly I wanted for people to be able to see the tested versions somewhere. I also fear that some nightly will change Cargo.lock format like it happened before and the test will stop working.

dpc · 2022-09-15T06:37:30Z

Mainly I wanted for people to be able to see the tested versions somewhere.

cat Cargo.lock after the update?

I also fear that some nightly will change Cargo.lock format like it happened before and the test will stop working.

I don't know the details, but isn't min-version checking ... kind of a best effort thing anyway?

@Kixunil

Kixunil · 2022-09-15T06:41:02Z

kind of a best effort thing anyway

Given we run whole test suite against them I don't think so. :)

apoelstra · 2022-09-15T13:09:27Z

I like this approach, because as @Kixunil says, you can see exactly the lockfile that's being used.

If it breaks more than once due to cargo changing its format maybe we'll have to revisit our approach.

Edit interesting idea @dpc to use cargo -Z minimal-versions and then catting the resulting lockfile..

dpc · 2022-09-15T16:34:13Z

I also fear that some nightly will change Cargo.lock format like it happened before and the test will stop working.

Can you please elaborate or paste some links? I don't understand what exactly changed.

I see the point or bitcoin being so widely used core library for Bitcoin Rust ecosystem that it needs a solid check here. However I'm for other projects, I'm always reluctant putting extra manual steps on the contributors.

Kixunil · 2022-09-15T17:26:56Z

If I remember correctly a lock file produced by current MSRV can't be read by old rust-bitcoin MSRV - 1.29. You have to delete it and re-generate. If similar thing happened in nightly it'd break us.

I don't actually want contributors to randomly update dependencies. :) If it's really required we will figure it out together.

apoelstra · 2022-09-15T19:23:17Z

Yes, there was definitely a break I remember around 1.30 or so which caused us no end of grief with the old MSRV, but I can't think of another time. So it's rare at least. And it may be that after the noise people made last time, they won't do it again :P

sanket1729

strong concept ACK. Agree with all of @tcharding's suggestions

Kixunil · 2022-09-17T07:39:08Z

Oh, I just remembered a good reason to not just cat the lock file: non-logged-in people can't see outputs of runs.

tcharding · 2023-03-22T02:55:58Z

Conflicts with #1729

apoelstra · 2023-03-22T15:30:05Z

cc rust-bitcoin/rust-secp256k1#591 which does something similar (but does not actuall test) in rust-secp

tcharding · 2023-03-23T05:36:03Z

Have you time to bring this back to life @Kixunil or would you like me to pick it up for you?

Kixunil · 2023-03-23T08:09:32Z

I'm pretty busy lately, feel free to take it if you have time!

tcharding · 2023-03-23T23:55:38Z

No worries man, will get to it shortly. Cheers.

tcharding · 2023-03-29T23:07:47Z

I picked up this PR over at #1764

c4c64c0 Test with minimal dependency versions (Martin Habovstiak) d5655d5 Bump core2 dependency from 0.3.0 -> 0.3.2 (Tobin C. Harding) Pull request description: This is work originally done by Kixunil in #1272, I picked it up to help out. The only changes I made were rebasingg, updating the recent lock file, adding `--locked` to hashes contrib file, and adding a co-developed-by tag for accountability. It could happen that we unknowingly depend on a new version of a crate without updating `Cargo.toml`. This could cause resolution issues for downstream users. It's also unclear for outsiders to know with which dependencies did we test the crate. This change commits two lock files: `minimal` and `recent`. `minimal` contains minimal dependency versions, while `recent` contains dependency versions at the time of making the change. Further, this adds CI jobs to test with both lock files, CI job for `internals` crate, removes old `serde` pinning and prints a warning if `recent` is no longer up to date. (We may have to override it somehow if any crate breaks MSRV.) The documentation is also updated accordingly. Closes #1230 ACKs for top commit: apoelstra: ACK c4c64c0 Kixunil: ACK c4c64c0 Tree-SHA512: 7d386e96ab747f6a6bafeea828ac65bd8bb11975eaa3408acecac369cd2f235f6e9d4c57202be18a3dc2eeb2a2df532d73e4d35cd1f3fbf092eb6414c55b1524

Kixunil added enhancement ci Tests labels Sep 14, 2022

Kixunil force-pushed the lock-files branch from 9c315cb to ffdb2f4 Compare September 14, 2022 14:45

apoelstra previously approved these changes Sep 14, 2022

View reviewed changes

Kixunil mentioned this pull request Sep 14, 2022

Redesign hex::BufEncoder to accept owned arrays #1273

Merged

Kixunil dismissed apoelstra’s stale review via e6d508d September 14, 2022 18:47

Kixunil force-pushed the lock-files branch 2 times, most recently from e6d508d to 371acd5 Compare September 14, 2022 18:55

Kixunil force-pushed the lock-files branch from 371acd5 to 1e4487a Compare September 14, 2022 19:03

Kixunil requested review from apoelstra and tcharding September 14, 2022 19:29

apoelstra approved these changes Sep 14, 2022

View reviewed changes

tcharding reviewed Sep 15, 2022

View reviewed changes

sanket1729 reviewed Sep 17, 2022

View reviewed changes

tcharding mentioned this pull request Mar 29, 2023

Test with minimal dependency versions #1764

Merged

Kixunil closed this Mar 30, 2023


		## External dependencies

		The crate integrates with a few external librarie, most notably `serde`. These

Conversation

Kixunil commented Sep 14, 2022

Uh oh!

Kixunil commented Sep 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Kixunil commented Sep 14, 2022

Uh oh!

apoelstra left a comment

Choose a reason for hiding this comment

Uh oh!

Kixunil commented Sep 14, 2022

Uh oh!

apoelstra commented Sep 14, 2022

Uh oh!

Kixunil commented Sep 14, 2022

Uh oh!

apoelstra commented Sep 14, 2022

Uh oh!

Kixunil commented Sep 14, 2022

Uh oh!

Kixunil commented Sep 14, 2022

Uh oh!

apoelstra left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dpc commented Sep 15, 2022

Uh oh!

Kixunil commented Sep 15, 2022

Uh oh!

dpc commented Sep 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Kixunil commented Sep 15, 2022

Uh oh!

apoelstra commented Sep 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dpc commented Sep 15, 2022

Uh oh!

Kixunil commented Sep 15, 2022

Uh oh!

apoelstra commented Sep 15, 2022

Uh oh!

sanket1729 left a comment

Choose a reason for hiding this comment

Uh oh!

Kixunil commented Sep 17, 2022

Uh oh!

tcharding commented Mar 22, 2023

Uh oh!

apoelstra commented Mar 22, 2023

Uh oh!

tcharding commented Mar 23, 2023

Uh oh!

Kixunil commented Mar 23, 2023

Uh oh!

tcharding commented Mar 23, 2023

Uh oh!

tcharding commented Mar 29, 2023

Uh oh!

Reviewers

Assignees

Labels

Kixunil commented Sep 14, 2022 •

edited

Loading

dpc commented Sep 15, 2022 •

edited

Loading

apoelstra commented Sep 15, 2022 •

edited

Loading