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

Add support for windows caching #306

Closed
wants to merge 7 commits into from

Conversation

mjcarroll
Copy link
Contributor

@mjcarroll mjcarroll commented Aug 18, 2022

🦟 Bug fix

Summary

Using ccache or buildcache on Windows is not supported when using the /Zi flag (ref: ccache/ccache#1040)

The recommended guidance at the moment is to drop to /Z7 to allow for shared caches to work correctly.

Enabled via: USE_GZ_RECOMMENDED_MSVC_CACHING and off by default.

Checklist

  • Signed all commits for DCO
  • Added tests
  • Updated documentation (as needed)
  • Updated migration guide (as needed)
  • Consider updating Python bindings (if the library has them)
  • codecheck passed (See contributing)
  • All tests passed (See test coverage)
  • While waiting for a review on your PR, please help review another open pull request to support the maintainers

Note to maintainers: Remember to use Squash-Merge and edit the commit message to match the pull request summary while retaining Signed-off-by messages.

Signed-off-by: Michael Carroll <[email protected]>
@github-actions github-actions bot added the 🌱 garden Ignition Garden label Aug 18, 2022
@mjcarroll mjcarroll marked this pull request as draft August 18, 2022 14:08
@mjcarroll mjcarroll self-assigned this Aug 18, 2022
Signed-off-by: Michael Carroll <[email protected]>
@chapulina chapulina added enhancement New feature or request Windows Windows support labels Aug 26, 2022
@mjcarroll mjcarroll marked this pull request as ready for review April 20, 2023 16:17
@@ -342,6 +346,29 @@ macro(_gz_setup_msvc)
# TODO: What flags should be set for PROFILE and COVERAGE build types?
# Is it even possible to generate those build types on Windows?

# Replace any higher warnings with our selected level
Copy link
Contributor

Choose a reason for hiding this comment

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

I would prefer not to mess with the CMAKE_{C/CXX}_ variables if possible (or not to do it more than we are currently doing). Would it work something like add_compile_options(${MSVC_WARNING_LEVEL})?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, in general, I agree. The issue here is that MSVC is really dumb about how it parses command line options, and if you have multiple warning switches, they can interact poorly. Simply appending a new warning level to the end causes warnings to show up on the command line (iirc)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The warning level bit is actually separate from the caching support here. It's just to suppress that warning with MSVC

Copy link
Contributor

Choose a reason for hiding this comment

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

The warning level bit is actually separate from the caching support here. It's just to suppress that warning with MSVC

The comment was looking to avoid messing with CMAKE_{C/CXX}_ for the warnings and for the optimization in the lines after. If you have already tested and suffer the multiple compile line options, I'm fine just by adding a comment before the CMAKE_{C/CXX}_ lines explaining why we are not usingadd_compile_options.

@j-rivero
Copy link
Contributor

j-rivero commented Jun 8, 2023

@osrf-jenkins run tests please

@azeey azeey added the beta Targeting beta release of upcoming collection label Jul 31, 2023
@mjcarroll mjcarroll closed this Aug 4, 2023
@mjcarroll mjcarroll deleted the mjcarroll/enable_windows_caching branch August 4, 2023 14:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
beta Targeting beta release of upcoming collection enhancement New feature or request 🌱 garden Ignition Garden Windows Windows support
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants