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

Drop extra install rules #126

Merged
merged 1 commit into from
Jan 27, 2025
Merged

Conversation

ilya-lavrenov
Copy link
Contributor

@CLAassistant
Copy link

CLAassistant commented Jan 7, 2025

CLA assistant check
All committers have signed the CLA.

@bashbaug bashbaug requested a review from Beanavil January 7, 2025 16:08
@Beanavil Beanavil mentioned this pull request Jan 7, 2025
Copy link
Member

@Beanavil Beanavil left a comment

Choose a reason for hiding this comment

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

The change LGTM, thanks for noticing this! It actually also uncovers a bug in the already-existing install command for the Utils and the Extensions that comes from the same commit that introduced the duplicated install(). IINM all target types should be part of the same component, for instance for the utils something like the following should be done:

  install(
    TARGETS ${UTIL_LIB_TARGET}
    EXPORT OpenCL${UTIL_LIB_NAME}Targets
    ARCHIVE
      DESTINATION ${CMAKE_INSTALL_LIBDIR}
      COMPONENT binary
    LIBRARY
      DESTINATION ${CMAKE_INSTALL_LIBDIR}
      COMPONENT binary
    RUNTIME
      DESTINATION ${CMAKE_INSTALL_BINDIR}
      COMPONENT binary
  )

@ilya-lavrenov
Copy link
Contributor Author

The change LGTM, thanks for noticing this! It actually also uncovers a bug in the already-existing install command for the Utils and the Extensions that comes from the same commit that introduced the duplicated install(). IINM all target types should be part of the same component, for instance for the utils something like the following should be done:

  install(
    TARGETS ${UTIL_LIB_TARGET}
    EXPORT OpenCL${UTIL_LIB_NAME}Targets
    ARCHIVE
      DESTINATION ${CMAKE_INSTALL_LIBDIR}
      COMPONENT binary
    LIBRARY
      DESTINATION ${CMAKE_INSTALL_LIBDIR}
      COMPONENT binary
    RUNTIME
      DESTINATION ${CMAKE_INSTALL_BINDIR}
      COMPONENT binary
  )

good catch! Updated.

@ilya-lavrenov ilya-lavrenov requested a review from Beanavil January 7, 2025 20:40
@bashbaug
Copy link
Contributor

Hi, can you please sign the CLA so we can accept this change? Sometimes the CLA bot can get stuck, so if you've already done this let us know and we'll see if we can get it un-stuck. Thanks!

Signed-off-by: Ilya Lavrenov <[email protected]>
@ilya-lavrenov
Copy link
Contributor Author

Hi, can you please sign the CLA so we can accept this change? Sometimes the CLA bot can get stuck, so if you've already done this let us know and we'll see if we can get it un-stuck. Thanks!

done

@bashbaug
Copy link
Contributor

Hi @ilya-lavrenov , the CLA task still doesn't show complete. Can you please check that everything looks right on your end? If so, we'll need to figure out what's going on with the bot - thanks!

@ilya-lavrenov
Copy link
Contributor Author

Hi @ilya-lavrenov , the CLA task still doesn't show complete. Can you please check that everything looks right on your end? If so, we'll need to figure out what's going on with the bot - thanks!

done

@bashbaug bashbaug merged commit 330af5a into KhronosGroup:main Jan 27, 2025
122 checks passed
@ilya-lavrenov ilya-lavrenov deleted the install-rules branch January 27, 2025 06:45
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.

4 participants