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

Exclude sources from unity builds #191

Merged
merged 2 commits into from
Sep 28, 2022

Conversation

steinmig
Copy link
Contributor

The library has some multiple variable names and macro names, leading to One Definition Rule problems with unity builds.

This cmake update excludes all sources from unity builds, letting them be compiled "normally"

@atztogo
Copy link
Collaborator

atztogo commented Sep 27, 2022

SKIP_UNITY_BUILD_INCLUSION is supported by cmake 3.16 or later. I'm not sure which cmake versiont we should require to users. @steinmig, is it possible to skill this section if cmake version is not support it?

@steinmig
Copy link
Contributor Author

Sorry, I just now noticed that you actually support all the way to 2.8.

I don't have a CMake 2 laying around, but I think it should support the current solution

@codecov-commenter
Copy link

Codecov Report

Base: 85.12% // Head: 85.12% // No change to project coverage 👍

Coverage data is based on head (dda25d3) compared to base (de222b7).
Patch has no changes to coverable lines.

Additional details and impacted files
@@           Coverage Diff            @@
##           develop     #191   +/-   ##
========================================
  Coverage    85.12%   85.12%           
========================================
  Files           16       16           
  Lines         1318     1318           
========================================
  Hits          1122     1122           
  Misses         196      196           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@atztogo atztogo merged commit 9458171 into spglib:develop Sep 28, 2022
@atztogo
Copy link
Collaborator

atztogo commented Sep 28, 2022

Merged. @steinmig, thank you very much!

@LecrisUT
Copy link
Collaborator

LecrisUT commented Mar 1, 2023

@steinmig Would you be so kind to double-check if the recent v2.1.0-rc1 still needs this?

@steinmig
Copy link
Contributor Author

steinmig commented Mar 2, 2023

@LecrisUT Yes this is still needed, since there are still variables / functions with identical names.

@LecrisUT
Copy link
Collaborator

LecrisUT commented Mar 2, 2023

Could you walk me through on how to generate an error without this set? As you can see this is currently disabled, and we might not have the appropriate tests to detect an error from this.

@steinmig
Copy link
Contributor Author

steinmig commented Mar 2, 2023

Execute cmake with the option for a unity build like:

cmake -DCMAKE_UNITY_BUILD=true ..

Basically this fix simply disables this option for all the files of this project, which can be useful if this is a dependency for a project that does use this option.
The benefit of this option is a shorter compile time, which we exploit for CI purposes.

(the option is usually no problem for C++ due to namespaces, but of course this is not a solution here. There are actually only a few duplicates, but I did not want to change the names, because one is function and I suspect this would be API breaking, although I didn't have a closer look at that)

LecrisUT added a commit to LecrisUT/spglib that referenced this pull request Mar 3, 2023
Signed-off-by: Cristian Le <cristian.le@mpsd.mpg.de>
LecrisUT added a commit to LecrisUT/spglib that referenced this pull request Apr 5, 2023
Signed-off-by: Cristian Le <cristian.le@mpsd.mpg.de>
LecrisUT added a commit that referenced this pull request Apr 7, 2023
Signed-off-by: Cristian Le <cristian.le@mpsd.mpg.de>
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.

None yet

4 participants