Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update path for HIP in GinkgoConfig.cmake.in #680

Closed
wants to merge 1 commit into from

Conversation

cho-uc
Copy link

@cho-uc cho-uc commented Dec 21, 2020

GinkgoConfig.cmake will be created in build directory, not in ginkgo cmake, therefore need to add "/../cmake/" as path

GinkgoConfig.cmake will be created in build directory, not in ginkgo cmake, therefore need to add "/../cmake/" as path
@tcojean
Copy link
Member

tcojean commented Dec 22, 2020

Hi! Thanks for this PR, you are correct we have an issue with this from the perspective of the build directory. One thing to consider though, is that the current setup works when installing Ginkgo (as the helpers are copied to the same directory as the GinkgoConfig.cmake), and your version would make us lose this ability.

We should probably fix this in another way, some ways to fix it that I can think of are:

  • Somehow adding a condition based on whether we installed or not Ginkgo (can be variable controlled, defined in the installation step, or maybe CMake has something for that), and switch the path depending on this "installed" condition.
  • Simply copy the two helper files from the cmake directory to the build directory.

For now, I am more leaning towards option 2. What do you think? Also @ginkgo-project/developers.

@pratikvn
Copy link
Member

option 2 also sounds better to me.

@pratikvn pratikvn added is:confirmed Someone confirmed this issue. reg:build This is related to the build system. labels Dec 22, 2020
@Slaedr
Copy link
Contributor

Slaedr commented Dec 22, 2020

If we are reasonably sure that these are the only two files that will need this kind of change when Ginkgo is used without installation, then option 2 sounds good. Otherwise, we might want to look at option 1 for a general solution in which we can fine-tune everything for a kind of in-source use case.

The package registry option that @tcojean mentioned is also an interesting idea for a more general solution, but I feel it's not the most suitable for this PR's use case. It seems like the idea is to have a Ginkgo build specifically for one dependent project, rather than user-wide or system-wide.

@upsj
Copy link
Member

upsj commented Dec 22, 2020

Option 1 sounds more brittle to me, since it sounds to me like we would need to assume that a build directory is used either to install Ginkgo, or to include from directly, but not both (since variables are more or less fixed, only generator expressions can depend on the output).

@cho-uc Just so we understand what you are looking for, can you tell us what your current mode of use is? Right now, we support and test for Ginkgo installed with make/ninja install and find_package(Ginkgo) or as a subdirectory build add_subdirectory(ginkgo), it sounds like we are missing another option you are using?

@cho-uc
Copy link
Author

cho-uc commented Dec 25, 2020

Since I don't have admin privilege in the server where I installed ginkgo, I can only built locally, run "make" but not "install".
Previously, when I run CMake for test_install, it gave me error that it cannot find hip_helpers.cmake and windows_helpers.cmake.
Once I fixed it, test_install was successfully built.

@upsj
Copy link
Member

upsj commented Dec 25, 2020

Thanks for the details, I see the issue now. Another solution to this issue would be to install Ginkgo in a custom user-local CMAKE_INSTALL_PREFIX (in case you are not familiar with CMake variables, you can do this by setting it in your CMakeCache.txt, using ccmake, or completely recreating your build directory with the -DCMAKE_INSTALL_PREFIX=/home/.../... flag). I've been using this approach with many libraries or tools I didn't want to install system-wide, it usually works pretty well.

In our root CMakeLists.txt, the CMAKE_INSTALL_PREFIX gets propagated to the CMAKE_PREFIX_PATH for the test_install step which is then used in find_package(Ginkgo)

@cho-uc
Copy link
Author

cho-uc commented Dec 25, 2020

Ach I see, I didn't set CMAKE_INSTALL_PREFIX so it used the default value in /usr/local/ in which I don't have access to.
Do you think it's still beneficial to revise this config file or should I just close this PR?

@tcojean
Copy link
Member

tcojean commented Dec 30, 2020

Good that you found your solution. I think this file should still be revised. As we discussed, there is indeed an issue with this file, but maybe it's better to tackle this in a separate PR since we need to use another way, and probably add a test as well.

tcojean added a commit that referenced this pull request Dec 31, 2020
tcojean added a commit that referenced this pull request Dec 31, 2020
tcojean added a commit that referenced this pull request Dec 31, 2020
@cho-uc cho-uc closed this Dec 31, 2020
tcojean added a commit that referenced this pull request Jan 7, 2021
tcojean added a commit that referenced this pull request Jan 8, 2021
Also test linking through the build directory.

This fixes the problem with `GinkgoConfig.cmake` from a build directory perspective, while keeping a correct installation setup. To ensure this works, I also test linking Ginkgo after exporting the build directory so that CMake is able to use this when doing a `find_package()`. We already had an option for this, it's only a matter of actually using it.

Issue found by: #680
Related PR: #685
tcojean added a commit that referenced this pull request Aug 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
is:confirmed Someone confirmed this issue. reg:build This is related to the build system.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants