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

Fix static build #360

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

jmachado-amd
Copy link
Collaborator

Hard-code -I${HIP_INCLUDE_DIRS} as a compile option of target example-c-basic to fix the static build.

Hard-code -I${HIP_INCLUDE_DIRS} as a compile option of
target `example-c-basic` to fix the static build.
@@ -26,6 +26,7 @@ add_executable(example-cpp-basic example_basic.cpp)
# We test for C99 compatibility in the example-c.c test
set_source_files_properties(example_basic.c PROPERTIES LANGUAGE CXX)
set_source_files_properties(example_basic.c PROPERTIES COMPILE_FLAGS "-xc -std=c99")
target_compile_options(example-c-basic PRIVATE -I${HIP_INCLUDE_DIRS})

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder perhaps this option -I${HIP_INCLUDE_DIRS} is safe and harmless for other examples as well. Just a thought.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The thing is that, unfortunately, this change should not have been necessary. The include directory pointed by ${HIP_INCLUDE_DIRS} is being directly, and properly, set on target example-c-basic by the target_include_directories in line 36; as well as being also indirectly set by the nearby target_link_libraries in some code paths.

The problem lies in the fact that current static libraries builds are purposefully ignoring the hip runtime include dirs set in the proper way.

Copy link
Collaborator

@cgmb cgmb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

-1. We are going to need to fix the actual root cause because a bug in hip::host is certain to affect other users besides hipsolver. And if we fix the root cause, there will be no need for this workaround.

@jmachado-amd
Copy link
Collaborator Author

jmachado-amd commented Feb 6, 2025

-1. We are going to need to fix the actual root cause because a bug in hip::host is certain to affect other users besides hipsolver. And if we fix the root cause, there will be no need for this workaround.

@cgmb, I am afraid that this is not a bug, as its stems from a series of design choices made in the hip runtime, i.e., the issue at hand is a result of their intended behaviour.

I agree that this is surely a major pain to all libraries, and that it should have been communicated to everyone; but the one thing that we can and have to fix is to make sure that we get the memo the next time something similar happens.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants