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.

Sign up for GitHub

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

Jump to bottom

Add a "Setup Python" action for github-hosted runners and remove unnecessary CUSTOM_MINGW environment variable #125590

Merged
merged 3 commits into from
May 28, 2024
Merged

Add a "Setup Python" action for github-hosted runners and remove unnecessary CUSTOM_MINGW environment variable #125590

merged 3 commits into from
May 28, 2024

Conversation

ChrisDenton
Copy link
Member

@ChrisDenton ChrisDenton commented May 26, 2024 •
edited by Kobzol
Loading

The Setup Python action isn't strictly necessary (even on Windows) but it is recommend by GitHub.

The CUSTOM_MINGW environment variable is redundant now as it's always set for mingw and always unset otherwise.

try-job: x86_64-mingw
try-job: x86_64-mingw
try-job: dist-x86_64-msvc
try-job: dist-x86_64-mingw

@rustbot
Copy link
Collaborator

rustbot commented May 26, 2024

r? @Kobzol

rustbot has assigned @Kobzol.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added A-testsuite Area: The testsuite used to check the correctness of rustc S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue. labels May 26, 2024
@ChrisDenton ChrisDenton changed the title Add a "Setup Python" action for self-hosted runners and remove unnecessary CUSTOM_MINGW environment variable Add a "Setup Python" action for github-hosted runners and remove unnecessary CUSTOM_MINGW environment variable May 26, 2024
@Kobzol
Copy link
Contributor

Kobzol commented May 28, 2024

Just to make sure that I understand the context, a custom MinGW was needed in the past, because the GitHub Windows runners had too old OS? Or is the reason that it is not needed now the fact that we have updated our minimum supported Windows versions?

@ChrisDenton
Copy link
Member Author

Just to make sure that I understand the context, a custom MinGW was needed in the past, because the GitHub Windows runners had too old OS? Or is the reason that it is not needed now the fact that we have updated our minimum supported Windows versions?

The windows-gnu runners do need a custom mingw to build rustc but they always need it so there's no reason to make it configurable via an environment variable. The script that uses CUSTOM_MINGW already has if isWindows && isKnownToBeMingwBuild so at that point CUSTOM_MINGW will always be set.

The windows-msvc runners don't need mingw to build rust as they use MSVC and LLVM tools. I'm not entirely clear why we used to install gnu build tools on windows-msvc runners but presumably it was needed at some point in the past. CI does need a way to run bash scripts and support for run-make tests (e.g. having a gnu make) but we don't need any special scripting to support that. It's already provided.

@Kobzol
Copy link
Contributor

Kobzol commented May 28, 2024

I see. Let's check if this works.

@bors try

@bors
Copy link
Contributor

bors commented May 28, 2024

⌛ Trying commit f931290 with merge ad92d5a...

bors added a commit to rust-lang-ci/rust that referenced this pull request May 28, 2024
@bors
Auto merge of rust-lang#125590 - ChrisDenton:mingw-ci-3, r=<try>
ad92d5a 
Add a "Setup Python" action for github-hosted runners and remove unnecessary `CUSTOM_MINGW` environment variable

The Setup Python action isn't strictly necessary ([even on Windows](rust-lang#125584)) but it is [recommend by GitHub](https://docs.github.com/en/actions/automating-builds-and-tests/building-and-testing-python#specifying-a-python-version).

The `CUSTOM_MINGW` environment variable is redundant now as it's always set for mingw and always unset otherwise.

try-job: x86_64-mingw
try-job: x86_64-mingw
try-job: dist-x86_64-msvc
try-job: dist-x86_64-mingw
@bors
Copy link
Contributor

bors commented May 28, 2024

☀️ Try build successful - checks-actions
Build commit: ad92d5a (ad92d5a4bb6eebae1c80572ff4f6997bf168c947)

@Kobzol
Copy link
Contributor

Kobzol commented May 28, 2024

Looks like it works.

@bors r+

@bors
Copy link
Contributor

bors commented May 28, 2024

📌 Commit f931290 has been approved by Kobzol

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 28, 2024
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request May 28, 2024
@matthiaskrgr
Rollup merge of rust-lang#125590 - ChrisDenton:mingw-ci-3, r=Kobzol
1c51557 
Add a "Setup Python" action for github-hosted runners and remove unnecessary `CUSTOM_MINGW` environment variable

The Setup Python action isn't strictly necessary ([even on Windows](rust-lang#125584)) but it is [recommend by GitHub](https://docs.github.com/en/actions/automating-builds-and-tests/building-and-testing-python#specifying-a-python-version).

The `CUSTOM_MINGW` environment variable is redundant now as it's always set for mingw and always unset otherwise.

try-job: x86_64-mingw
try-job: x86_64-mingw
try-job: dist-x86_64-msvc
try-job: dist-x86_64-mingw
@matthiaskrgr matthiaskrgr mentioned this pull request May 28, 2024
Closed
bors added a commit to rust-lang-ci/rust that referenced this pull request May 28, 2024
@bors
Auto merge of rust-lang#125663 - matthiaskrgr:rollup-ej3yuxm, r=matth…
767e12b 
…iaskrgr

Rollup of 6 pull requests

Successful merges:

 - rust-lang#117671 (NVPTX: Avoid PassMode::Direct for args in C abi)
 - rust-lang#124251 (Add an intrinsic for `ptr::metadata`)
 - rust-lang#125573 (Migrate `run-make/allow-warnings-cmdline-stability` to `rmake.rs`)
 - rust-lang#125590 (Add a "Setup Python" action for github-hosted runners and remove unnecessary `CUSTOM_MINGW` environment variable)
 - rust-lang#125598 (Make `ProofTreeBuilder` actually generic over `Interner`)
 - rust-lang#125637 (rustfmt fixes)

r? `@ghost`
`@rustbot` modify labels: rollup
@matthiaskrgr matthiaskrgr mentioned this pull request May 28, 2024
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request May 28, 2024
@bors
Auto merge of rust-lang#125665 - matthiaskrgr:rollup-srkx0v1, r=matth…
274499d 
…iaskrgr

Rollup of 5 pull requests

Successful merges:

 - rust-lang#117671 (NVPTX: Avoid PassMode::Direct for args in C abi)
 - rust-lang#125573 (Migrate `run-make/allow-warnings-cmdline-stability` to `rmake.rs`)
 - rust-lang#125590 (Add a "Setup Python" action for github-hosted runners and remove unnecessary `CUSTOM_MINGW` environment variable)
 - rust-lang#125598 (Make `ProofTreeBuilder` actually generic over `Interner`)
 - rust-lang#125637 (rustfmt fixes)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 86f6fae into rust-lang:master May 28, 2024
7 checks passed
@rustbot rustbot added this to the 1.80.0 milestone May 28, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request May 28, 2024
@rust-timer
Unrolled build for rust-lang#125590
4e91a28 
Rollup merge of rust-lang#125590 - ChrisDenton:mingw-ci-3, r=Kobzol

Add a "Setup Python" action for github-hosted runners and remove unnecessary `CUSTOM_MINGW` environment variable

The Setup Python action isn't strictly necessary ([even on Windows](rust-lang#125584)) but it is [recommend by GitHub](https://docs.github.com/en/actions/automating-builds-and-tests/building-and-testing-python#specifying-a-python-version).

The `CUSTOM_MINGW` environment variable is redundant now as it's always set for mingw and always unset otherwise.

try-job: x86_64-mingw
try-job: x86_64-mingw
try-job: dist-x86_64-msvc
try-job: dist-x86_64-mingw
@ChrisDenton ChrisDenton deleted the mingw-ci-3 branch May 28, 2024 20:41
@workingjubilee workingjubilee mentioned this pull request May 29, 2024
Closed
@Kobzol
Copy link
Contributor

Kobzol commented Jun 7, 2024

The setup Python action doesn't work for arm64 yet, which is needed for #126113. @ChrisDenton Do you have anything against removing it? I don't think that it provides any value to us, Ubuntu 20.04 comes with Python 3.8, and that should be more than enough for the few scripts that we have (at least, I don't think that this was a problem in practice in the past few years).

@ChrisDenton
Copy link
Member Author

Seems fine to remove it. I don't think there's any particular reason we need it.

@Kobzol Kobzol mentioned this pull request Jun 7, 2024
Merged
@Kobzol
Copy link
Contributor

Kobzol commented Jun 7, 2024

Ok, posted #126116.

bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 8, 2024
@bors
Auto merge of rust-lang#126116 - Kobzol:ci-remove-setup-python, r=jdno
655600c 
CI: remove `Setup Python` action

This action was added recently in rust-lang#125590, but it shouldn't really be needed, as our CI was working fine before without it. Our base Ubuntu 20.04 images use Python 3.8, while this action installed Python 3.12, but we don't really need that.

Since this action does not support ARM yet, this blocks rust-lang#126113. See rust-lang#125590 (comment).

r? `@jdno`
flip1995 pushed a commit to flip1995/rust-clippy that referenced this pull request Jun 28, 2024
@bors
Auto merge of #126116 - Kobzol:ci-remove-setup-python, r=jdno
fc573d1 
CI: remove `Setup Python` action

This action was added recently in rust-lang/rust#125590, but it shouldn't really be needed, as our CI was working fine before without it. Our base Ubuntu 20.04 images use Python 3.8, while this action installed Python 3.12, but we don't really need that.

Since this action does not support ARM yet, this blocks rust-lang/rust#126113. See rust-lang/rust#125590 (comment).

r? `@jdno`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mati865 mati865 mati865 approved these changes

Assignees

@Kobzol Kobzol

Labels
A-testsuite Area: The testsuite used to check the correctness of rustc S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.80.0
Development

Successfully merging this pull request may close these issues.
5 participants
@ChrisDenton @rustbot @Kobzol @bors @mati865