Skip to content

Fix Rust CI build#441

Merged
andymck merged 3 commits intomasterfrom
fix-ci
Feb 13, 2025
Merged

Fix Rust CI build#441
andymck merged 3 commits intomasterfrom
fix-ci

Conversation

@kurotych
Copy link
Member

@kurotych kurotych commented Feb 12, 2025

set an upper bound on supported rand versions

@kurotych kurotych changed the title Fix CI build Fix Rust CI build Feb 12, 2025
@kurotych kurotych marked this pull request as ready for review February 12, 2025 13:43
Copy link
Contributor

@andymck andymck left a comment

Choose a reason for hiding this comment

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

LGTM, the alternative is to pin the deps at 0.8. Might be less risk giving this gets pulled in by just about everything...lets see what @madninja thinks

@kurotych kurotych requested a review from madninja February 12, 2025 14:13
prost = { workspace = true }
rand = ">=0.8"
rand_chacha = ">=0.3"
rand = ">=0.9"
Copy link
Member

Choose a reason for hiding this comment

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

Would 0.8 work as lowest version?

Copy link
Member Author

Choose a reason for hiding this comment

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

In case you want to explicitly set rand = 0.8 for this dependency—I think not. The breaking changes arrived in 0.9

But, I've tested the oracles repo with this branch. It has 3 different rand version inside and it compiles successfully

oracles on  use-new-rand [?] via 🦀 v1.83.0 on ☁️  akurotych@nova-labs.com took 4s
❯ cargo tree -i rand
error: There are multiple `rand` packages in your project, and the specification `rand` is ambiguous.
Please re-run this command with one of the following specifications:
  rand@0.7.3
  rand@0.8.5
  rand@0.9.0

Copy link
Member

@madninja madninja Feb 13, 2025

Choose a reason for hiding this comment

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

I meant >= 0.8 but that would break because it picks up 0.9, right?

I usually try to avoid many versions of crates. It works but bloats the binary size.

The usual way to fix this is with -
=0.8 to avoid yet another copy

Copy link
Member Author

@kurotych kurotych Feb 13, 2025

Choose a reason for hiding this comment

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

I meant >= 0.8 but that would break because it picks up 0.9, right?

Yes, correct

I usually try to avoid many versions of crates. It works but bloats the binary size.
The usual way to fix this is with -
...
=0.8 to avoid yet another copy

Pining version to =0.8 is the another way to fix this problem. I don't have any strong opinion which way is better.
Do you prefer pining the version to 0.8?

Copy link
Member

Choose a reason for hiding this comment

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

Yes please. Reducing duplicates is good

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes please. Reducing duplicates is good

Done. I've reverted code changes and set an upper bound on supported rand versions to give Cargo more freedom in choosing the rand version.

@kurotych kurotych requested a review from madninja February 13, 2025 12:13
prost = { workspace = true }
rand = ">=0.8"
rand_chacha = ">=0.3"
rand = "<=0.8.5"
Copy link
Member

Choose a reason for hiding this comment

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

That’s too tight. =0.8

Copy link
Contributor

Choose a reason for hiding this comment

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

@madninja so you want it pinned at 0.8 ?

Copy link
Member

Choose a reason for hiding this comment

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

< 0.9

@andymck andymck merged commit 9e3fd99 into master Feb 13, 2025
7 checks passed
@andymck andymck deleted the fix-ci branch February 13, 2025 16:07
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.

4 participants