Skip to content

Various cmake cleanups#279

Merged
LecrisUT merged 4 commits intospglib:developfrom
LecrisUT:cmake/clean-up
Jun 8, 2023
Merged

Various cmake cleanups#279
LecrisUT merged 4 commits intospglib:developfrom
LecrisUT:cmake/clean-up

Conversation

@LecrisUT
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown

codecov-commenter commented Jun 6, 2023

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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 <[email protected]>
@LecrisUT
Remove omp target
0e00e4f 
Signed-off-by: Cristian Le <[email protected]>
@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 <[email protected]>
@LecrisUT
Only sanitize spglib C library
0d567ac 
Signed-off-by: Cristian Le <[email protected]>
@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