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

Clap MSRV incompatible #740

Closed
samcarey opened this issue Feb 15, 2024 · 9 comments
Closed

Clap MSRV incompatible #740

samcarey opened this issue Feb 15, 2024 · 9 comments
Labels
bug Something isn't working

Comments

@samcarey
Copy link

samcarey commented Feb 15, 2024

Describe the bug

Zenoh 0.10.1-rc (and main) require Rust version 1.72.0 and specify a dependency on Clap of "4.4.11", which can now resolve to Clap version "4.5" (as of February 7th), which now has a minimum supported rust version of 1.74.

To reproduce

Run cargo update and then try to build zenoh:

error: package `clap v4.5.0` cannot be built because it requires rustc 1.74 or newer, while the currently active rustc version is 1.72.0Either upgrade to rustc 1.74 or newer, or use
cargo update -p [email protected] --precise ver
where `ver` is the latest version of `clap` supporting rustc 1.72.0

System info

N/A

@YuanYuYuan
Copy link
Contributor

YuanYuYuan commented Feb 16, 2024

Hi @samcarey, I encountered the same problem as well. I think this is due to the limit of cargo, and a RFC has been proposed. clap-rs/clap#4811 (comment)

Bumping up the rust toolchain and upgrading the dependencies require more checks. The modification of async runtime going to be added will change the dependence as well. I suggest let's fix the version number of clap at this moment. And add this issue in the roadmap to remind us to upgrade the toolchain. @Mallets , any comments are welcome.

@Mallets
Copy link
Member

Mallets commented Feb 16, 2024

The problem is more rooted in Rust and cargo as it seems and it's not only specific to Zenoh.
Bumping the Rust MSRV would work for Zenoh this time, but this doesn't prevent the fact that we could break downstream dependencies as well. Nevertheless, one could argue that those dependencies are already broken since they are not able to build Zenoh...

An alternative approach would be to pin every dependency to an exact version with =, but I'm not sure that would work out with the dependencies of a dependency.

@YuanYuYuan
Copy link
Contributor

The problem is more rooted in Rust and cargo as it seems and it's not only specific to Zenoh. Bumping the Rust MSRV would work for Zenoh this time, but this doesn't prevent the fact that we could break downstream dependencies as well. Nevertheless, one could argue that those dependencies are already broken since they are not able to build Zenoh...

An alternative approach would be to pin every dependency to an exact version with =, but I'm not sure that would work out with the dependencies of a dependency.

I found that a new crate with clap >= 4.5.1 still can't compile even if we freeze the clap version inside zenoh to be less than 4.4.18.

@gabrik
Copy link
Contributor

gabrik commented Feb 20, 2024

I found that a new crate with clap >= 4.5.1 still can't compile even if we freeze the clap version inside zenoh to be less than 4.4.18.

I was looking at the dependencies and clap is a dependency of zenohd and examples, so a create that depends on zenoh should not be impacted by this.

So the problem is when building zenohd right? Not a crate that depends on zenoh

If this is the case using = to fix the version should solve the issue, tbh I do not understand why a crate should depend on zenohd

@YuanYuYuan
Copy link
Contributor

YuanYuYuan commented Feb 20, 2024

I found that a new crate with clap >= 4.5.1 still can't compile even if we freeze the clap version inside zenoh to be less than 4.4.18.

I was looking at the dependencies and clap is a dependency of zenohd and examples, so a create that depends on zenoh should not be impacted by this.

So the problem is when building zenohd right? Not a crate that depends on zenoh

If this is the case using = to fix the version should solve the issue, tbh I do not understand why a crate should depend on zenohd

To be clear, building either zenohd or any example depending on clap is fine. The problem is that users can't compile zenoh once they make a cargo update (inside zenoh repository) since it would upgrade the clap to 4.5.* and break the current MSRC used in zenoh.

I've also encountered this problem while developing on tokio-porting since I was working "inside" the zenoh repository. However, normal users or developers are expected to call the zenoh library outside the zenoh repository, which is just fine after a quick check.

Saying, a plain crate with dependencies of current zenoh and clap >= 4.5.1 ( 4.4.* is also fine) can compile without any errors. Running a cargo update outside is also fine. IMO, this is still an issue, but it's "internal" that we need to keep in mind when upgrading our dependencies. (But don't worry, our CI can check this failure.)

@fuzzypixelz
Copy link
Member

fuzzypixelz commented Feb 20, 2024

I found that a new crate with clap >= 4.5.1 still can't compile even if we freeze the clap version inside zenoh to be less than 4.4.18.

I was looking at the dependencies and clap is a dependency of zenohd and examples, so a create that depends on zenoh should not be impacted by this.

So the problem is when building zenohd right? Not a crate that depends on zenoh

If this is the case using = to fix the version should solve the issue, tbh I do not understand why a crate should depend on zenohd

The Cargo Binary Dependencies RFC is already in nightly Rust under -Z bindeps. It's supposed to allow crates to use Cargo binary packages in tests and build scripts.

I reproduced this kind of scenario here.

Even if we decide that this is an edge case not worth supporting at the moment and decide to go ahead with pinning the minor version, we should document it in the README.

In any case, using clap = "~4.4" is strictly better than clap = "=4.4.11" because the former leaves the patch flexible (so it accepts bugfixes and decreases the chances of conflicts). Increases in the MSRV of a crate are not considered SemVer-incompatible changes by Cargo (and the Rust community), the official policy is to treat this as a minor change. So for ensuring that dependency MSRVs don't cause issues like this anymore, a "Tilde" requirement should be enough.

As far as crate authors are concerned, only the latest Rust compiler is supported, so we will routinely run into these issues as our Rust toolchain (currently 1.72) gets older. Perhaps we should also put the choice of the toolchain into question.

@YuanYuYuan
Copy link
Contributor

I found that a new crate with clap >= 4.5.1 still can't compile even if we freeze the clap version inside zenoh to be less than 4.4.18.

I was looking at the dependencies and clap is a dependency of zenohd and examples, so a create that depends on zenoh should not be impacted by this.
So the problem is when building zenohd right? Not a crate that depends on zenoh
If this is the case using = to fix the version should solve the issue, tbh I do not understand why a crate should depend on zenohd

The Cargo Binary Dependencies RFC is already in nightly Rust under -Z bindeps. It's supposed to allow crates to use Cargo binary packages in tests and build scripts.

I reproduced this kind of scenario here.

Even if we decide that this is an edge case not worth supporting at the moment and decide to go ahead with pinning the minor version, we should document it in the README.

In any case, using clap = "~4.4" is strictly better than clap = "=4.4.11" because the former leaves the patch flexible (so it accepts bugfixes and decreases the chances of conflicts). Increases in the MSRV of a crate are not considered SemVer-incompatible changes by Cargo (and the Rust community), the official policy is to treat this as a minor change. So for ensuring that dependency MSRVs don't cause issues like this anymore, a "Tilde" requirement should be enough.

As far as crate authors are concerned, only the latest Rust compiler is supported, so we will routinely run into these issues as our Rust toolchain (currently 1.72) gets older. Perhaps we should also put the choice of the toolchain into question.

It's interesting that users can introducing zenohd into the test. And I'd like to point out that the native zenoh library right now can pass @fuzzypixelz 's PoC script since the reason to the failure is basically the same as the case I mentioned above.

In summary, we should NOT pin the version of clap at this moment, otherwise the PoC above or any other crates depending on zenoh and clap >= 4.5 at the same time would fail to compile. I think we can keep the Cargo.toml at this moment and upgrade the rust toolchain in the near future.

@fuzzypixelz
Copy link
Member

@samcarey May I ask why do you need to update the lockfile?

@Mallets
Copy link
Member

Mallets commented Jul 4, 2024

Closing as outdated. Feel free to re-open it in case it is still relevant.

@Mallets Mallets closed this as completed Jul 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants