-
Notifications
You must be signed in to change notification settings - Fork 12.7k
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
Try to not reinstall tools in mingw CI #125546
Conversation
rustbot has assigned @Mark-Simulacrum. Use |
mingw CI looks happy https://github.com/rust-lang/rust/actions/runs/9235561579?pr=125546 |
@bors r+ |
…acrum Try to not reinstall tools in mingw CI Reinstalling the tools seems prone to failure (e.g. [latest](rust-lang#125529 (comment))) and is more work. It also seems unnecessary as CI actually uses a vendored tarball for builds. cc `@mati865`
…iaskrgr Rollup of 8 pull requests Successful merges: - rust-lang#125307 (tidy: stop special-casing tests/ui entry limit) - rust-lang#125375 (Create a triagebot ping group for Rust for Linux) - rust-lang#125413 (drop region constraints for ambiguous goals) - rust-lang#125433 (A small diagnostic improvement for dropping_copy_types) - rust-lang#125508 (Stop SRoA'ing `DynMetadata` in MIR) - rust-lang#125530 (cleanup dependence of `ExtCtxt` in transcribe when macro expansion) - rust-lang#125544 (Also mention my-self for other check-cfg docs changes) - rust-lang#125546 (Try to not reinstall tools in mingw CI) r? `@ghost` `@rustbot` modify labels: rollup
Is it possible that this PR has caused #125557 (comment) 🤔? Or is that completely out of question? 😅 |
…acrum Try to not reinstall tools in mingw CI Reinstalling the tools seems prone to failure (e.g. [latest](rust-lang#125529 (comment))) and is more work. It also seems unnecessary as CI actually uses a vendored tarball for builds. cc `@mati865`
It seems possible. @bors rollup=iffy |
Rollup of 7 pull requests Successful merges: - rust-lang#125307 (tidy: stop special-casing tests/ui entry limit) - rust-lang#125375 (Create a triagebot ping group for Rust for Linux) - rust-lang#125433 (A small diagnostic improvement for dropping_copy_types) - rust-lang#125508 (Stop SRoA'ing `DynMetadata` in MIR) - rust-lang#125530 (cleanup dependence of `ExtCtxt` in transcribe when macro expansion) - rust-lang#125544 (Also mention my-self for other check-cfg docs changes) - rust-lang#125546 (Try to not reinstall tools in mingw CI) r? `@ghost` `@rustbot` modify labels: rollup
Try to not reinstall tools in mingw CI Reinstalling the tools seems prone to failure (e.g. [latest](rust-lang#125529 (comment))) and is more work. It also seems unnecessary as CI actually uses a vendored tarball for builds. cc `@mati865`
@bors cancel Edit: Not sure how to cancel an ongoing merge attempt? |
@bors retry r- |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Ok, I've now tested it on an msvc runner and it passes. The trick was to not install msys2, which used to be necessary for run-make but apparently isn't any longer (perhaps because many tests have been ported to recipes). |
# Clean up and prepare the MSYS2 installation. MSYS2 is needed primarily for | ||
# the test suite (run-make), but is also used by the MinGW toolchain for assembling things. | ||
# Clean up and prepare the MSYS2 installation. | ||
# MSYS2 is used by the MinGW toolchain for assembling things. |
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 see MSYS2 anywhere in this script apart from the comment and filename, probably could be renamed to setup-windows-python.sh
or something like that.
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.
Yeah, I'm not convinced this script is still needed at all but I didn't want to change too much at once. Or if it is just needed by the mingw targets then it could be merged into the other script.
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 all Windows jobs need Python setup one way or another and after these changes this script is basically a custom actions/setup-python
step: https://docs.github.com/en/actions/automating-builds-and-tests/building-and-testing-python#specifying-a-python-version
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, that says "To maintain consistent behavior with other runners and to allow Python to be used out-of-the-box without the setup-python action, GitHub adds a few versions from the tools cache to PATH"
Consistent behaviour with other runners sounds like something we can rely on, no?
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.
We use custom Docker images on Linux builders, no idea about Mac. So this consistency won't really apply here.
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.
Ah, I see. I think I'll experiment with maybe using the action in a follow up PR.
@@ -39,10 +39,7 @@ if isWindows; then | |||
esac | |||
|
|||
if [[ "${CUSTOM_MINGW:-0}" == 0 ]]; then |
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 this is just a dead code so a followup PR could get rid of all CUSTOM_MINGW
occurrences.
@bors r+ rollup=never p=1000 |
🌲 The tree is currently closed for pull requests below priority 100. This pull request will be tested once the tree is reopened. |
☀️ Test successful - checks-actions |
Finished benchmarking commit (bdbbb6c): comparison URL. Overall result: ❌ regressions - no action needed@rustbot label: -perf-regression Instruction countThis is a highly reliable metric that was used to determine the overall result at the top of this comment.
Max RSS (memory usage)This benchmark run did not return any relevant results for this metric. CyclesThis benchmark run did not return any relevant results for this metric. Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 670.186s -> 669.111s (-0.16%) |
Reinstalling the tools seems prone to failure (e.g. latest) and is more work. It also seems unnecessary as CI actually uses a vendored tarball for builds.
cc @mati865