-
Notifications
You must be signed in to change notification settings - Fork 13.6k
bootstrap: refactor mingw dist and fix gnullvm #144659
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
base: master
Are you sure you want to change the base?
Conversation
@bors try jobs=dist-aarch64-windows-gnullvm,dist-x86_64-windows-gnullvm,dist-i686-windows-gnu,dist-x86_64-windows-gnu Also including regular windows-gnu to double-check there is no regression there. |
bootstrap: extract cc query into a new function try-job: dist-aarch64-windows-gnullvm try-job: dist-x86_64-windows-gnullvm try-job: dist-i686-windows-gnu try-job: dist-x86_64-windows-gnu
💔 Test failed (CI). Failed job:
|
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
@bors try jobs=dist-aarch64-windows-gnullvm,dist-x86_64-windows-gnullvm,dist-i686-mingw,dist-x86_64-mingw |
bootstrap: extract cc query into a new function try-job: dist-aarch64-windows-gnullvm try-job: dist-x86_64-windows-gnullvm try-job: dist-i686-mingw try-job: dist-x86_64-mingw
Tested the changes against parent commit using the following commands on Linux:
I hope this clearly (albeit in a lengthy way) shows no regression for |
cc7a7a5
to
e22a6ce
Compare
rustbot has assigned @albertlarsan68. Use |
Beta was branched already last week, right? |
Judging by their profile albertlarsan68 seems to be away right now, so I will take a liberty to reroll because the release is near and this might get backported for the next beta. r? rust-lang/bootstrap |
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.
The first commits are a nice cleanup, thanks.
} | ||
// FIXME(#144565): Remove this whole `let ...` |
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 don't understand the FIXME, it also refers to some other PR?
rustc_dlls.push("libgcc_s_dw2-1.dll"); | ||
} else { | ||
rustc_dlls.push("libgcc_s_seh-1.dll"); | ||
} | ||
} else { |
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 would spell gnullvm explicitly here, to make it more obvious which targets need libunwind.dll
.
Fixes #144533
The first two commits are NFC and only clean up the code, paving the way for the third commit. That said, I think they are worthwhile even without that fix - reusing the same function for two different outcomes was confusing.
The third commit is the fix for #144533, but due to the cross-compilation dance it requires a workaround to find the DLL since that logic really was meant only for Windows builders. That workaround is short-lived and will be removed as soon as gnullvm bootstraps itself.