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

Improvements to building and CI for mingw/msys #121182

Merged
merged 3 commits into from
Feb 25, 2024
Merged

Conversation

majaha
Copy link
Contributor

@majaha majaha commented Feb 16, 2024

I was getting error messages when trying to follow the build instructions the mingw build for Rust, and managed to track the issue down to an incomparability of Rust's bootstrap program with MSYS2's version of git. Essentially, the problem is that MSYS2's git works in emulated unix-y paths, but bootstrap expects a Windows path. I found a workaround for this by using relative paths instead of absolute paths.

Along with that fix, this PR also updates the build instructions for MinGW to be compatible with modern versions of MSYS2, and some changes to CI to make sure that MSYS2's version of git is tested. In particular, I'm suggesting using the MSYS2 github action specially made for this purpose, which is much less hacky than the old approach and gives us more control of what packages are installed. I also cleaned up as many alternate versions of key tools as I could find from PATH, to avoid accidental usage, and cleaned up some abuses of the CUSTOM_MINGW environment variable.

This fixes #105696 and fixes #117567

src/bootstrap runs git to find the root of the repository, but this can
go awry when building in MSYS for the mingw target. This is because
MSYS git returns a unix-y path, but bootstrap requires a Windows-y path.
@rustbot
Copy link
Collaborator

rustbot commented Feb 16, 2024

r? @Mark-Simulacrum

rustbot has assigned @Mark-Simulacrum.
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-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue. labels Feb 16, 2024
@rustbot
Copy link
Collaborator

rustbot commented Feb 16, 2024

This PR modifies src/bootstrap/src/core/config.

If appropriate, please update CONFIG_CHANGE_HISTORY in src/bootstrap/src/utils/change_tracker.rs.

@Mark-Simulacrum
Copy link
Member

cc @mati865 -- perhaps you can weigh in here? My knowledge of Windows isn't deep enough to say whether this moves us in the right direction or how closely affiliated the msys2 org/github action is with the upstream project (i.e. is trusting it equivalent to trusting msys2 artifacts?). We may still want to replicate the artifacts as I think we were doing before into our own cache regardless...

@mati865
Copy link
Contributor

mati865 commented Feb 18, 2024

how closely affiliated the msys2 org/github action is with the upstream project

msys2/setup-msys2 is the official GH Action maintained by MSYS2 and the recommend way to install it.

whether this moves us in the right direction

I'm currently ill and don't want to make any statements because of my impaired ability to understand the code.
Since the problem in #105696 is caused by MSYS2 git returning UNIX style paths (like /c/rust/src/whatever) I think it might be simpler to just call cygpath (with -w or -m depending or what bootstrap expects) on paths passed to obtained from git.
Conversion from UNIX to Windows (cygpath -w) would give C:\rust\src\whatever and conversion to mixed (-cygpath -m) would give give C:/rust/src/whatever.
Other alternative would be to use a library that would give bootstrap the proper path from the beginning like gix #110369 or git2-rs.

@majaha
Copy link
Contributor Author

majaha commented Feb 18, 2024

I did think about Cygpath, but just getting git to give us the path of ../../../s we need seemed like a simpler solution to me.

@mati865
Copy link
Contributor

mati865 commented Feb 21, 2024

In overall looks good but I'm not sure whether Rust should install MSYS2 each time as it adds another possible point of failure to the CI. However if the caching works good this might not be a problem.

@Mark-Simulacrum
Copy link
Member

I'm going to go ahead and approve. I'm not an expert on this area but my sense is that this should be easy to revert if needed, so that's my preferred approach here.

@bors r+

@bors
Copy link
Contributor

bors commented Feb 24, 2024

📌 Commit e27c472 has been approved by Mark-Simulacrum

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Feb 24, 2024

🌲 The tree is currently closed for pull requests below priority 50. This pull request will be tested once the tree is reopened.

@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 Feb 24, 2024
@bors
Copy link
Contributor

bors commented Feb 25, 2024

⌛ Testing commit e27c472 with merge 0ecbd06...

@bors
Copy link
Contributor

bors commented Feb 25, 2024

☀️ Test successful - checks-actions
Approved by: Mark-Simulacrum
Pushing 0ecbd06 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Feb 25, 2024
@bors bors merged commit 0ecbd06 into rust-lang:master Feb 25, 2024
12 checks passed
@rustbot rustbot added this to the 1.78.0 milestone Feb 25, 2024
@ChrisDenton
Copy link
Member

Just a note but it seems that the mingw builders are now the slowest by roughly 20 to 30 minutes.

Job Time
auto - i686-mingw 2h 22m
auto - x86_64-mingw 2h 20m
auto - dist-x86_64-linux-alt 1h 55m

This is a fair increase from the roughly 1hr 40m it took before. I don't know if that was expected.

@Mark-Simulacrum
Copy link
Member

That doesn't sound expected to me. I'd be in favor of reverting and adjusting as needed before relanding.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (0ecbd06): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

This benchmark run did not return any relevant results for this metric.

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-2.0% [-2.0%, -2.0%] 1
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) -2.0% [-2.0%, -2.0%] 1

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 651.083s -> 649.512s (-0.24%)
Artifact size: 311.22 MiB -> 311.19 MiB (-0.01%)

@mati865
Copy link
Contributor

mati865 commented Feb 26, 2024

Agreed. Initially, I thought it might be a cache issue that would hopefully improve. But on second thought, I think it's just the slowness of Cygwin/MSYS2 Git.
I'll try to check the logs later today, but I think the revert is well justified.

@mati865
Copy link
Contributor

mati865 commented Feb 27, 2024

Sorry, haven't managed to take a look yesterday.
Looking at auto branch build:

So indeed everything that uses Git became much slower and I don't see any way to regain the performance other than reverting CI changes part of this PR.

@majaha
Copy link
Contributor Author

majaha commented Feb 28, 2024

Unfortunately, MSYS's Git is just slow. I thought it would be neater if it were tested in CI so that we can confidently suggest using it in the build instructions. But if it really is unbearably slow, we could perhaps take another approach. Maybe we could tell people to steer clear of MSYS Git and just install Git-for-Windows instead, although that's a bit of a pain to have to install a separate thing.

I don't understand the difference in time for the "checkout the source code" step, there shouldn't be a difference. Although taking a closer look, maybe the "disable git crlf conversion" step isn't applying to the checkout action because the config is going in the wrong home directory, and the extra work of expanding the line endings accounts for the extra time taken? (We could run that script twice, once in MSYS Bash and once in Git Bash, to make sure it gets configured everywhere).

For install-msys2.sh, that script is also installing things from network, which could be slowing it down. But it's also quite possible that deleting all those files is also what's slowing it down. (Maybe we could just mv them off path instead?)

I think quite a lot of this would be sped up by just moving back to using Git-for-Windows for mingw builds, but if we do that we should probably list it as a stronger requirement in the build instructions.

@mati865
Copy link
Contributor

mati865 commented Mar 1, 2024

I don't understand the difference in time for the "checkout the source code" step, there shouldn't be a difference.

Sorry, I was sleepy and haven't realised MSYS2 is installed after the checkout. Longer checkout must have been a noise.

For install-msys2.sh, that script is also installing things from network, which could be slowing it down. But it's also quite possible that deleting all those files is also what's slowing it down. (Maybe we could just mv them off path instead?)

If you enable timestamps you can see:

Tue, 27 Feb 2024 13:42:03 GMT installing pactoys...
Tue, 27 Feb 2024 13:49:37 GMT resolving dependencies...
Tue, 27 Feb 2024 13:49:38 GMT looking for conflicting packages...
Tue, 27 Feb 2024 13:49:38 GMT
Tue, 27 Feb 2024 13:49:38 GMT Packages (33) mingw-w64-x86_64-brotli-1.1.0-1 

Which means CI spent 7 minutes between lines

pacman -S --noconfirm pactoys
and
pacboy -S --noconfirm cmake:p

Pactoys install is fast because it's tiny so calling rm is the only thing left.

I think quite a lot of this would be sped up by just moving back to using Git-for-Windows for mingw builds, but if we do that we should probably list it as a stronger requirement in the build instructions.

I'll revive #110369 over the weekend and see if build time of newer version is acceptable. It should be even faster than Git for Windows.

@mati865
Copy link
Contributor

mati865 commented Mar 4, 2024

I'll revive #110369 over the weekend and see if build time of newer version is acceptable. It should be even faster than Git for Windows.

With the latest version the time goes from 10s to 13s on my PC so about 30% increase, definitely an improvement but still not ideal.

@majaha are you going to revert CI changes?

@majaha
Copy link
Contributor Author

majaha commented Mar 5, 2024

With the latest version the time goes from 10s to 13s on my PC so about 30% increase, definitely an improvement but still not ideal.
Sorry, could you explain what those numbers were measurements of?

I'd rather not revert all the CI changes as I think parts of it are more sensible than what was there before. But I'm willing to try and fix up the main issues, if you wouldn't mind pointing me to what you think they are.

The biggest issue is this: bootstrap tests: [TIMING] core::build_steps::test::Bootstrap Bootstrap -- 1939.995
Running the tests for bootstrap takes over 30 minutes by itself. I've noticed that bootstrap became excruciatingly slow with MSYS's git in my own testing. I'm sure this will be fixed by reverting back to Git-for-Windows, so I think we should start there. I'll try to make a pull request for that tomorrow evening.

@mati865
Copy link
Contributor

mati865 commented Mar 6, 2024

Sorry, could you explain what those numbers were measurements of?

It was regarding part the comment that I quoted:

I'll revive #110369 over the weekend and see if build time of newer version is acceptable. It should be even faster than Git for Windows.

What that PR did was avoiding calling Git binary by using a native library so we avoid Cygwin issues. The downside is longer build time for bootstrap binary and this is shown by those numbers (tested on Linux).

I'm willing to try and fix up the main issues, if you wouldn't mind pointing me to what you think they are.

As you already noticed reverting back to GfW would cut down significant amount of time.
Another thing to consider is doing mv instead of rm if you want to get rid of preinstalled tools but I think it might be unnecessary.

majaha added a commit to majaha/rust that referenced this pull request Mar 7, 2024
In rust-lang#121182 the mingw build was
changed to use MSYS2's version of Git. This commit reverts that, as
it was considered too slow.
@majaha
Copy link
Contributor Author

majaha commented Mar 7, 2024

I've made the pull request to revert the git change: #122125

workingjubilee added a commit to workingjubilee/rustc that referenced this pull request Mar 10, 2024
…crum

Revert back to Git-for-Windows for MinGW CI builds

Following discussion in rust-lang#121182 it was decided to revert using MSYS2 Git for mingw builds.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Mar 10, 2024
…crum

Revert back to Git-for-Windows for MinGW CI builds

Following discussion in rust-lang#121182 it was decided to revert using MSYS2 Git for mingw builds.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Mar 10, 2024
…crum

Revert back to Git-for-Windows for MinGW CI builds

Following discussion in rust-lang#121182 it was decided to revert using MSYS2 Git for mingw builds.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Mar 10, 2024
…crum

Revert back to Git-for-Windows for MinGW CI builds

Following discussion in rust-lang#121182 it was decided to revert using MSYS2 Git for mingw builds.
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Mar 10, 2024
Rollup merge of rust-lang#122125 - majaha:mingw_ci_new, r=Mark-Simulacrum

Revert back to Git-for-Windows for MinGW CI builds

Following discussion in rust-lang#121182 it was decided to revert using MSYS2 Git for mingw builds.
@majaha
Copy link
Contributor Author

majaha commented Mar 11, 2024

Having a quick flick through this log: https://github.com/rust-lang-ci/rust/actions/runs/8220783357/job/22480355462

It seems like cleaning up the tools by rm-ing them is taking four to seven minutes. I'm going to go ahead and write the patch that moves them instead.

@majaha
Copy link
Contributor Author

majaha commented Mar 11, 2024

Patch is up over here: #122321

github-actions bot pushed a commit to rust-lang/miri that referenced this pull request Mar 12, 2024
Revert back to Git-for-Windows for MinGW CI builds

Following discussion in rust-lang/rust#121182 it was decided to revert using MSYS2 Git for mingw builds.
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 17, 2024
`mv` tools off the path instead of `rm -r`-ing them in `install-msys2.sh`

This is a follow up patch to rust-lang#121182

r? `@Mark-Simulacrum`
github-actions bot pushed a commit to rust-lang/miri that referenced this pull request Mar 19, 2024
`mv` tools off the path instead of `rm -r`-ing them in `install-msys2.sh`

This is a follow up patch to rust-lang/rust#121182

r? `@Mark-Simulacrum`
lnicola pushed a commit to lnicola/rust-analyzer that referenced this pull request Apr 7, 2024
`mv` tools off the path instead of `rm -r`-ing them in `install-msys2.sh`

This is a follow up patch to rust-lang/rust#121182

r? `@Mark-Simulacrum`
RalfJung pushed a commit to RalfJung/rust-analyzer that referenced this pull request Apr 27, 2024
`mv` tools off the path instead of `rm -r`-ing them in `install-msys2.sh`

This is a follow up patch to rust-lang/rust#121182

r? `@Mark-Simulacrum`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-testsuite Area: The testsuite used to check the correctness of rustc merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue.
Projects
None yet
7 participants