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

Move to Rust 1.72 #5

Merged
merged 3 commits into from
Oct 1, 2023
Merged

Conversation

Oppen
Copy link
Contributor

@Oppen Oppen commented Sep 22, 2023

No description provided.

@0xLucqs
Copy link
Collaborator

0xLucqs commented Sep 25, 2023

weird that we have this error can you fix it though plz

@Oppen
Copy link
Contributor Author

Oppen commented Sep 25, 2023

It seems the lambdaworks release is not properly pinned. The errors are caused by changes in a newer release than main has, I solved them for the PR extending the APIs.

@Oppen
Copy link
Contributor Author

Oppen commented Sep 25, 2023

Fixed. TL;DR: Cargo assumes stronger guarantees than semver strictly provides, so it assumes it can simply increase the patch level of deps even when in the 0.x.y range, which is wrong and leads to this kind of breakage, unless you either commit your Cargo.lock or manually pin the dependency. I did the latter for now, as we're not yet tracking breaking changes in lambdaworks AFAICT.

@xJonathanLEI
Copy link
Collaborator

xJonathanLEI commented Sep 25, 2023

Cargo considers there's a breaking change if and only if the left-most non-zero number changes. It's important to take this into account when releasing crates, even pre-1.0.0.

It doesn't matter if this library pins it or not, as library Cargo.lock files are ignored for downstream applications. And you can't have two versions that are considered "compatible" by Cargo in the same application. So pinning it here is sorta problematic (the application cannot also use 0.1.4). Better just follow Cargo's rules.

For starknet-rs I always bump minor if there's a breaking change, although it's still pre-1.0.0.

@Oppen
Copy link
Contributor Author

Oppen commented Sep 25, 2023

It doesn't matter if this library pins it or not, as library Cargo.lock files are ignored for downstream applications. And you can't have two versions that are considered "compatible" by Cargo in the same application. So pinning it here is sorta problematic (the application cannot also use 0.1.4). Better just follow Cargo's rules.

What I mean by "pinning" here is the Cargo.toml syntax for it, rather than relying on Cargo.lock. The reason I say it's wrong is because the intent of Cargo's behavior is to enforce semver (or at least that's what the common expectation seems to be), so it should follow the letter IMO. But that's just semantics, the fact is that we need to work with Cargo, be it pinning or versioning according to its rules rather than strict semver.
Re: the application using 0.1.4, yeah, downstream will have to do the same pinning. That's true for any interface breakage. If pinning is too controversial I may try to make some time for yanking 0.1.4 and releasing 0.2.0 instead.

PS: in part the issue is that I wasn't aware of breakage when I did the release. Lambdaworks doesn't yet have a proper changelog to inform of this.

@xJonathanLEI
Copy link
Collaborator

Yeah I originally thought you meant pinning with Cargo.lock and thus the original comment. I later saw the Cargo.toml line and edited my comment to include the 0.1.4 bit (so the Cargo.lock part is kinda irrelevant - sorry for the confusion). Generally, I'd consider pinning like this bad because if the application uses some other library that pins to =0.1.4 (or just puts in 0.1 but actually expects 0.1.4 or newer) things would fall apart as Cargo refuses to mix 0.1.3 and 0.1.4 in the same binary.

@Oppen
Copy link
Contributor Author

Oppen commented Sep 28, 2023

I yanked 0.1.4 and reverted the pinning.

@Oppen
Copy link
Contributor Author

Oppen commented Sep 28, 2023

@LucasLvy @xJonathanLEI it should be working correctly now.

Copy link
Collaborator

@xJonathanLEI xJonathanLEI left a comment

Choose a reason for hiding this comment

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

LGTM

@xJonathanLEI
Copy link
Collaborator

@abdelhamidbakhta What's our merge policy? How many approvals do we need? Can any collaborator smash the merge button once the threshold is reached?

@0xLucqs 0xLucqs merged commit 285e4c0 into starknet-io:main Oct 1, 2023
2 checks passed
@Oppen Oppen deleted the remove_nightly branch October 2, 2023 14:35
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.

3 participants