Skip to content

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

Merged
jschwe merged 1 commit into
corrosion-rs:masterfrom
rami3l:fix/rustup-1-28-0
Dec 29, 2024
Merged

jschwe merged 1 commit into
corrosion-rs:masterfrom
rami3l:fix/rustup-1-28-0

Conversation

@rami3l

@rami3l rami3l commented Dec 28, 2024 •

Copy link
Copy Markdown
Contributor

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

jschwe commented Dec 28, 2024

Copy link
Copy Markdown
Collaborator

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.

@jschwe jschwe left a comment

Copy link
Copy Markdown
Collaborator

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

rami3l commented Dec 28, 2024 •

Copy link
Copy Markdown
Contributor Author

@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

jschwe commented Dec 28, 2024

Copy link
Copy Markdown
Collaborator

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
@jschwe

jschwe commented Dec 29, 2024 •

Copy link
Copy Markdown
Collaborator

@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.

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