Refine install_test with a find_package inside a sub-directory#964
Refine install_test with a find_package inside a sub-directory#964tpadioleau merged 4 commits intomainCExA-project/ddc:mainfrom
Conversation
|
blocked by pdidev/pdi#526 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #964 +/- ##
=======================================
Coverage 94.72% 94.72%
=======================================
Files 59 59
Lines 2863 2863
Branches 890 890
=======================================
Hits 2712 2712
Misses 93 93
Partials 58 58 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
85f38ba to
49e4b4a
Compare
3d8a1ad to
44be49b
Compare
b00bfca to
15ccca2
Compare
18e9f9d to
1d0639d
Compare
1d0639d to
14640f8
Compare
| function(assert) | ||
| if(NOT (${ARGN})) | ||
| string(REPLACE ";" " " ASSERT_TEXT "${ARGN}") | ||
| message(FATAL_ERROR "Assertion failed: ${ASSERT_TEXT}") | ||
| endif() | ||
| endfunction() |
There was a problem hiding this comment.
Can you add docstrings to this function.
An example of how it works would be useful
| ## Test targets | ||
| assert(NOT TARGET DDC::core) | ||
| assert(NOT TARGET DDC::fft) | ||
| assert(NOT TARGET DDC::pdi) | ||
| assert(NOT TARGET DDC::splines) | ||
| ## Test normal variables | ||
| assert(NOT DEFINED DDC_FOUND) | ||
| assert(NOT DEFINED DDC_BUILD_DOUBLE_PRECISION) | ||
| assert(NOT DEFINED DDC_fft_FOUND) | ||
| assert(NOT DEFINED DDC_pdi_FOUND) | ||
| assert(NOT DEFINED DDC_splines_FOUND) | ||
| ## Test cache variables | ||
| assert(NOT DEFINED CACHE{DDC_FOUND}) | ||
| assert(NOT DEFINED CACHE{DDC_BUILD_DOUBLE_PRECISION}) | ||
| assert(NOT DEFINED CACHE{DDC_fft_FOUND}) | ||
| assert(NOT DEFINED CACHE{DDC_pdi_FOUND}) | ||
| assert(NOT DEFINED CACHE{DDC_splines_FOUND}) |
There was a problem hiding this comment.
Can we refactor these checks.
Comments would be useful what you expect to be defined and not defined
# Targets that must exist
foreach(tgt IN ITEMS DDC::core)
assert(TARGET ${tgt})
endforeach()
# Targets that must NOT exist
foreach(tgt IN ITEMS DDC::fft DDC::pdi DDC::splines)
assert(NOT TARGET ${tgt})
endforeach()There was a problem hiding this comment.
Not sure, for the tests below I will need two loops for the targets and normal variable sections. I am not convinced we gain much in readability:
foreach(tgt IN ITEMS DDC::core)
assert(TARGET ${tgt})
endforeach()
foreach(tgt IN ITEMS DDC::fft DDC::pdi DDC::splines)
assert(NOT TARGET ${tgt})
endforeach()
foreach(tgt IN ITEMS DDC_FOUND DDC_BUILD_DOUBLE_PRECISION)
assert(DEFINED ${tgt})
endforeach()
foreach(tgt IN ITEMS DDC_fft_FOUND DDC_pdi_FOUND DDC_splines_FOUND)
assert(NOT DEFINED ${tgt})
endforeach()
foreach(tgt IN ITEMS DDC_FOUND DDC_BUILD_DOUBLE_PRECISION DDC_fft_FOUND DDC_pdi_FOUND DDC_splines_FOUND)
assert(NOT DEFINED CACHE{${tgt}})
endforeach()Regarding the comments, I feel assertions are really short enough to be self explanatory.
There was a problem hiding this comment.
It is more for the explanatory
In the current implementation, the similar checks are repeated, and it is not explained what you expect in each check.
## Test targets
assert(NOT TARGET DDC::core)
assert(NOT TARGET DDC::fft)
assert(NOT TARGET DDC::pdi)
assert(NOT TARGET DDC::splines)In my way, it is clear what you expect to be defined and not defined. Also, it is much easier to maintain
## Since we only find DDC, we expect that DDC::core and its related variable are defined,
## but other components should not be defined
foreach(tgt IN ITEMS DDC::core)
assert(TARGET ${tgt})
endforeach()
foreach(tgt IN ITEMS DDC::fft DDC::pdi DDC::splines)
assert(NOT TARGET ${tgt})
endforeach()
foreach(tgt IN ITEMS DDC_FOUND DDC_BUILD_DOUBLE_PRECISION)
assert(DEFINED ${tgt})
endforeach()
foreach(tgt IN ITEMS DDC_fft_FOUND DDC_pdi_FOUND DDC_splines_FOUND)
assert(NOT DEFINED ${tgt})
endforeach()
foreach(tgt IN ITEMS DDC_FOUND DDC_BUILD_DOUBLE_PRECISION DDC_fft_FOUND DDC_pdi_FOUND DDC_splines_FOUND)
assert(NOT DEFINED CACHE{${tgt}})
endforeach()There was a problem hiding this comment.
It is more for the explanatory
In the current implementation, the similar checks are repeated
I would hope the CMake code itself is self explanatory and intuitive. That would avoid out of date comments.
it is not explained what you expect in each check
I am not sure to understand, I expect what is implemented in each check.
Overall I think I can add a comment about the general idea: DDC components are additive, only requested components will be defined, if not already defined. The PR is not changing any behavior but only adding a test for it.
There was a problem hiding this comment.
It is more for the explanatory
In the current implementation, the similar checks are repeatedI would hope the CMake code itself is self explanatory and intuitive. That would avoid out of date comments.
I understand that self explanatory CMake code is nicer. To me, line by line checks are not particularly readable.
Also, this implementation is not quite scalable. If we increment the components, we will likely make mistakes.
it is not explained what you expect in each check
I am not sure to understand, I expect what is implemented in each check.
It is not clear what you would like to explain with comments.
For example, the following comment ## Test targets is repeated, but it is clear that you are testing targets.
I would rather like to know why and what you expect in each check.
## Test targets
assert(NOT TARGET DDC::core)
assert(NOT TARGET DDC::fft)
assert(NOT TARGET DDC::pdi)
assert(NOT TARGET DDC::splines)
...
# Test whether `find_package(DDC)` can be called multiple times
find_package(DDC 0.10 REQUIRED)
## Test targets
assert(TARGET DDC::core)
assert(NOT TARGET DDC::fft)
assert(NOT TARGET DDC::pdi)
assert(NOT TARGET DDC::splines)Overall I think I can add a comment about the general idea: DDC components are additive, only requested components will be defined, if not already defined. The PR is not changing any behavior but only adding a test for it.
That would be useful
tpadioleau
left a comment
There was a problem hiding this comment.
Thanks for the review, adding a documentation for the internal CMake assert function.
| ## Test targets | ||
| assert(NOT TARGET DDC::core) | ||
| assert(NOT TARGET DDC::fft) | ||
| assert(NOT TARGET DDC::pdi) | ||
| assert(NOT TARGET DDC::splines) | ||
| ## Test normal variables | ||
| assert(NOT DEFINED DDC_FOUND) | ||
| assert(NOT DEFINED DDC_BUILD_DOUBLE_PRECISION) | ||
| assert(NOT DEFINED DDC_fft_FOUND) | ||
| assert(NOT DEFINED DDC_pdi_FOUND) | ||
| assert(NOT DEFINED DDC_splines_FOUND) | ||
| ## Test cache variables | ||
| assert(NOT DEFINED CACHE{DDC_FOUND}) | ||
| assert(NOT DEFINED CACHE{DDC_BUILD_DOUBLE_PRECISION}) | ||
| assert(NOT DEFINED CACHE{DDC_fft_FOUND}) | ||
| assert(NOT DEFINED CACHE{DDC_pdi_FOUND}) | ||
| assert(NOT DEFINED CACHE{DDC_splines_FOUND}) |
There was a problem hiding this comment.
Not sure, for the tests below I will need two loops for the targets and normal variable sections. I am not convinced we gain much in readability:
foreach(tgt IN ITEMS DDC::core)
assert(TARGET ${tgt})
endforeach()
foreach(tgt IN ITEMS DDC::fft DDC::pdi DDC::splines)
assert(NOT TARGET ${tgt})
endforeach()
foreach(tgt IN ITEMS DDC_FOUND DDC_BUILD_DOUBLE_PRECISION)
assert(DEFINED ${tgt})
endforeach()
foreach(tgt IN ITEMS DDC_fft_FOUND DDC_pdi_FOUND DDC_splines_FOUND)
assert(NOT DEFINED ${tgt})
endforeach()
foreach(tgt IN ITEMS DDC_FOUND DDC_BUILD_DOUBLE_PRECISION DDC_fft_FOUND DDC_pdi_FOUND DDC_splines_FOUND)
assert(NOT DEFINED CACHE{${tgt}})
endforeach()Regarding the comments, I feel assertions are really short enough to be self explanatory.
install_testmore robust.