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

rust: upgrade clap from 3.0.0-beta.2 to 3.2.23 #6040

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

wookayin
Copy link

@wookayin wookayin commented Nov 11, 2022

TL;DR) Upgrade clap dependency for rustboard, from 3.0.0-beta.2 to 3.2.23

Motivation for features / changes

I am trying to use rustboard_core as a dependency to some rust project.
When I added rustboard_core v0.6.1 as a dependency, somehow the version of
resolved clap library was 3.2.23, not 3.0.0-beta.2 as pinned by tensorboard.

(More off-topic details about clap version resolution)

I am not knowledgable about rust's build system, but having the following dependencies in my downstream project resolves clap to be 3.2.23, though I expect it to be 3.0.0-beta.2:

[dependencies]
rustboard = { git = "https://github.com/tensorflow/tensorboard", tag = "2.10.1" }
clap = "3.0.0-beta.2"

$ cargo tree
myproject v0.5.0-alpha.0 (...)
 ...
└── rustboard v0.7.0-alpha.0 (https://github.com/tensorflow/tensorboard?tag=2.10.1#a74c10dd)
    ├── ...
    ├── clap v3.2.23
    │   ├── atty v0.2.14
    │   │   └── libc v0.2.137
    │   ├── bitflags v1.3.2
    │   ├── clap_lex v0.2.4
    ...

Rust will resolve all the transitive dependencies (myproject -> rustboard -> {clap, clap_derive, clap_lex, ...}) to the latest compatible versions as per the semantic versioning resolves to an unexpected version (cf. https://stackoverflow.com/questions/62933825/why-we-need-to-specify-all-dependencies-including-transitives-in-rust), but in our case 3.0.0-beta is NOT compatible with 3.2.x despite the semantic versioning.

cargo/rustc version: 1.64.0

A workaround for the downstream project would be pin an exact version for all the transitive dependencies recursively:

# Cargo.toml

[dependencies]
rustboard = { git = "https://github.com/tensorflow/tensorboard", tag = "2.11.0" }
clap = "=3.0.0-beta.2"
clap_derive = "=3.0.0-beta.2"

Anyway clap 3.x version has introduced some breaking changes,
which 3.0.0-beta.2 version did not include. For the sake of future
development and better dependency resolution with downstream packages,
it would be a good idea to migrate to stable versions of clap.

Technical description of changes

Bump up clap dependency to 3.2.23 (the last version in 3.x series).

Adapt to breaking API changes in clap 3.x series:

  • use clap::Clap -> clap::Parser
  • AllowEmptyValues have been removed (clap#2360) as this is the new
    default option.

Screenshots of UI changes

N/A

Detailed steps to verify changes work correctly (as executed by you)

cd tensorboard/data/server/
cargo test
cargo build --release
./target/release/rustboard

There is only changes in CLI interface, no expected changes on the core
part or the business logic.

Alternate designs / implementations considered

The latest version of clap is v4.x, probably has more deprecations and
breaking changes since v3.x version. I was thinking about upgrading clap
to v4.0.x versions, but I ended up choosing a rather simple way for now.

/cc @wchargin

@wookayin
Copy link
Author

As a bazel newbie, I am actually not quite sure about updating bazel dependencies by running cargo raze. Please help me if you think the changes in c4ef1e6 (so many new files?) are inappropriate.

@groszewn
Copy link
Contributor

Hi @wookayin, thanks for the contribution! The number of files from cargo raze is not unexpected.

Seems like the build is failing with your updates, potentially related to bazelbuild/rules_rust#459.

@wookayin
Copy link
Author

wookayin commented Nov 15, 2022

@groszewn Hi, thanks for your comment. In my environments cargo raze produce a different result than in the CI jobs. Is it possibly due to a different cargo/rust version error? (I'm using 1.64 where CI jobs are using 1.58). I'm not very knowledgeable about rust yet, so any hints would be appreciated.

@wookayin
Copy link
Author

Figured out, it was cargo-raze version needed to be pinned at v0.13.0: https://github.com/tensorflow/tensorboard/blob/master/.github/workflows/ci.yml#L268 -- I was using v0.16.0 (the latest). This particular cargo-raze version choice should be documented...

@groszewn
Copy link
Contributor

I tested with the latest rust/cargo versions and experienced the same error.

If you take a look at the generated third_party/rust/remote/BUILD.clap-3.2.23.bazel, you'll notice that the data section of rust_library is empty, which seems to track with bazelbuild/rules_rust#459.

In order to include the data dependency, you can add the following section to Cargo.toml in order for the build to pass:

[package.metadata.raze.crates.clap.'3.2.23']
data_attr = "glob([\"examples/demo.md\"])"

@wookayin
Copy link
Author

wookayin commented Nov 16, 2022

Thanks for the info. I managed to make the cargo raze step pass (tested Github Actions), but there is a build failure about crossbeam-rs/crossbeam#803; rustc toolchain is required to be 1.61 or higher or crossbeam-epoch pinned at 0.9.7 (This PR was bumping the version form 0.9.1 to 0.9.11). Let me try fixing those build errors and get back to you.

@groszewn
Copy link
Contributor

groszewn commented Dec 6, 2022

@wookayin just to circle back on this, I've updated the rust and cargo raze version in CI (#6089, #6091, #6093), which may simplify your needed updates.

Adapt to breaking API changes in clap 3.x series:

- use clap::Clap -> clap::Parser
- AllowEmptyValues have been removed (clap#2360) as this is the new
  default option.
Based on rustc==1.65.0 and cargo-raze==0.16.1

```
$ cd tensorboard/data/server/
$ rm -rf ../../../third_party/rust && cargo fetch && cargo raze
```
@wookayin
Copy link
Author

wookayin commented Dec 6, 2022

Thanks! I tried rustc==1.65.0 and cargo-raze==0.16.1 which should easily address the previous failure on the crossbeam dependency. @groszewn Can you please help with approving running workflows?

@groszewn
Copy link
Contributor

Hi @wookayin, just wanted to circle back on this. I updated the rustc/cargo versions a while back (#6089, #6091, #6093, #6106) and approved running workflows, but seems like there's still some issues to work through with these changes.

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.

2 participants