8000 Add options to control install behaviour in ASAN builds by lawruble13 · Pull Request #138 · ROCm/rocm-cmake · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Add options to control install behaviour in ASAN builds #138

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.

Sign up for GitHub

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

Jump to bottom
Draft
wants to merge 3 commits into
base: release-staging/rocm-rel-5.7
Choose a base branch
from
Draft

Add options to control install behaviour in ASAN builds #138

wants to merge 3 commits into from

Conversation

lawruble13
Copy link
Collaborator

No description provided.

@lawruble13 lawruble13 requested review from pfultz2 and cgmb as code owners July 13, 2023 20:08
lawruble13
lawruble13 commented Jul 13, 2023
View reviewed changes
@lawruble13 lawruble13 marked this pull request as draft July 13, 2023 20:21
@pfultz2
Copy link
Collaborator
pfultz2 commented Jul 13, 2023

So PRIVATE means its not a user facing file, and is only used internally by the library. As such, it should be installed when using asan and not asan builds. For PRIVATE installs it should go into lib/<project-name>/ or lib/asan/<project-name>. This way the relative path to the file should be the same regardless if its asan or non-asan.

@lawruble13
Copy link
Collaborator Author

So PRIVATE means its not a user facing file, and is only used internally by the library. As such, it should be installed when using asan and not asan builds.

Ah, I misunderstood what you had said in our last discussion. Should there then be a flag to force a target to be ignored in the ASAN build, in addition to this PRIVATE flag?

pfultz2
pfultz2 reviewed Jul 14, 2023
View reviewed changes
share/rocm/cmake/ROCMInstallTargets.cmake
@@ -146,6 +156,11 @@ function(rocm_install_targets)
set(INCLUDE_INSTALL_DIR ${CMAKE_INSTALL_INCLUDEDIR})
endif()

if (PARSE_PRIVATE)
set(BIN_INSTALL_DIR ${BIN_INSTALL_DIR}/${PROJECT_NAME})
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should be ${LIB_INSTALL_DIR}/${PROJECT_NAME}.

pfultz2
pfultz2 reviewed Jul 14, 2023
View reviewed changes
share/rocm/cmake/ROCMInstallTargets.cmake
@@ -146,6 +156,11 @@ function(rocm_install_targets)
set(INCLUDE_INSTALL_DIR ${CMAKE_INSTALL_INCLUDEDIR})
endif()

if (PARSE_PRIVATE)
set(BIN_INSTALL_DIR ${BIN_INSTALL_DIR}/${PROJECT_NAME})
set(LIB_INSTALL_DIR ${LIB_INSTALL_DIR}/${PROJECT_NAME})
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this would be ${LIB_INSTALL_DIR}/${PROJECT_NAME}/lib. Also, we should probably do this for include as well.

@pfultz2
Copy link
Collaborator
pfultz2 commented Jul 14, 2023

Should there then be a flag to force a target to be ignored in the ASAN build, in addition to this PRIVATE flag?

No, we should just ignore everything thats not a shared object by default in ASAN builds. Then the PRIVATE flag can install the file into lib/${PROJECT_NAME} or lib/asan/${PROJECT_NAME} for runtime components when ASAN is enabled.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pfultz2 pfultz2 pfultz2 left review comments

@cgmb cgmb Awaiting requested review from cgmb cgmb is a code owner

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@lawruble13 @pfultz2
0