Skip to content

Various cmake cleanups #279

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 4 commits into from
Jun 8, 2023
Merged

Various cmake cleanups #279

merged 4 commits into from
Jun 8, 2023

Conversation

LecrisUT
Copy link
Collaborator

@LecrisUT LecrisUT commented Jun 6, 2023

Some cmake cleanups and improvements

  • Removed openmp specific target (keep things simple for now)
  • Making top cmake file more readable
  • Make it more compatible with FetchContent (propagating Spglib_VERSION and other such variables)
  • Added SPGLIB_INSTALL. More inline with other projects standard

@LecrisUT LecrisUT force-pushed the cmake/clean-up branch 2 times, most recently from b5c59bb to 93507ad Compare June 6, 2023 09:01
@LecrisUT
Copy link
Collaborator Author

LecrisUT commented Jun 6, 2023 •
edited
Loading

Seems like address sanitizer is detecting some errors in test_magnetic_symmetry.test_spgms_get_magnetic_dataset_high_mag_symprec

Weird. Why is it only triggered here? I am able to replicate this issue locally

@LecrisUT
Copy link
Collaborator Author

LecrisUT commented Jun 6, 2023

@lan496 about 054ceca. Are sanitizers needed in the c++ part of gtests? I have disabled it because it was reported as:

HINT: this may be a false positive if your program uses some custom stack unwind mechanism, swapcontext or vfork
      (longjmp and C++ exceptions *are* supported)

@LecrisUT LecrisUT requested a review from lan496 June 6, 2023 14:24
@codecov-commenter
Copy link

Codecov Report

Patch coverage has no change and project coverage change: +3.05 🎉

Comparison is base (920b2c9) 82.86% compared to head (272cad9) 85.92%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

Additional details and impacted files 
@@             Coverage Diff             @@
##           develop     #279      +/-   ##
===========================================
+ Coverage    82.86%   85.92%   +3.05%     
===========================================
  Files           23       23              
  Lines         6081     6081              
===========================================
+ Hits          5039     5225     +186     
+ Misses        1042      856     -186
Flag Coverage Δ
c_api 74.21% <ø> (?)
fortran_api 37.38% <ø> (?)
python_api 82.86% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

see 7 files with indirect coverage changes

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

@lan496
Copy link
Member

lan496 commented Jun 7, 2023

@LecrisUT I think it is not necessary. Actually, I remember test_magnetic_symmetry.test_spgms_get_magnetic_dataset_high_mag_symprec sometimes causes such a strange warning on the address sanitizer.

lan496
lan496 approved these changes Jun 7, 2023
View reviewed changes
Copy link
Member

@lan496 lan496 left a comment

Choose a reason for hiding this comment

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

Thanks, LGTM!

@LecrisUT
Copy link
Collaborator Author

LecrisUT commented Jun 7, 2023

Lets hold off to check the CI from #285 first

LecrisUT added 4 commits June 8, 2023 10:40
@LecrisUT
Move CMakePResets
26fe787 
Signed-off-by: Cristian Le <cristian.le@mpsd.mpg.de>
@LecrisUT
Remove omp target
0e00e4f 
Signed-off-by: Cristian Le <cristian.le@mpsd.mpg.de>
@LecrisUT
Cleanup cmake configuration
5fb7894 
- Added visual separators for better reading
- Added `SPGLIB_INSTALL` for better standardization
- Moved external library definition to top-level
- Improved FetchContent compatibility
- Various cleanups

Signed-off-by: Cristian Le <cristian.le@mpsd.mpg.de>
@LecrisUT
Only sanitize spglib C library
0d567ac 
Signed-off-by: Cristian Le <cristian.le@mpsd.mpg.de>
@LecrisUT LecrisUT merged commit 219f589 into spglib:develop Jun 8, 2023
@LecrisUT LecrisUT deleted the cmake/clean-up branch June 19, 2023 13:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lan496 lan496 lan496 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@LecrisUT @codecov-commenter @lan496