Fixing External Include of Build Directory #1532
Conversation
INSTALL.md
Outdated
| For example, to build everything (in debug mode), use: | ||
|
|
||
| ```cmake | ||
| cmake -G "Unix Makefiles" -H. -BDebug -DCMAKE_BUILD_TYPE=Debug -DGINKGO_DEVEL_TOOLS=ON \ | ||
| cmake -S. -BDebug -DCMAKE_BUILD_TYPE=Debug -DGINKGO_DEVEL_TOOLS=ON \ |
There was a problem hiding this comment.
| cmake -S. -BDebug -DCMAKE_BUILD_TYPE=Debug -DGINKGO_DEVEL_TOOLS=ON \ | |
| cmake -S.. -BDebug -DCMAKE_BUILD_TYPE=Debug -DGINKGO_DEVEL_TOOLS=ON \ |
Maybe keep using the build (or any child folder) not the source directory as the build directory
cmake/install_helpers.cmake
Outdated
| if (GINKGO_EXPORT_BUILD_DIR) | ||
| export(PACKAGE Ginkgo) | ||
| endif() | ||
| export(PACKAGE Ginkgo) |
There was a problem hiding this comment.
could you explain more on this?
It does nothing without CMAKE_EXPORT_PACKAGE_REGISTRY=ON. Thus, it does not write anything to the user registry like ~/.cmake in the linux. (it does not affect us to use build directory as the external library)
never noticed cmake contains a user package registry before
There was a problem hiding this comment.
I think that already covers it. Without CMAKE_EXPORT_PACKAGE_REGISTRY=ON this is a no-op, so we don't need to guard it. Also setting GINKGO_EXPORT_BUILD_DIR had no effect on the other CMake option, so no need for that.
The CMake registry basically just stores the paths to the packages.
| - ninja -j${NUM_CORES} -l${CI_LOAD_LIMIT} install | ||
| - awk '!/^#/ { print ($2 - $1)/1000 " " $4 }' .ninja_log | sort -nr | ||
| - | | ||
| (( $(ctest -N | tail -1 | sed 's/Total Tests: //') != 0 )) || exit 1 | ||
| - ctest --output-on-failure --timeout 6000 ${CTEST_EXTRA_ARGS} | ||
| - ninja test_exportbuild |
There was a problem hiding this comment.
to ensure it still works after installation?
There was a problem hiding this comment.
I think that should not be necessary. The export test is only allowed to use the *_ROOT to find ginkgo, so the previous install should have no effect.
I would still keep this test before to prevent the current situation.
There was a problem hiding this comment.
Side-note: The reason we do ninja install directly in the beginning is that the HIP compilation doesn't create (or didn't used to create) the correct depfiles or something like that on the first pass, so calling ninja and then ninja install caused the HIP objects to be rebuilt. If we tackle #1334, that is no longer an issue and we can separate between ninja and ninja install again
There was a problem hiding this comment.
TBH, I think we should push the CMake update for HIP, even if that would mean to drop support for rocm < 5.0.1. I don't think it's even possible to find the documentation for rocm 4.5 from visiting amd.com, so it seems to me that these versions are not really supported anymore.
There was a problem hiding this comment.
for the issue that hip needs built twice, we can just run ninja && ninja if we do not want ninja install rebuilds the stuff.
I do not think the two build is a big issue.
Dropping the support rocm < 5.0.1 is not the main issue from updating cmake for HIP.
Updating cmake for hip means we drops the nvidia support of hip. In my mind, the new cmake is not completed yet because they claim they can do that on both architectures. Also, recovering the support after deleting is harder than keeping the current one.
There was a problem hiding this comment.
This is an issue of build time, our HIP builds (especially debug) are extremely slow, we don't want to build the binaries twice. It seems to me like the old hip_add_* approach hasn't been mentioned in their documentation in a long time, on top of all its downsides I also do not want to keep relying on it. We have full CUDA support via our CUDA backend, to me there are no technical reasons to keep this around, as HIP is just a thin layer over CUDA anyways.
4b5c882 to
bb25a80
Compare
bb25a80 to
95a8e2a
Compare
It only guards `export(PACKAGE Ginkgo)` which doesn't have any effects in CMake > v3.15, unless `CMAKE_EXPORT_PACKAGE_REGISTRY` is set.
fixup! spilt extract binary dir export from install
95a8e2a to
cd741d2
Compare
|
Error: PR already merged! |
|
We have two version of cmakeConfig and implementation in the code. Is there any way to reduce the possibility of missing partial changes in the future? |
This PR extracts the exporting of the binary dir from our install configuration. To properly allow external projects to import Ginkgo directly from the binary tree, it is necessary to create two separate sets of config files. Otherwise, the binary tree config would refer to directories that are part of the install tree, which might not exist.
Old Description:
The build directory is missing the
GinkgoTargets.cmakefile. This prevents external projects from using the Ginkgo build directory directly, instead they need to use the install directory.The issue was introduced in 75777b4. Reverting a small part of it fixes the issue.
I also changed the
test_exportbuildtest to only search Ginkgo using theGinkgo_ROOTpath. TBH, I'm not sure if that was the intention of the test, but it can reproduce both the failing and succeeding behavior.