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

fix: update FindRust to work with rustup v1.28.0 #591

Merged
merged 1 commit into from
Dec 29, 2024

Conversation

rami3l
Copy link
Contributor

@rami3l rami3l commented Dec 28, 2024

Closes #590 by applying the following mapping:

Variable \ Rustup ver. v1.27.1 v1.28.0
$_TOOLCHAIN_OVERRIDE (override) (active)
$_TOOLCHAIN_DEFAULT (default) (default) or (active, default)

Rationale

Corrosion would like to find out the path for the active toolchain via rustup toolchain list --verbose from the toolchains marked (override) and/or (default): if (override) is present, then it's active, otherwise (default) would be active:

if (NOT DEFINED Rust_TOOLCHAIN)
if (NOT DEFINED _TOOLCHAIN_OVERRIDE)
set(_TOOLCHAIN_SELECTED "${_TOOLCHAIN_DEFAULT}")
else()
set(_TOOLCHAIN_SELECTED "${_TOOLCHAIN_OVERRIDE}")
endif()

In rustup v1.28.0 beta (or rust-lang/rustup#3225 to be more precise), we have recognized that this output format has become a common source of confusion among users in determining the active toolchain, so we now mark the active toolchain with (active) instead. Unfortunately, this has broken corrosion's parsing logic.

Verification

Changes verified locally via the following interactions:

> rustup --version
rustup 1.28.0 :: 1.27.1+544 (7ccf717e6 2024-12-23)
...

> rustup toolchain list --verbose # The "default" state.
stable-aarch64-apple-darwin (active, default) ...
nightly-aarch64-apple-darwin ...
nightly-2023-09-30-aarch64-apple-darwin ...

> rustup +nightly toolchain list --verbose # The "override" state.
stable-aarch64-apple-darwin (default) ...
nightly-aarch64-apple-darwin (active) ...
nightly-2023-09-30-aarch64-apple-darwin ...

> cmake -S. -Bbuild -URust_TOOLCHAIN
-- Rust Toolchain: stable-aarch64-apple-darwin
-- Rust Target: aarch64-apple-darwin
-- Found Rust: SOME/.rustup/toolchains/stable-aarch64-apple-darwin/bin/rustc (found version "1.83.0")
-- Configuring done (0.2s)
-- Generating done (0.1s)
...

> RUSTUP_TOOLCHAIN=nightly cmake -S. -Bbuild -URust_TOOLCHAIN
-- Rust Toolchain: nightly-aarch64-apple-darwin
-- Rust Target: aarch64-apple-darwin
-- Found Rust: SOME/.rustup/toolchains/nightly-aarch64-apple-darwin/bin/rustc (found version "1.85.0")
-- Configuring done (0.2s)
-- Generating done (0.1s)
...

Concerns

  • I'm not sure why -URust_TOOLCHAIN is required to make this work. Has it been somehow set to "" by something?
  • Am I doing the verification right? If not, how should I properly verify this change?

@jschwe
Copy link
Collaborator

jschwe commented Dec 28, 2024

The intended way to specify a toolchain is by running cmake -S. -Bbuild -DRust_TOOLCHAIN=xxx. It'st best to rm -rf build to start from a clean build dir when testing.
RUSTUP_TOOLCHAIN=nightly working is a bonus, but most users include corrosion from CMake files, so configuring corrosion via CMake variables is the standard case.

Copy link
Collaborator

@jschwe jschwe left a comment

Choose a reason for hiding this comment

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

LGTM, I'll be travelling today so I won't be able to make a new release today

@rami3l
Copy link
Contributor Author

rami3l commented Dec 28, 2024

@jschwe Thanks for the quick response! The CI error does look weird to me as it doesn't seem related to my change, is this a pre-existing issue?

@jschwe
Copy link
Collaborator

jschwe commented Dec 28, 2024

Yeah, the CI issue with installing cbindgen is unrelated. It's popped up recently and I haven't had time to investigate what changed and why it's only failing on Linux.

@jschwe jschwe merged commit a88bc9b into corrosion-rs:master Dec 29, 2024
40 checks passed
@jschwe
Copy link
Collaborator

jschwe commented Dec 29, 2024

@rami3l Thanks for the PR! Please do note, that I still need to do a backport to the v0.5 branch and cut a new release.

@rami3l rami3l deleted the fix/rustup-1-28-0 branch December 29, 2024 14:33
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.

[Bug]: corrosion breaks due to new rustup toolchain list format in rustup v1.28.0
2 participants