-
Notifications
You must be signed in to change notification settings - Fork 94
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
Fixing External Include of Build Directory #1532
Merged
Merged
Changes from all commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
37f70b6
make `test_exportbuild` fail
MarcelKoch cd2b37c
remove GINKGO_EXPORT_BUILD_DIR
MarcelKoch 8967810
spilt extract binary dir export from install
MarcelKoch 0f23a81
use `set_and_check` and `check_required_components`
MarcelKoch cd741d2
enable test_exportbuild by default
MarcelKoch File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -25,5 +25,4 @@ | |
CONFIG_LOG: "ON" | ||
CXX_FLAGS: "" | ||
EXTRA_CMAKE_FLAGS: "" | ||
EXPORT_BUILD_DIR: "OFF" | ||
CI_PROJECT_DIR_SUFFIX: "" |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
to ensure it still works after installation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 callingninja
and thenninja install
caused the HIP objects to be rebuilt. If we tackle #1334, that is no longer an issue and we can separate betweenninja
andninja install
againThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for the issue that hip needs built twice, we can just run
ninja && ninja
if we do not wantninja 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.