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

Fix sccache usage in Windows CI builds #897

Merged
merged 1 commit into from
Feb 9, 2024

Conversation

jherico
Copy link
Contributor

@jherico jherico commented Feb 7, 2024

Description

This corrects a couple problems preventing the Windows CI builds from using sccache.

Fix for #896 and reduces windows build significantly.

@jherico jherico marked this pull request as draft February 7, 2024 04:52
@jherico jherico changed the title Fix MSVC flag setting in CMake Fix sccache usage in Windows CI builds Feb 7, 2024
@jherico jherico marked this pull request as ready for review February 7, 2024 05:39
@jherico
Copy link
Contributor Author

jherico commented Feb 7, 2024

@gpx1000 You mentioned having trouble getting sccache working in Windows CI builds. I was able to track it down after I noticed something odd in the beginning of the CMake config.

The basic issue was twofold...

  • Although sccache doesn't work with the Visual Studio generator, some sources online seem to suggest it works with the "NMake Makefiles" generator, but as far as I can tell it does not. Switching to Ninja works though.
  • The online commentary about the need to use /Z7 instead of /Zi may be accurate, but the mechanism that was being used didn't work for the reasons I noted in CMake configuration order of operations is incorrect #896

tomadamatkinson
tomadamatkinson previously approved these changes Feb 7, 2024
Copy link
Collaborator

@tomadamatkinson tomadamatkinson left a comment

Choose a reason for hiding this comment

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

Nice! Looks like this cuts the Windows build down from ~30minutes to ~7minutes in the CI

@jherico
Copy link
Contributor Author

jherico commented Feb 7, 2024

It occurred to me that I should revert the change adding messaging about lack of ccache or sccache since they don't work with the most commonly used Windows generators. I could gate them behind a if (CMAKE_GENERATOR MATCHES "Visual Studio"), but I figured it's best to just leave it closer to as is.

@gpx1000
Copy link
Collaborator

gpx1000 commented Feb 7, 2024

I always kept it in Ninja build for Windows as I wanted to ensure the windows build system was using at least NMake. SCCache was supposed to work with NMake but I never got that working right. I think it's far better to just bite the bullet and use Ninja as you have here. I don't think at first blush that the macos build failure in CI is due to the changes here. lemme know if you need help changing it.
Thanks for the contribution; greatly appreciated! And great catches of bugs.

Copy link
Collaborator

@SaschaWillems SaschaWillems left a comment

Choose a reason for hiding this comment

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

Thank you very much for this PR. Works fine for me on Windows 11 👍🏻

Copy link
Collaborator

@gpx1000 gpx1000 left a comment

Choose a reason for hiding this comment

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

lgtm; thanks for the contribution!

@tomadamatkinson tomadamatkinson merged commit b03b5c4 into KhronosGroup:main Feb 9, 2024
24 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants