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: cmake install shouldn't hardcode install path #241

Merged
merged 3 commits into from
Feb 14, 2024

Conversation

filippobrizzi
Copy link
Contributor

@filippobrizzi filippobrizzi commented Feb 2, 2024

Cmake install shouldn't hardcode install path.

@eclipse-zenoh-bot
Copy link
Contributor

@filippobrizzi If this pull request contains a bugfix or a new feature, then please consider using Closes #ISSUE-NUMBER syntax to link it to an issue.

@filippobrizzi filippobrizzi changed the title Fix: cmake install shouldn' Fix: cmake install shouldn't hardcode install path Feb 2, 2024
@eclipse-zenoh-bot
Copy link
Contributor

@filippobrizzi If this pull request contains a bugfix or a new feature, then please consider using Closes #ISSUE-NUMBER syntax to link it to an issue.

1 similar comment
@eclipse-zenoh-bot
Copy link
Contributor

@filippobrizzi If this pull request contains a bugfix or a new feature, then please consider using Closes #ISSUE-NUMBER syntax to link it to an issue.

@filippobrizzi
Copy link
Contributor Author

PTAL @p-avital @milyin

@Mallets Mallets requested a review from milyin February 12, 2024 09:06
Copy link
Contributor

@milyin milyin left a comment

Choose a reason for hiding this comment

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

Nice approach. This should allow to move lib and include directories with zenoh-c installed into different place if necessary. I agree with the change.
@filippobrizzi can you tell, is it some standard recommendation to avoid absolute paths in cmake package file? I'd be grateful if you add some link about it

@filippobrizzi
Copy link
Contributor Author

filippobrizzi commented Feb 13, 2024

Nice approach. This should allow to move lib and include directories with zenoh-c installed into different place if necessary. I agree with the change. @filippobrizzi can you tell, is it some standard recommendation to avoid absolute paths in cmake package file? I'd be grateful if you add some link about it

@milyin A reference I found is from the CMake documentation https://cmake.org/cmake/help/v3.17/guide/tutorial/index.html#adding-export-configuration-step-11.

I have to admit my solution is halfway elegant as in theory this piece of code

get_filename_component(_IMPORT_PREFIX "${CMAKE_CURRENT_LIST_FILE}" PATH)
get_filename_component(_IMPORT_PREFIX "${_IMPORT_PREFIX}" PATH)
get_filename_component(_IMPORT_PREFIX "${_IMPORT_PREFIX}" PATH)
get_filename_component(_IMPORT_PREFIX "${_IMPORT_PREFIX}" PATH)
if(_IMPORT_PREFIX STREQUAL "/")
  set(_IMPORT_PREFIX "")
endif()

should be automatically generated from CMake when calling configure_package_config_file, but in this case it doesn't work as we create an IMPORTED library.

I have searched extensively on Google for an elegant solution for imported targets but with no luck. This is the best working solution I could come up with.

@milyin milyin merged commit 5bffd9e into eclipse-zenoh:main Feb 14, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants