Add KDSME as submodule for CI testing #905
Conversation
dantti
force-pushed
the
dantti/kdsme_submodule
branch
5 times, most recently
from
6abf61f to
fc44db5
Compare
dantti
force-pushed
the
dantti/kdsme_submodule
branch
from
fc44db5 to
0e7b8c2
Compare
dantti
force-pushed
the
dantti/kdsme_submodule
branch
from
0e7b8c2 to
8953e06
Compare
dantti
force-pushed
the
dantti/kdsme_submodule
branch
from
8953e06 to
fd9a93a
Compare
| find_package(${KDSME_PACKAGE_NAME} 1.2 CONFIG QUIET) | ||
|
|
||
| # UI part | ||
| if(GAMMARAY_BUILD_UI AND ${KDSME_PACKAGE_NAME}_FOUND) | ||
| if(GAMMARAY_BUILD_UI AND GAMMARAY_WITH_KDSME) |
There was a problem hiding this comment.
This breaks the ability to use a KDSME package installed separately on the system, doesn't it? Since find_package() is no longer even attempted, even if KDSME is installed, it won't be used — the submodule is the only option. And if the .git directory isn't present (e.g. package releases), there's no way to make it use a KDSME at all.
Distro packagers won't like that, for one. ...Unless I'm completely missing something?
There was a problem hiding this comment.
FTR, my suggestion would be to keep the find_package(), but insert a condition to trigger the submodule stuff:
if(NOT ${KDSME_PACKAGE_NAME}_FOUND AND GAMMARAY_WITH_KDSME)Maybe even change the option name to GAMMARAY_USE_INTERNAL_KDSME or something.
There was a problem hiding this comment.
There was a problem hiding this comment.
A fourth option:
- Keep the submodule, but instead of using it in the build with
add_subdirectory(), have the CI build-and-install KDSME from there before it configures GammaRay. Then pick it up withfind_package()as usual.
The built KDSME could even be added to the GitHub cache, to speed things up.
There was a problem hiding this comment.
Or, in fact, you could use FetchContent to incorporate the submodule'd KDSME — with CMake 3.24+, if you give FetchContent_Declare() a FIND_PACKAGE_ARGS argument, it'll automatically automatically try a find_package() first, and use the installed package if it's found.
(FetchContent_Declare() doesn't have to download it from a remote repo. Like ExternalProject_Add(), you can give it a SOURCE_DIR argument with the path to existing sources.)
There was a problem hiding this comment.
FetchContent doesn't solve the include issue, it's just a little better ExternalProject_Add() but we are avoiding it in favor of submodules.
dantti
force-pushed
the
dantti/kdsme_submodule
branch
from
fd9a93a to
02f7f6a
Compare
|
the distro packagers won't care because the released tarball will include the submodule. If I do things correctly when I make the release the point is : graphviz is wildly different on all supported platforms and we can't keep chasing the different API |
No description provided.