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

Don't show deprecation warning without OS #2854

Merged
merged 2 commits into from
Nov 11, 2021

Conversation

pjsier
Copy link
Contributor

@pjsier pjsier commented Sep 28, 2021

Closes #2837. Don't show a deprecation warning when an OS isn't present in the PartialTargetTriple. This way running rustup install stable-x86_64 won't throw a warning.

Let me know if there's anything I should change here, thanks!

@pjsier pjsier changed the title don't show deprecation warning without OS Don't show deprecation warning without OS Sep 28, 2021
@pjsier pjsier force-pushed the deprecation-warning-stable-x86 branch from b4f49fa to 3d9e1af Compare September 28, 2021 01:24
@kinnison
Copy link
Contributor

kinnison commented Oct 9, 2021

I think instead it'd make more sense to fully expand the inputs with the default host setting instead. That way we're actually going to test the compatibility as intended

@pjsier
Copy link
Contributor Author

pjsier commented Oct 9, 2021

Thanks for the review! I'm not sure I'm understanding, do you mean setting the defaults the way it's done here?

rustup/src/dist/dist.rs

Lines 399 to 407 in a9e1989

// If OS was specified, don't default to host environment, even if the OS matches
// the host OS, otherwise cannot specify no environment.
let env = if self.target.os.is_some() {
self.target.env
} else {
self.target.env.or(host_env)
};
let arch = self.target.arch.unwrap_or(host_arch);
let os = self.target.os.unwrap_or(host_os);

@pjsier pjsier force-pushed the deprecation-warning-stable-x86 branch from 990eb4c to a958387 Compare October 19, 2021 00:28
@pjsier
Copy link
Contributor Author

pjsier commented Oct 19, 2021

I'm not sure why the macOS build is failing, but I tried to make some updates here. Let me know if this is what you had in mind!

@kinnison
Copy link
Contributor

kinnison commented Nov 8, 2021

I think we'd do better to simply ensure we fully resolve the specifiers before calling the function, rather than trying to make more heuristics in the function itself.

@pjsier pjsier force-pushed the deprecation-warning-stable-x86 branch from cabb3e6 to 9cd22db Compare November 8, 2021 13:31
@pjsier
Copy link
Contributor Author

pjsier commented Nov 8, 2021

Thanks for taking a look! I think I misunderstood initially, and I missed the resolve method on PartialToolchainDesc before. I think this is more what you had in mind but let me know if it isn't

@pjsier pjsier force-pushed the deprecation-warning-stable-x86 branch from 9cd22db to 48aac03 Compare November 8, 2021 13:37
@kinnison
Copy link
Contributor

kinnison commented Nov 8, 2021

That does indeed look closer to what I imagined; I'd like to know for sure that this solves the issue reported in #2837 and #2672 - I'm not currently near a Windows box though, are you? If not we should definitely add a test case into the test suite for this.

@pjsier
Copy link
Contributor Author

pjsier commented Nov 9, 2021

Makes sense, I have a Windows machine but a test case makes sense either way so I added on here. Let me know if the condition is too specific to the error message, but otherwise it seems to work across architectures

Copy link
Contributor

@kinnison kinnison left a comment

Choose a reason for hiding this comment

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

While this is tightly coupled to the error message, that's fine because we can fix the test if we reword the message, thank you for your effort.

@kinnison kinnison merged commit 3b99e48 into rust-lang:master Nov 11, 2021
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.

Deprecation message when installing stable-x86_64 on x86_64
2 participants