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

[cmake] Use GNU install dirs variables instead of hardcoded value #1892

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

DownerCase
Copy link
Contributor

Description

Removes hardcoded install destinations that aren't derived from the appropriate CMAKE_INSTALL_<DIR>, without this the special case CMAKE_INSTALL_PREFIX of / behaves incorrectly, with files split between (for example) /usr/include and /include

Replaces the following uses with their eCAL_install_<DIR> equivalents:

  • INSTALL_LIB_DIR
  • INSTALL_BIN_DIR
  • INSTALL_INCLUDE_DIR
  • INSTALL_CMAKE_DIR (was unused)

Moved the asioTargets.cmake from lib/CMake to lib/cmake consistent with elsewhere.

Gets hdf5 submodule to use the GNU install dirs.

This should only really make any difference at all when CMAKE_INSTALL_PREFIX is exactly /; which admittedly is rare and unusual. Beyond that, it does mean all the install paths are now user controllable.

Related issues

Cherry-pick to

  • none

Copy link
Contributor

@KerstinKeller KerstinKeller left a comment

Choose a reason for hiding this comment

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

Thanks for making the PR!

The only concern I have, that by using the values in the ecal or lang/csharp subfolders, we will no longer be able to build these components on their own.
We could introduce eCAL_core_xxx directories, or use the CMAKE_INSTALL_... variables directly there.

Or, as I think that specifying the install directories is no longer required in CMake (I think they were before CMake 3.15, or so), maybe it could be deleted altogether.

endif()
set(INSTALL_CMAKE_DIR ${DEF_INSTALL_CMAKE_DIR} CACHE PATH
"Installation directory for CMake files")

Copy link
Contributor

Choose a reason for hiding this comment

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

A bit strange that we still have those when we do provide all these eCAL_install directories.

@KerstinKeller KerstinKeller added the cherry-pick-to-NONE Don't cherry-pick these changes label Jan 13, 2025
@DownerCase
Copy link
Contributor Author

Thanks for making the PR!

The only concern I have, that by using the values in the ecal or lang/csharp subfolders, we will no longer be able to build these components on their own. We could introduce eCAL_core_xxx directories, or use the CMAKE_INSTALL_... variables directly there.

Not sure what you mean w.r.t the ecal subfolder as eCAL core already uses the values (eg:

#define ECAL_INSTALL_APP_DIR "@eCAL_install_app_dir@"
, https://github.com/eclipse-ecal/ecal-core/blob/e91a95c74fefc5e8d97eb19ed0788da022ed5e46/CMakeLists.txt#L154)

The csharp and python subfolders can't currently be built independently as they lack the requisite find_package(eCAL) calls (amongst other issues).

Or, as I think that specifying the install directories is no longer required in CMake (I think they were before CMake 3.15, or so), maybe it could be deleted altogether.

That's pretty accurate. For install(TARGETS ...) the docs note:

"For regular executables, static libraries and shared libraries, the DESTINATION argument is not required.
For these target types, when DESTINATION is omitted, a default destination will be taken from the appropriate variable from GNUInstallDirs, or set to a built-in default value if that variable is not defined.
"

For install(DIRECTORY ...) (i.e: old style method of install headers) "Either a TYPE or a DESTINATION must be provided, but not both." (and it will follow the same logic as above to pick a value)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cherry-pick-to-NONE Don't cherry-pick these changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants