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

use sccache on Windows #3618

Merged
merged 7 commits into from
Feb 26, 2021
Merged

use sccache on Windows #3618

merged 7 commits into from
Feb 26, 2021

Conversation

Be-ing
Copy link
Contributor

@Be-ing Be-ing commented Feb 5, 2021

No description provided.

@Be-ing
Copy link
Contributor Author

Be-ing commented Feb 25, 2021

Still not working after #3589...

@Be-ing Be-ing force-pushed the sccache branch 4 times, most recently from 0442210 to 160dd1d Compare February 25, 2021 23:01
@Be-ing
Copy link
Contributor Author

Be-ing commented Feb 25, 2021

Well this is strange. Commenting out the /fp:fast compiler flag gets rid of the parse_arguments: CannotCache(multiple input files) error from the sccache server, but now it fails with

..\lib\googletest-1.10.x\googletest\src\gtest-all.cc: fatal error C1041: cannot open program database 'D:\a\mixxx\mixxx\build\bin\gmock_main.pdb'; if multiple CL.EXE write to the same .PDB file, please use /FS
cl : Command line warning D9025 : overriding '/Z7' with '/Zi'

But /FS is used and there is no /Zi used?

@Be-ing
Copy link
Contributor Author

Be-ing commented Feb 26, 2021

Google Test sets the Zi compiler option which is incompatible with Z7 and breaks the build. I submitted a pull request upstream to fix that.

@Be-ing Be-ing force-pushed the sccache branch 2 times, most recently from 0e7a9f1 to a0d34ab Compare February 26, 2021 01:15
@Holzhaus
Copy link
Member

Maybe it's time to switch to doctest.

@Be-ing
Copy link
Contributor Author

Be-ing commented Feb 26, 2021

Requiring a tiny change to Google Test is IMO not a good reason to spend the effort switching away from it. But I am pretty wary of relying on Goolge-maintained software...

@Be-ing Be-ing force-pushed the sccache branch 4 times, most recently from a781ada to e992159 Compare February 26, 2021 03:01
@Be-ing Be-ing marked this pull request as ready for review February 26, 2021 08:02
@Be-ing
Copy link
Contributor Author

Be-ing commented Feb 26, 2021

That brings the total Windows job time from 35 minutes to 19 minutes.

Using Ninja from the GH Actions VM image instead of downloading it from Chocolatey reduced the job time another 1.5-2 minutes, so now Windows jobs take 16-17 minutes. 🚀 Now, if GitHub Actions would get zstd working with caching on Windows...

@daschuer
Copy link
Member

What was the reason for switching from clcache to cscache? Maybe clcache did not work for another reason that is fixed now as well.
I am asking because of relying on a patched version of sccache is not optimal.

Is cmake able to verify if the sccache is patched?

Is the PDB debugging still possible? It was originsl broken with /Zi, but I have read somwher that Microsoft has fixed it.

Please verify if this fix already works for us.

@Be-ing
Copy link
Contributor Author

Be-ing commented Feb 26, 2021

clcache is abandoned. Maybe it would work with the changes in this branch, but relying on unmaintained software is a bad idea. As far as I can tell, sccache is the only currently maintained compiler cache for MSVC.

I double checked and the mixxx.pdb file is still generated and installed with /Z7.

@Be-ing
Copy link
Contributor Author

Be-ing commented Feb 26, 2021

Is cmake able to verify if the sccache is patched?

No, but the build still works with the unpatched sccache. It just doesn't cache.

@daschuer
Copy link
Member

daschuer commented Feb 26, 2021

If clcache just works for us, there is no need for active development. I would prefer to use this, because it will introduce less hassle to a new contributor, compared to use a patched version of cscache. It is even more bad if users believe to use it, but waiting ages anyway because of the bug.

@Be-ing
Copy link
Contributor Author

Be-ing commented Feb 26, 2021

Installing the patched sccache is no harder than clcache. Once the user has Rust installed, it is just:

cargo install --git https://github.com/Be-ing/sccache.git --branch fix_msvc_fp

and cargo does the rest. On GitHub Actions, building sccache takes 10 minutes the first time, then it is cached. This is practically the same as clcache, which also requires installing a language toolchain (Python) and running python -m pip install git+https://github.com/frerich/clcache.git

If clcache just works for us

But it doesn't, we don't know why, and I'm not wasting my time figuring out why unmaintained software doesn't work. I have already spent multiple days getting sccache working.

@Be-ing
Copy link
Contributor Author

Be-ing commented Feb 26, 2021

All that will change when my PR for sccache is merged is the cargo command:

cargo install sccache

@Be-ing
Copy link
Contributor Author

Be-ing commented Feb 26, 2021

I am opposed to adding dependencies on unmaintained tools when there is a currently maintained alternative which is used by large projects (Firefox and Rustc).

@Be-ing
Copy link
Contributor Author

Be-ing commented Feb 26, 2021

Merge?

Copy link
Contributor

@uklotzde uklotzde left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for pushing this even further! LGTM

@uklotzde uklotzde merged commit 42c89a5 into mixxxdj:2.3 Feb 26, 2021
@daschuer
Copy link
Member

We have now a 9m 29s penalty for build sccache from source.
I think we should fetch a pre build binary once the patch is merged.

@Holzhaus
Copy link
Member

We have now a 9m 29s penalty for build sccache from source.
I think we should fetch a pre build binary once the patch is merged.

https://github.com/actions/cache/blob/master/examples.md#rust---cargo

@Be-ing Be-ing deleted the sccache branch February 27, 2021 01:59
@Be-ing
Copy link
Contributor Author

Be-ing commented Feb 27, 2021

The sccache build is already cached. GH Actions separates caches by branch so it may need to be rebuilt for the initial merge. But now it should take < 1 minute to restore from the cache.

Using the hash of the Cargo.lock file would be a good idea. Currently the key is just the commit hash of sccache Git repository. Of course that won't scale when we use cargo for more binaries such as Aoide.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants