Update CMake standard to reflect FIAT and OOPS#30
Update CMake standard to reflect FIAT and OOPS#30msleigh merged 2 commits intoadd-cmake-standardfrom
Conversation
cmake/guidelines.rst
Outdated
| Some repositories (e.g. **FIAT**, **OOPS**) contain both Fortran and C or C++ sources. | ||
| For such projects: | ||
|
|
||
| - Declare all used languages in ``project()``. |
There was a problem hiding this comment.
This is already covered under the "Project Declaration" section - in contrast the phrasing here almost seems to (wrongly) imply that repositories using only one language don't need to declare it. Or is it just me that gets that impression? Maybe a rewording would help? E.g. "Ensure all languages used are declared in project()"
cmake/guidelines.rst
Outdated
|
|
||
| Dual Build Mode Support | ||
| ----------------------- | ||
| Projects should support both embedded and standalone builds:: |
There was a problem hiding this comment.
What do we mean here by "embedded" builds? The example seems to suggest that we should support IFS builds via add_subdirectory, which doesn't apply to components already moved to the bundle.
Perhaps we could amend the above to: "Sub-components, i.e. projects built via add_subdirectory invocations rather than via a separate bundle entry, should support both embedded and standalone builds. Fully standalone projects, e.g. ecWAM, should not support embedded builds."
cmake/guidelines.rst
Outdated
|
|
||
| - Declare all used languages in ``project()``. | ||
| - Apply language-specific flags via ``ecbuild_add_c_flags`` and ``ecbuild_add_fortran_flags``. | ||
| - Avoid setting shared flags (``CMAKE_C_FLAGS`` etc.) globally; handle each language separately. |
There was a problem hiding this comment.
This line seems a little off, CMAKE_C_FLAGS would also only set c flags so they are not global. Instead it should be:
- Avoid adding compiler flags to the native CMAKE flags lists, as these are missing some of the safety and correctness checks present in the above `ecbuild` macros
There was a problem hiding this comment.
I took the 'global' to mean the difference between e.g. CMAKE_C_FLAGS and IFS_C_FLAGS, but from the second half of the sentence you're probably right. Perhaps this should be two distinct points?
There was a problem hiding this comment.
Ah yes good point, we can give both reasons to not write to these lists:
- Are applied across the entire build (i.e. the bundle CMake superspace) and not just the current cmake project, which is risky
- We lose the configure time sanity checks that ecbuild provides
cmake/guidelines.rst
Outdated
| - Avoid setting shared flags (``CMAKE_C_FLAGS`` etc.) globally; handle each language separately. | ||
| - Keep hardening or safety-specific C flags (stack protection, warnings) in | ||
| ``cmake/compile_flags.cmake`` with clear comments explaining their purpose. | ||
| - Use ``target_compile_options()`` to apply per-target options instead of directory-wide flags. |
There was a problem hiding this comment.
... to add compiler flags only to a specific target; these get appended to the project-wide flags already in place.
cmake/guidelines.rst
Outdated
|
|
||
| - Be isolated in a dedicated module (``cmake/compile_flags.cmake``) | ||
| - Be documented with a comment stating their motivation (e.g. memory safety, API conformance) | ||
| - Not be applied unconditionally across all targets |
There was a problem hiding this comment.
The compile_flags macros in all of our projects other than ifs-source are invoked at a cmake scope above those that declare targets. They typically contain calls to ecbuild_add_<lang>_flags and so their contents will be applied across the repository to all targets.
There's no straightforward to remedy that (the same cmake design flaw also makes setting source-file specific flags a lot harder), so we should simply remove this last statement.
There was a problem hiding this comment.
Again, I'm looking at this from a very IFS-centric perspective, but in the context of extra-stringent flags I took "unconditionally" to mean "not protected by e.g. if (IFS_CHECK_BOUNDS) (the dangers of AI-written text...). I agree with your point but perhaps this should be clarified if it's not mentioned elsewhere.
There was a problem hiding this comment.
Hmm yes. In that case this could be something like "the application of these safety related compiler flags should be togglable via an option or configure-time cmake flag".
cmake/guidelines.rst
Outdated
| Standalone components (e.g. **FIAT**, **ecTrans**, **OOPS**) must export | ||
| CMake package configuration files for downstream discovery:: | ||
|
|
||
| ecbuild_pkgconfig( |
There was a problem hiding this comment.
As I understand it, this is only needed to package cmake projects for export to other non-cmake projects. It was needed during our migration to cmake, but now we don't need it, is never used or tested and is very likely as a result broken.
I would say we should explicitly deprecate the ecbuild_pkgconfig functionality in the ifs components, eccodes and such might still use it.
There was a problem hiding this comment.
Some ifs sub-projects call it, perhaps it's worth checking with the owners if we can remove these?
cmake/guidelines.rst
Outdated
| - Limit compiler-specific conditionals to dedicated modules | ||
| - Provide consistent packaging and installation metadata across all components | ||
| - Maintain strict compatibility with ecbuild and CMake ≥ 3.20 | ||
| - Define common helper macros in a shared ECMWF CMake toolkit |
There was a problem hiding this comment.
Well, now that you've brought it up, do we need to emphasise the role of ecbuild more strongly somewhere in the doc? At the moment the most weighted statement on the subject (as opposed to guidance for a specific function) is "Follow ecbuild conventions where available", which is a bit wishy washy.
Assuming that's done, then perhaps this line should be something like "Identify commonalities where new ecbuild macros may be shared across projects."
prgillies
left a comment
prgillies
left a comment
There was a problem hiding this comment.
Added some minor comments. I couldn't comment on lines without changes so adding them here:
Naive question about 'buildable directories' line 62/64, eg arpifs doesn't have a CMakeLists.txt and the source is added in cmake/arpifs.cmake. Do the guidelines here cover that method.
How does the version specified with ecbuild_find_package (line 220/263 for eccodes) interact with the version in the bundle.yml? Presumably the bundle.yml version must be greater or equal to the version specified here?
4 participants
After comment from Olivier I extended to cover OOPS and FIAT CMakeLists.txt. This is for those changes.
(In further comments Olivier suggested we remove the part about over-riding compiler flags altogether, as (a) not well done here, but also (b) we don't know what the standard should be anyway. But I will do this afterwards, as a separate PR.)