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] Use SIRIUS namespace convention for options and other SIRIUS … #832

Merged
merged 1 commit into from
Jul 25, 2023

Conversation

mtaillefumier
Copy link
Collaborator

@mtaillefumier mtaillefumier commented Apr 25, 2023

  • Namespace cmake options.
  • Namespace the SIRIUS compilation flags
  • The spack recipe is modified to reflect the cmake changes.
  • Use the local spack recipe for sirius instead of the official version (We need to update it eventually)
  • updated the docker containers for the CI.
  • Update the docker containers for app distribution (we should do nothing here)
  • Update the spack recipe in the spack repo. Need a PR for this.

@simonpintarelli
Copy link
Collaborator

Do we need the namespaces in the options? Isn't this only an advantage if the project is included as subdirectory?
I'd rather have the fortran api with improved linking, e.g. almost everything internal can be made private, which means we could almost delete siriusConfig.cmake.

@mtaillefumier
Copy link
Collaborator Author

Yes it is actually good practice to have compilation options namespaced. It is done almost everywhere. It is even more important when we install findPackage.cmake modules because variable names is problematic in general. USE_CUDA for instance can be used in project A that depends on SIRIUS (that also has this variable in siriusConfig.cmake) but sirius is compiled without cuda support.

Removing the siriusConfig.cmake is out of the table because findPackage(sirius) rely on it to actually find the dependencies needed during linking time (that's also why we need to copy the findPACKAGE.cmake we provide). It would be like using pkgconfig for detecting sirius without actually creating the pc file associated to it.

the fortran stuff is decoupled from this.

@simonpintarelli
Copy link
Collaborator

simonpintarelli commented Apr 25, 2023

I agree it seems the prefixed option names is common. But what is the benefit for SIRIUS? As far as I can see the options USE_XXX etc don't show up in the installation directory lib/cmake. In siriusConfig.cmake the options are substituted with the value before installation, therefore it's unclear to me what is the benefit of the namespace.

For the Fortran API I don't mean to retire cmake config, just correctly declaring the dependencies, at the moment anything is public.

@mtaillefumier
Copy link
Collaborator Author

USE_OPTION can be used by other projects as well which is also common. USE_CUDA for instance is a typical example of this issue and it can have disastrous consequences as we encountered in cosma / costa for instance. I had similar issues when working on cp2k cmake build system. It is unfortunate that cmake does not automatically do this so we have to compensate for this design flaw.

@simonpintarelli
Copy link
Collaborator

simonpintarelli commented Apr 25, 2023

USE_OPTION can be used by other projects as well which is also common. USE_CUDA for instance is a typical example of this issue and it can have disastrous consequences as we encountered in cosma / costa for instance. I had similar issues when working on cp2k cmake build system. It is unfortunate that cmake does not automatically do this so we have to compensate for this design flaw.

That's my question, cosma/costa uses in-tree build, but for SIRIUS this is not the case. Thus I don't see the problem of calling the option USE_CUDA. E.g. my assumption is that in-tree build of SIRIUS is a bad idea and not used anywhere.

@mtaillefumier
Copy link
Collaborator Author

it is a bad idea indeed to build sirius in tree (that's why submodule is also a bad idea). The problem is identical to the DFLAGS in header files provided by different libraries. -DUSE_MKL might mean something different for dependency A and B and might trigger branches in their respective header files that might only be supported if the dependencies A and/or B are compiled with the relevant flags. That's why for instance compilation dependent flags should avoid __NAME because __NAME is usually reserved to compilers.
Namespacing

@mtaillefumier
Copy link
Collaborator Author

I agree it seems the prefixed option names is common. But what is the benefit for SIRIUS? As far as I can see the options USE_XXX etc don't show up in the installation directory lib/cmake. In siriusConfig.cmake the options are substituted with the value before installation, therefore it's unclear to me what is the benefit of the namespace.

well we do things properly but other do not. ;)

For the Fortran API I don't mean to retire cmake config, just correctly declaring the dependencies, at the moment anything is public.

I misunderstood your intend here for the fortran api. I though you were referring to the problem you mentioned yesterday.

Indeed everything is public in cmake and I do not know how to make things private by default. I am not even sure it can be done actually.

@mtaillefumier mtaillefumier force-pushed the cmake_improvements branch 3 times, most recently from fe9667d to 75b8dfa Compare June 8, 2023 10:45
@mtaillefumier mtaillefumier force-pushed the cmake_improvements branch 4 times, most recently from 18f30d9 to b94e6e9 Compare June 23, 2023 08:12
@mtaillefumier mtaillefumier force-pushed the cmake_improvements branch 6 times, most recently from 55a8c9a to 59ea439 Compare July 4, 2023 09:05
@mtaillefumier mtaillefumier force-pushed the cmake_improvements branch 7 times, most recently from 6e0d115 to 627718c Compare July 17, 2023 12:52
@mtaillefumier mtaillefumier force-pushed the cmake_improvements branch 3 times, most recently from 68ca5c4 to 1e18c37 Compare July 24, 2023 08:13
@simonpintarelli simonpintarelli self-requested a review July 24, 2023 14:40
@simonpintarelli simonpintarelli marked this pull request as ready for review July 24, 2023 14:41
Copy link
Collaborator

@simonpintarelli simonpintarelli left a comment

Choose a reason for hiding this comment

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

lgtm

$<$<BOOL:${SIRIUS_USE_MAGMA}>:SIRIUS_MAGMA>
$<$<BOOL:${SIRIUS_USE_ROCM}>:SIRIUS_GPU SIRIUS_ROCM>
$<$<BOOL:${SIRIUS_USE_VDWXC}>:SIRIUS_USE_VDWXC>
$<$<STREQUAL:${SIRIUS_USE_FP32},"ON">:SIRIUS_USE_FP32>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Before it defined USE_FP32_BOOL (after checking for single precision support in spfft) in main CMakeLists.txt. I would rather keep the two variables, e.g. SIRIUS_USE_FP32_BOOL and SIRIUS_USE_FP32, instead of using STREQUAL on something which should be a boolean.

…internal variables

- updated README.md file to reflect the new convention
- updated the spack recipe
- removed multiple versions of the spack recipe
- updated the docker files when needed
- remove unused modules in `cmake/modules`
@simonpintarelli simonpintarelli merged commit 2c72e1f into develop Jul 25, 2023
3 of 4 checks passed
@simonpintarelli simonpintarelli deleted the cmake_improvements branch July 25, 2023 09:36
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

3 participants