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

CMake: Add support for UNITY_BUILD #2839

Merged
merged 5 commits into from
Jan 22, 2024
Merged

CMake: Add support for UNITY_BUILD #2839

merged 5 commits into from
Jan 22, 2024

Conversation

jschueller
Copy link
Contributor

Allows to speed-up compilation, or at least avoid erros in vtk where netcdf is used as a third-party

@WardF
Copy link
Member

WardF commented Jan 12, 2024

Can you explain what this option is doing, and/or what the errors being encountered & fixed by this are?

@WardF WardF self-assigned this Jan 12, 2024
@WardF WardF added this to the 4.9.3 milestone Jan 12, 2024
@jschueller
Copy link
Contributor Author

yes, this option allows to merge several source files into a single compilation unit to speed up builds
but when doing that some declarations from different files may conflict, so a workaround is to exclude these files from unity builds
the big picture is avoiding these errors in vtk unity builds, as vtk that uses netcdf as a third-party
see also
https://cmake.org/cmake/help/latest/prop_tgt/UNITY_BUILD.html

@WardF
Copy link
Member

WardF commented Jan 19, 2024

Thanks for providing the inference, I was unfamiliar with UNITY_BUILD. Out of curiosity, how is netCDF being built in a unity build; if I'm reading the reference correctly, this is something that would be enabled via setting target properties in the various CMakeLists.txt files1, rather than something that would be specified on the command line2.

Am I misreading this? Looking at the warning, it says that developers should not, but as well all know, that doesn't mean developers aren't XD. But unless that's what's happening, I wonder if these changes would make more sense wherever the UNITY_BUILD_MODE is being defined in the underlying infrastructure. RIght now, these changes exist in a vacuum, and, absent any context, aren't immediately clear what they're doing or why they're in these files.

A fix might be to modify our project to use UNITY_BUILD natively (thoughts, @K20shores)? That might be a bit down the road, however.

Footnotes

  1. https://cmake.org/cmake/help/latest/prop_tgt/UNITY_BUILD_MODE.html#prop_tgt:UNITY_BUILD_MODE

  2. https://cmake.org/cmake/help/latest/prop_tgt/UNITY_BUILD.html

@jschueller
Copy link
Contributor Author

jschueller commented Jan 19, 2024

it can be enabled at configure time with cmake -DCMAKE_UNITY_BUILD=ON which enables UNITY_BUILD for all targets (thats why it was enabled in vtk),

I dont advise to enable it by default because it can be slower for incremental build needed when developing,
its faster for one-time builds (interesting on CI etc)

@WardF
Copy link
Member

WardF commented Jan 19, 2024

Ah, I see, the CMAKE_UNITY_BUILD is infact intended for command-line use. Thanks!

I'll take a closer look at this on Monday, we should be able to get it in but I'll want to provide some comments or accompanying information so that these changes don't exist in a contextless vacuum. There's very little chance I'll remember any of these 18 months from now XD.

WardF
WardF previously approved these changes Jan 22, 2024
@WardF
Copy link
Member

WardF commented Jan 22, 2024

I added comments to future-proof against my own future confusion, and updated the release notes. Once the tests pass, I'll get this merged in. Thanks @jschueller!

@WardF WardF merged commit 5b79304 into Unidata:main Jan 22, 2024
99 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants