Cmake refactoring #233

LecrisUT · 2023-02-14T15:40:00Z

Bare with me on this one, it is a beast of a refactoring:

51e15ad: Renamed cmake options to be more consistent with good practices suggested by Craig Scott (prominent cmake maintainer). This is from a discussion on libxc.
e19b59c, 2538feb: Similar to the previous one, some good practices
8d41b31: Random improvement if you want to run cmake --install --component Spglib_Development to install only specific components. Might be useful downstream if they want to install only specific files from this
b4128d3: Add an alias library libsymspg-omp that is guaranteed to be built with OpenMP. The main library is still built with OpenMP, but subsequent runs might overwrite it. This makes more sense with the next changes
4b520c1, f9a1548: Implement components. More bellow
3c71588: Disabled install completely when project is imported via ExternalProject

We can use components like:

find_package(Spglib COMPONENTS omp static
    OPTIONAL_COMPONENTS fortran)

Which will test that spglib is built with omp and as a static library, but also include fortran if available. If not passed any components, it will try to load all components. Normally components should only be optional additional libraries, but here we have 2 special cases:

shared and static: These components will switch what kind of libraries to include in the project, i.e. to link as a shared or static library. These are optional and if not passed, it will try to figure out from SPGLIB_SHARED_LIBS (required to be as it is defined) or BUILD_SHARED_LIBS if defined (allows to fallback to what's available)
omp: This includes the libsyspg-omp library. This is effectively a copy of libsymspg that is guaranteed to be built via OpenMP. Normally we should not be including the build options as components like that, but often times you can see exceptions for MPI and OpenMP types (e.g. in package distributions).

I have disabled completely install steps when spglib is imported via ExternalProject. This could otherwise lead to overwriting the system installed version. I couldn't find a clean way to allow installation safely, so I've left a note to contact us about how it should be done.

Luckily we have not yet pushed a 2.1 release with the options changes, so there is little affecting downstream.

lan496 · 2023-02-19T04:55:32Z

@LecrisUT Thanks for the refactoring and improvements of documents!
Just curious, how the target Spglib_symspg defined in CMakeLists.txt are resolved as Spglib::symspg as shown in example/CMakeLists.txt (not Spglib::Spglib_symspg)?
Is it a CMake functionality (if so, where is it officially documented), or resolved in spglib's side?

LecrisUT · 2023-02-19T06:54:31Z

This I got from the discussion in Libxc. Specifically it is controlled by EXPORT_NAME here. The complete documentation of all of the cmake (target) properties is here (man cmake-properties)

Standard according to "Professional CMake: A Practical Guide" Signed-off-by: Cristian Le <cristian.le@mpsd.mpg.de>

The `find_package` was not previously implemented so it is still possible to rename Signed-off-by: Cristian Le <cristian.le@mpsd.mpg.de>

Signed-off-by: Cristian Le <cristian.le@mpsd.mpg.de>

Cmake object magic! Signed-off-by: Cristian Le <cristian.le@mpsd.mpg.de>

Signed-off-by: Cristian Le <cristian.le@mpsd.mpg.de>

This is helpful if the project is included via `ExternalProject` and you use something like `ccmake`. In that case all options will be displayed, and the helper messages can become ambiguous (despite the clear prefixing) Signed-off-by: Cristian Le <cristian.le@mpsd.mpg.de>

…hContent Couldn't find a safe way to ensure install will not override pre-installed spglib installation. This needs to be discussed with specific implementation examples. Signed-off-by: Cristian Le <cristian.le@mpsd.mpg.de>

Signed-off-by: Cristian Le <cristian.le@mpsd.mpg.de>

- Fixed fortran install - Moved `CMAKE_POSITION_INDEPENDENT_CODE` to the relevant object Signed-off-by: Cristian Le <cristian.le@mpsd.mpg.de>

Signed-off-by: Cristian Le <cristian.le@mpsd.mpg.de>

codecov-commenter · 2023-02-20T14:14:50Z

Codecov Report

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

Coverage data is based on head (61b4f32) compared to base (2f8b635).
Patch has no changes to coverable lines.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

Additional details and impacted files

@@           Coverage Diff            @@
##           develop     #233   +/-   ##
========================================
  Coverage    93.34%   93.34%           
========================================
  Files           15       15           
  Lines          902      902           
========================================
  Hits           842      842           
  Misses          60       60

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.

LecrisUT · 2023-02-20T14:17:48Z

Rebased it onto develop and cleaned up the documentation, let me know if you find it cleaner now.

A few things though:

@atztogo Can you make the example_c and/or example_f08 more minimal so that it has more realistic example? Also have a very simple example in the main README of what spglib can do.
@atztogo I have removed your tetrahedron.c example because it couldn't be compiled without tetrahedron.h, is it ok, or can you otherwise add all the necessary files here?
Is the mailing list in sourceforge (https://sourceforge.net/p/spglib/mailman/) still active? Should we get rid of it and use Github and maybe gitter/matrix/irc for more live channels?

atztogo · 2023-02-21T03:39:56Z

@LecrisUT, @lan496, I have a minor question. I just recently know cmake option of -B. Currently the spglib documentation it is written like below.

$ mkdir build
$ cd build
$ cmake .. ${CMAKE_OPTIONS}
$ cmake --build .
$ cmake --install .

But it can be also

$ cmake -B build ${CMAKE_OPTIONS} .
$ cmake --build build
$ cmake --install build

Which do you prefer?

atztogo · 2023-02-21T03:42:02Z

@atztogo Can you make the example_c and/or example_f08 more minimal so that it has more realistic example? Also have a very simple example in the main README of what spglib can do.

I agree. It was also used as tests. We have (C) and are preparing (Fortran) tests, so I agree. Can it be written in an issue?

atztogo · 2023-02-21T03:46:38Z

@atztogo I have removed your tetrahedron.c example because it couldn't be compiled without tetrahedron.h, is it ok, or can you otherwise add all the necessary files here?

OK! Thanks.

atztogo · 2023-02-21T03:49:14Z

Is the mailing list in sourceforge (https://sourceforge.net/p/spglib/mailman/) still active? Should we get rid of it and use Github and maybe gitter/matrix/irc for more live channels?

Sourceforge mailing list is not at all very active. We can use github instead. I have no idea about gitter/matrix/irc. Only github is not enough?

atztogo

Great!

lan496 · 2023-02-21T04:24:35Z

Which do you prefer?

@atztogo I like the latter because it needs fewer commands, although a bit newer CMake >= 3.13 is required.

LecrisUT · 2023-02-21T06:01:16Z

@LecrisUT, @lan496, I have a minor question. I just recently know cmake option of -B. Currently the spglib documentation it is written like below.

Which do you prefer?

Oh yeah, I completely forget about -B. I'm also ok with rewriting it to that.

I have no idea about gitter/matrix/irc. Only github is not enough?

Sure, only github is fine. The others are like slack, but open sourced. All those 3 are interoperable. I'm always reachable on matrix if someone wants to discuss something there.

Signed-off-by: Cristian Le <cristian.le@mpsd.mpg.de>

LecrisUT requested review from atztogo and lan496 February 14, 2023 15:40

LecrisUT mentioned this pull request Feb 17, 2023

Build failure in fortran_test.c using Intel compiler ifort/ifx #234

Closed

lan496 approved these changes Feb 19, 2023

View reviewed changes

LecrisUT added 13 commits February 20, 2023 13:56

Rename prefixed cmake options

93d1c93

Standard according to "Professional CMake: A Practical Guide" Signed-off-by: Cristian Le <cristian.le@mpsd.mpg.de>

Rename project to uppercased name

faab106

The `find_package` was not previously implemented so it is still possible to rename Signed-off-by: Cristian Le <cristian.le@mpsd.mpg.de>

Namespace targets

eb23a8e

Signed-off-by: Cristian Le <cristian.le@mpsd.mpg.de>

Add install components

c6b938b

Signed-off-by: Cristian Le <cristian.le@mpsd.mpg.de>

Add component package helper

634fc44

Signed-off-by: Cristian Le <cristian.le@mpsd.mpg.de>

Add an alias library symspg-omp that is always built with OpenMP

393af3b

Cmake object magic! Signed-off-by: Cristian Le <cristian.le@mpsd.mpg.de>

Implement components

4526506

Signed-off-by: Cristian Le <cristian.le@mpsd.mpg.de>

Fix bundled example

b7e68e3

Signed-off-by: Cristian Le <cristian.le@mpsd.mpg.de>

Update documentation to include components

9177fe0

Signed-off-by: Cristian Le <cristian.le@mpsd.mpg.de>

Build fixes

d059ac2

- Fixed fortran install - Moved `CMAKE_POSITION_INDEPENDENT_CODE` to the relevant object Signed-off-by: Cristian Le <cristian.le@mpsd.mpg.de>

Clean up the documentation

61b4f32

Signed-off-by: Cristian Le <cristian.le@mpsd.mpg.de>

LecrisUT force-pushed the cmake/standards branch from 7add637 to 61b4f32 Compare February 20, 2023 14:09

atztogo approved these changes Feb 21, 2023

View reviewed changes

[skip ci] Documentation simplification

4041895

Signed-off-by: Cristian Le <cristian.le@mpsd.mpg.de>

LecrisUT merged commit 02159ee into spglib:develop Feb 21, 2023

LecrisUT added this to the 2.1 milestone Feb 21, 2023

LecrisUT deleted the cmake/standards branch February 24, 2023 09:48

LecrisUT added the enhancement Significant ehancements label Mar 1, 2023

LecrisUT mentioned this pull request Mar 27, 2024

ci: Fix CI builders #459

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Cmake refactoring #233

Cmake refactoring #233

Uh oh!

LecrisUT commented Feb 14, 2023 •

edited

Loading

Uh oh!

lan496 commented Feb 19, 2023

Uh oh!

LecrisUT commented Feb 19, 2023

Uh oh!

codecov-commenter commented Feb 20, 2023

Uh oh!

LecrisUT commented Feb 20, 2023

Uh oh!

atztogo commented Feb 21, 2023

Uh oh!

atztogo commented Feb 21, 2023

Uh oh!

atztogo commented Feb 21, 2023

Uh oh!

atztogo commented Feb 21, 2023

Uh oh!

atztogo left a comment

Uh oh!

lan496 commented Feb 21, 2023

Uh oh!

LecrisUT commented Feb 21, 2023

Uh oh!

Uh oh!

Cmake refactoring #233

Cmake refactoring #233

Uh oh!

Conversation

LecrisUT commented Feb 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lan496 commented Feb 19, 2023

Uh oh!

LecrisUT commented Feb 19, 2023

Uh oh!

codecov-commenter commented Feb 20, 2023

Codecov Report

Uh oh!

LecrisUT commented Feb 20, 2023

Uh oh!

atztogo commented Feb 21, 2023

Uh oh!

atztogo commented Feb 21, 2023

Uh oh!

atztogo commented Feb 21, 2023

Uh oh!

atztogo commented Feb 21, 2023

Uh oh!

atztogo left a comment

Choose a reason for hiding this comment

Uh oh!

lan496 commented Feb 21, 2023

Uh oh!

LecrisUT commented Feb 21, 2023

Uh oh!

Uh oh!

LecrisUT commented Feb 14, 2023 •

edited

Loading