-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
Bootstrap: detect Windows based on sys.platform #73691
Conversation
(rust_highfive has picked a reviewer for you, use r? to override) |
What effect does this have on the branches where we detect MSYS/MinGW to build |
It's going to build |
Perhaps we could move the |
Sorry for the broken patch! Yes, I was just thinking to move this fallback to the end of the |
5a8d27d
to
8b368db
Compare
# Some Windows platforms might have a `uname` command that returns a | ||
# non-standard string (e.g. gnuwin32 tools returns `windows32`). In | ||
# these cases, fall back to using sys.platform. | ||
return 'x86_64-pc-windows-msvc' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, is there a reason not to directly compare with windows32 here? (FWIW windows32 to me sounds like a 32-bit platform, but maybe that's historical baggage or something?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that's historical baggage of the gnuwin32 tools (they also return i686-pc
for uname -m
even on a 64-bit system). I didn't find a 64-bit equivalent, so this is probably not a reliable way to determine the actual architecture.
My thinking here was to provide a default for Windows platforms with any non-standard uname
value (same as not having uname
), but I can certainly change this to a direct comparison with windows32. What would be best?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, so I think this is probably fine -- we can always limit the scope later, if necessary, or add more conditionals.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the review!
@bors r+ |
📌 Commit 8b368db has been approved by |
…=Mark-Simulacrum Bootstrap: detect Windows based on sys.platform Closes rust-lang#73689.
…=Mark-Simulacrum Bootstrap: detect Windows based on sys.platform Closes rust-lang#73689.
…=Mark-Simulacrum Bootstrap: detect Windows based on sys.platform Closes rust-lang#73689.
…arth Rollup of 12 pull requests Successful merges: - rust-lang#72771 (Warn if linking to a private item) - rust-lang#72937 (Fortanix SGX target libunwind build process changes) - rust-lang#73485 (Perform obligation deduplication to avoid buggy `ExistentialMismatch`) - rust-lang#73529 (Add liballoc impl SpecFromElem for i8) - rust-lang#73579 (add missing doc links) - rust-lang#73627 (Shortcuts for min/max on double-ended BTreeMap/BTreeSet iterators) - rust-lang#73691 (Bootstrap: detect Windows based on sys.platform) - rust-lang#73694 (Document the Self keyword) - rust-lang#73718 (Document the super keyword) - rust-lang#73728 (Document some invariants correctly/more) - rust-lang#73738 (Remove irrelevant comment) - rust-lang#73765 (Remove blank line) Failed merges: r? @ghost
Closes #73689.