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

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

Open
rami3l opened this issue Dec 27, 2024 · 5 comments · May be fixed by #591
Open

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

rami3l opened this issue Dec 27, 2024 · 5 comments · May be fixed by #591
Assignees
Labels
bug Something isn't working

Comments

@rami3l
Copy link

rami3l commented Dec 27, 2024

Current Behavior

Hi! @rami3l from the rustup team here.

After announcing rustup v1.28.0 beta, we have received a bug report that goes like so:

(After upgrading rustup from v1.27.1 to v1.28.0, corrosion is no longer able to find the toolchain it needs:)

CMake Warning (dev) at build/_deps/corrosion-src/cmake/FindRust.cmake:363 (message):
  Unexpected output from `rustc --version` for Toolchain
  `stable-x86_64-unknown-linux-gnu`: ``.

  Ignoring this toolchain.
Call Stack (most recent call first):
  build/_deps/corrosion-src/cmake/Corrosion.cmake:63 (find_package)
  build/_deps/corrosion-src/CMakeLists.txt:73 (include)
This warning is for project developers.  Use -Wno-dev to suppress it.

-- Rust Toolchain: 
CMake Error at build/_deps/corrosion-src/cmake/FindRust.cmake:23 (message):
  Could not find toolchain ''

  Available toolchains:

    `nightly-x86_64-unknown-linux-gnu`
    `nightly-2024-03-10-x86_64-unknown-linux-gnu`
    `1.70-x86_64-unknown-linux-gnu`
    `1.77-x86_64-unknown-linux-gnu`

Call Stack (most recent call first):
  build/_deps/corrosion-src/cmake/FindRust.cmake:469 (_findrust_failed)
  build/_deps/corrosion-src/cmake/Corrosion.cmake:63 (find_package)
  build/_deps/corrosion-src/CMakeLists.txt:73 (include)

rust-lang/rustup#4127

Expected Behavior

The project builds correctly.

Steps To Reproduce

Build any corrosion project with rustup v1.28.0.

The direct cause of this issue is the interaction between rust-lang/rustup#3225 and the following section of corrosion:

if (_TOOLCHAIN_RAW MATCHES "([a-zA-Z0-9\\._\\-]+)[ \t\r\n]?(\\(default\\) \\(override\\)|\\(default\\)|\\(override\\))?[ \t\r\n]+(.+)")
set(_TOOLCHAIN "${CMAKE_MATCH_1}")
set(_TOOLCHAIN_TYPE "${CMAKE_MATCH_2}")
set(_TOOLCHAIN_PATH "${CMAKE_MATCH_3}")
set(_TOOLCHAIN_${_TOOLCHAIN}_PATH "${CMAKE_MATCH_3}")
if (_TOOLCHAIN_TYPE MATCHES ".*\\(default\\).*")
set(_TOOLCHAIN_DEFAULT "${_TOOLCHAIN}")
endif()
if (_TOOLCHAIN_TYPE MATCHES ".*\\(override\\).*")
set(_TOOLCHAIN_OVERRIDE "${_TOOLCHAIN}")
endif()

Environment

- OS: Fedora 41

(However, this error should be platform-independent.)

Remarks

We can do a silent fix on our side, however, we would prefer informing you of this issue in advance so that we can figure out the best way forward together. I will be happy to work on a patch to corrosion if you'd like.

The original motivation of rust-lang/rustup#3225 is that it might not be immediately clear to the end user what the active toolchain is, I wonder if you have the same issue in this project in that respect.

Also, we are interested in providing a machine-facing output mode. Please feel free to provide suggestions WRT how you'd expect this to be done.

Many thanks in advance, and happy holidays :)

@rami3l rami3l added the bug Something isn't working label Dec 27, 2024
@jschwe
Copy link
Collaborator

jschwe commented Dec 27, 2024

Thanks for the heads-up!

Corrosion parses the rustup output to resolve the rustup proxy to a specific toolchain, so that the selected toolchain doesn't change between the CMake configure and build phase (Environment variables could be different).
A --json option (JSON is basically the only machine-readable format supported by CMake) would be great, but I guess we need a more short-term fix.

We had a similar breakage with Rust 1.79 (see #501), and I would also expect this change to break a couple of users.
Users can easily vendor corrosion, and the recommended approach is to pin a specific version, so fixes won't reach users automatically. How much time do we have until the release will be pushed?

@jschwe jschwe pinned this issue Dec 27, 2024
@rami3l
Copy link
Author

rami3l commented Dec 27, 2024

@jschwe The good news is that the rustup project is not on Rust's release train; we are rather the rails supporting that train, if you'd like. Although we do wish to ship a release soon, this time we should not be in a real hurry; I'd expect the beta phase to last at least a few weeks.

May I ask what precisely is corrosion relying on rustup toolchain list for? According to your previous comment it looks like you're looking exactly for the active toolchain (i.e. the one that commands such as cargo will refer to)? If so, this will actually be easier with the new output format.

I'd also be glad to investigate @kanru's advice of changing the middle segment of the output line to (default) (active) for example, if that happens to make your project work again; but I'm afraid this is not a very sustainable approach.

@rami3l
Copy link
Author

rami3l commented Dec 28, 2024

@jschwe Let me guess: you actually just want to get the active toolchain path with this snippet...

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

... however you might have tried the following options and concluded that neither is satisfactory:

  • rustup show active-toolchain. This seems like the most obvious option, however it is somehow not willing to give you the toolchain path...
  • rustup toolchain list --verbose. This does give you the toolchain paths, but you have to manually deduce what the active toolchain is :|

... so there's definitely something to be improved on our side: I've made rustup#4129 for this.

OTOH let me take some time to have a look at your code. I'm not a CMake expert so please bear with me :)

@jschwe
Copy link
Collaborator

jschwe commented Dec 28, 2024

if that happens to make your project work again; but I'm afraid this is not a very sustainable approach.

Ideally, rustup would provide the information we need via a stable JSON format. Maybe we could get a JSON output format before the human readable output is changed again?

you actually just want to get the active toolchain path with this snippet...

Yes, basically that. But we do need/want the whole list of toolchains (including paths), since we want the user to be able to select a toolchain via the Rust_TOOLCHAIN variable (e.g. Rust_TOOLCHAIN=nightly should select the nightly compiler).
Additionally when using a CMake TUI / GUI tool we want to be able to change the active Rust toolchain for corroson by cycling through a list of available toolchains (you can try this by e.g. running ccmake build and scrolling to the Rust_TOOLCHAIN variable and cycling through the available variables).

Upon thinking about this again, probably corrosion should delegate determining the active toolchain to rustup by using one of the override options when calling rustup toolchain list (would that work?).

Perhaps we should use rustup show active-toolchain to resolve the active toolchain and separately run rustup toolchain list --verbose to get a list of the paths, if this allows us to simplify the regex and make it more robust.

@rami3l
Copy link
Author

rami3l commented Dec 28, 2024

Ideally, rustup would provide the information we need via a stable JSON format. Maybe we could get a JSON output format before the human readable output is changed again?

@jschwe I wish something like that could happen, yes. But being a volunteer I can't give any words of guarantee at the moment, and I can't help worrying that something this will take quite a lot of bikeshedding...

As you can see right now, rather than providing what we are already providing you in some machine-readable format, it's more about cleaning up the overall output logic from our side to make sure that the output contents are satisfactory. That said, I believe a transliteration to JSON will be very easy once we have ironed out the sharp edges such as rust-lang/rustup#4129.

But we do need/want the whole list of toolchains (including paths), since we want the user to be able to select a toolchain via the Rust_TOOLCHAIN variable (e.g. Rust_TOOLCHAIN=nightly should select the nightly compiler). Additionally when using a CMake TUI / GUI tool we want to be able to change the active Rust toolchain for corroson by cycling through a list of available toolchains

I see. Thanks for the details!

Upon thinking about this again, probably corrosion should delegate determining the active toolchain to rustup by using one of the override options when calling rustup toolchain list (would that work?).

I'm sorry but I'm not sure I have understood what you meant by this... But if you mean something like specifying a toolchain override when running rustup, e.g. rustup +nightly show active-toolchain or rustup +nightly show active-toolchain, then yes, rustup will respond to this.

Perhaps we should use rustup show active-toolchain to resolve the active toolchain and separately run rustup toolchain list --verbose to get a list of the paths, if this allows us to simplify the regex and make it more robust.

This might be a more ideal approach indeed.

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.

2 participants