-
Notifications
You must be signed in to change notification settings - Fork 14.6k
[libclc] Fix installed symlinks to be relative again #149728
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
Conversation
Fix the symlink creation logic to use relative paths instead of absolute, in order to ensure that the installed symlinks actually refer to the installed .bc files rather than the ones from the build directory. This was broken in llvm#146833. The change is a bit roundabout but it attempts to preserve the spirit of llvm#146833, that is the ability to use multiple output directories (provided they all resides in `${LIBCLC_OUTPUT_LIBRARY_DIR}` and preserve the same structure in the installed tree). Signed-off-by: Michał Górny <[email protected]>
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.
Thanks
foreach( a IN LISTS ARG_ALIASES ) | ||
if(CMAKE_HOST_UNIX OR LLVM_USE_SYMLINKS) | ||
cmake_path(RELATIVE_PATH libclc_builtins_lib | ||
BASE_DIRECTORY ${LIBCLC_OUTPUT_LIBRARY_DIR} |
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.
It seems libclc_builtins_lib is always located in LIBCLC_OUTPUT_LIBRARY_DIR, so here probably we only need to set LIBCLC_LINK_OR_COPY_SOURCE to be file basename part of libclc_builtins_lib.
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.
Sure, but that's precisely what #146833 changed, apparently to account for the possibility of moving the files in the future.
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.
+@frasercrmck to confirm
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.
Perhaps best leave it as you've got it here, as it's more robust to changes. If (some) files are moved in the future, everything would likely still be relative to LIBCLC_OUTPUT_LIBRARY_DIR
which is the thing that would most likely change. But I don't see the harm in having it as is.
Thanks! |
Fix the symlink creation logic to use relative paths instead of absolute, in order to ensure that the installed symlinks actually refer to the installed .bc files rather than the ones from the build directory. This was broken in #146833. The change is a bit roundabout but it attempts to preserve the spirit of #146833, that is the ability to use multiple output directories (provided they all resides in
${LIBCLC_OUTPUT_LIBRARY_DIR}
and preserve the same structure in the installed tree).