Repository navigation
Conversation
aaa2c4f to
5618e4b
Compare
|
@LecrisUT I think my open ticket is waiting for this PR. |
|
Yes, but we should get #125 to be easier to review, at the very least for me to re-review myself 😅 |
5618e4b to
4446c06
Compare
Signed-off-by: Cristian Le <git@lecris.dev>
Signed-off-by: Cristian Le <git@lecris.dev>
Signed-off-by: Cristian Le <git@lecris.dev>
4446c06 to
3af8f08
Compare
| @@ -0,0 +1,21 @@ | |||
| cmake_minimum_required(VERSION 3.17...4.0) | |||
There was a problem hiding this comment.
I don't remember why I set this one. But if anyone sees an issue with it just ping me. It might have been related to add_library(INTERFACE IMPORTED) or something else in the parts below.
There was a problem hiding this comment.
Does that mean 4.0 is the max? There is already a 4.1 release candiate.
There was a problem hiding this comment.
No, it means use policies up to version 4.0 as NEW. Read it as we have tested this part with "all cmakes" between <min>...<max>. The comment is about why I chose 3.17 as the minimum. For Config.cmake files, the minimum should be as low as possible.
There was a problem hiding this comment.
Would recommend scalapack-config.cmake.in instead. Same as other related file name.
There was a problem hiding this comment.
Hmm, I thought that one was an outdated form of the other, but I checked the history again, and both forms were always supported since 3.0. Sure, no big preference on my side.
There was a problem hiding this comment.
scalapack-config.cmake.in allows users to use either find_package(scalapack) or find_package(SCALAPACK) or others, while scalapackConfig.cmake.in limit it to only find_package(scalapack). Usually I would prefer the former case 🤔
There was a problem hiding this comment.
Hmm, do you have a reference for this? The find_package logic is a piece of art, but I don't recall this behavior difference.
There was a problem hiding this comment.
There was a problem hiding this comment.
Aha I see it is in the help message, ok it rings a bell now. Was there a reason why I preferred the fixed naming convention? gtest and catch2 use the Config.cmake model, but I did see -config.cmake ones as well, but can't find a more recent popular project.
We would need to standardize the name though, so keeping -config.cmake makes sense until then
| RUNTIME DESTINATION Testing | ||
| ) | ||
| install(TARGETS ${lib} | ||
| EXPORT scalapackTargets |
There was a problem hiding this comment.
Similar here, and I think we should directly drop the macro as the TODO proposes.
There was a problem hiding this comment.
I lost the context on the naming convention here. I thought there was a part of the interface between install(TARGETS), install(EXPORT) and export(EXPORT) that used a *Targets as default, but I tried to find it again and nothing 🤷. Will have to check the generated files again as well, but otherwise np with the old name.
There was a problem hiding this comment.
Sounds a bit strange to me. It seems the build system isn't clean enough.
There was a problem hiding this comment.
It seems the build system isn't clean enough.
For something that is more than 1 year old, probably 😁. But there were quite a few pitfalls around find_package logic that I was probably looking into in parallel at that time.
| INSTALL_DESTINATION ${CMAKE_INSTALL_LIBDIR}/cmake/scalapack | ||
| ) | ||
| write_basic_package_version_file( | ||
| scalapackConfigVersion.cmake |
|
I openend #170 towards an overall cleanup, and I'd be happy if you'd like to take a look :) |
First PR in the refactoring series. Use modern design for creating the
Config.cmakefile.Depends-on: #123, #125
Fixes #109
(Mostly, the targets created and exported are still not visible, but I want to address that in another PR that focuses on readability, which would be the last in the series)