Skip to content

[SYCL][NFC] Use relative paths for SYCL headers install #3940

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

Merged
merged 1 commit into from
Jun 18, 2021

Conversation

npmiller
Copy link
Contributor

${SYCL_INCLUDE_DEPLOY_DIR} is defined as
${CMAKE_INSTALL_PREFIX}/${SYCL_INCLUDE_DIR}, this is unnecessary
because when using relative paths, CMake will automatically prefix
install destinations with ${CMAKE_INSTALL_PREFIX}, so we can directly
use ${SYCL_INCLUDE_DIR}.

It is also preferable to use relative paths, because it enables the
DESTDIR mechanism that can make it possible to change the install
prefix after the configuration, with for example things like make DESTDIR=/new/install/prefix install, or when manually calling the CMake
install scripts.

This patch has no functional changes and should have no effect on
existing builds, aside from enabling DESTDIR.

@npmiller npmiller requested a review from a team as a code owner June 16, 2021 16:26
@npmiller npmiller requested a review from dm-vodopyanov June 16, 2021 16:26
@dm-vodopyanov dm-vodopyanov changed the title [SYCL] Use relative paths for SYCL headers install [SYCL][NFC] Use relative paths for SYCL headers install Jun 16, 2021
Copy link
Contributor

@dm-vodopyanov dm-vodopyanov left a comment

Choose a reason for hiding this comment

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

Other part LGTM. Thanks for the patch!

`${SYCL_INCLUDE_DEPLOY_DIR}` is defined as
`${CMAKE_INSTALL_PREFIX}/${SYCL_INCLUDE_DIR}`, this is unnecessary
because when using relative paths, CMake will automatically prefix
install destinations with `${CMAKE_INSTALL_PREFIX}`, so we can directly
use `${SYCL_INCLUDE_DIR}`.

It is also preferable to use relative paths, because it enables the
`DESTDIR` mechanism that can make it possible to change the install
prefix after the configuration, with for example things like `make
DESTDIR=/new/install/prefix install`, or when manually calling the CMake
install scripts.

This patch has no functional changes and should have no effect on
existing builds, aside from enabling `DESTDIR`.
@npmiller npmiller force-pushed the sycl-cmake-install-fix branch from 384e583 to 41748ee Compare June 16, 2021 18:17
Copy link
Contributor

@dm-vodopyanov dm-vodopyanov left a comment

Choose a reason for hiding this comment

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

Thanks, LGTM. Please avoid force-pushing in the future, it makes the reviewing difficult as the info about changes disappears.

@pvchupin pvchupin merged commit 808b38d into intel:sycl Jun 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants