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(config): warn if host is incompatible with the toolchain in rustup default #3980

Merged
merged 12 commits into from
Aug 5, 2024

Conversation

rami3l
Copy link
Member

@rami3l rami3l commented Aug 3, 2024

Closes #3972:

FWIW we do emit this warning for rustup toolchain install:

error!("DEPRECATED: future versions of rustup will require --force-non-host to install a non-host toolchain.");
warn!("toolchain '{name}' may not be able to run on this system.");
warn!(
"If you meant to build software to target that platform, perhaps try `rustup target add {}` instead?",
target_triple.to_string()
);

> uname -mrs
Darwin 23.5.0 arm64

> rustup toolchain install stable-unknown-linux-gnu --profile=minimal
error: DEPRECATED: future versions of rustup will require --force-non-host to install a non-host toolchain.
warn: toolchain 'stable-unknown-linux-gnu' may not be able to run on this system.
warn: If you meant to build software to target that platform, perhaps try `rustup target add aarch64-unknown-linux-gnu` instead?
info: syncing channel updates for 'stable-aarch64-unknown-linux-gnu'
info: latest update on 2024-07-25, rust version 1.80.0 (051478957 2024-07-21)
info: downloading component 'cargo'
info: downloading component 'rust-std'
^C⏎                           

... it could be that this is not covered by rustup default somehow. So adding the same warning to that path should address this issue.

#3972 (comment)

A unified ensure_installed() function also paves the way to addressing #2686.

@rami3l rami3l force-pushed the feat/toolchain-ensure branch 4 times, most recently from 0d81e09 to 30f64a7 Compare August 3, 2024 06:31
@rami3l rami3l changed the title refactor(distributable)!: replace install_if_not_installed() with ensure_installed() fix(config): call warn_if_host_is_incompatible() in ensure_install() Aug 3, 2024
src/cli/common.rs Outdated Show resolved Hide resolved
@rami3l rami3l marked this pull request as ready for review August 3, 2024 07:04
@rami3l rami3l requested review from rbtcollins and djc and removed request for rbtcollins August 3, 2024 07:04
@rami3l rami3l changed the title fix(config): call warn_if_host_is_incompatible() in ensure_install() fix(config): warn if host is incompatible with the toolchain in rustup default Aug 3, 2024
Copy link
Contributor

@djc djc left a comment

Choose a reason for hiding this comment

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

I like the direction here, would mainly like a cleaner commit history here to make it easier to review the details.

src/config.rs Outdated Show resolved Hide resolved
src/cli/common.rs Show resolved Hide resolved
src/cli/common.rs Outdated Show resolved Hide resolved
src/cli/common.rs Outdated Show resolved Hide resolved
@rami3l
Copy link
Member Author

rami3l commented Aug 3, 2024

@djc Thanks for your guidance! I'm a bit occupied with other stuff right now so I'll take another look later.

Just to be sure, I actually prefer removing --force-non-host and the related deprecation notice after searching for the existing usages I think we should keep the flag itself, but remove the deprecation notice and leave the warnings as-is. Is that okay to you?

@rami3l rami3l force-pushed the feat/toolchain-ensure branch 3 times, most recently from 588f718 to ed2bfde Compare August 5, 2024 03:29
@rami3l rami3l requested a review from djc August 5, 2024 06:08
src/config.rs Show resolved Hide resolved
@rami3l rami3l added this pull request to the merge queue Aug 5, 2024
Merged via the queue into rust-lang:master with commit d7cd595 Aug 5, 2024
27 checks passed
@rami3l rami3l deleted the feat/toolchain-ensure branch August 5, 2024 13:37
@rbtcollins
Copy link
Contributor

Just to be sure, I actually prefer removing --force-non-host and the related deprecation notice after searching for the existing usages I think we should keep the flag itself, but remove the deprecation notice and leave the warnings as-is. Is that okay to you?

@rami3l So the point of this was to support cross-rs properly (they need non-host toolchains), while stopping people that are just following their nose from getting stuck and confused. E.g. installing an arm64 toolchain on an amd64 machine and wondering why that breaks.

cross-rs should be updated by now I believe, so the appropriate action is to remove the warning and reject non-host toolchains unless the flag is provided.

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.

rustup default doesn't show proper warning when architecture is not compatible
3 participants