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

Install CMake config to architecture-specific location #364

Merged
merged 1 commit into from
Dec 22, 2024

Conversation

cottsay
Copy link
Contributor

@cottsay cottsay commented Dec 20, 2024

As an architecture-specific CMake project, the CMake config files should be installed to an architecture-specific location.

This issue was identified during the process of packaging apriltag for inclusion in Fedora Linux and EPEL[1]. You can see a similar patch was added to the debian build process for Ubuntu[2].

Related docs: https://cmake.org/cmake/help/latest/command/find_package.html#config-mode-search-procedure

[1] https://bugzilla.redhat.com/show_bug.cgi?id=2173758#c17
[2] https://salsa.debian.org/science-team/apriltag/-/blob/master/debian/patches/cmake-into-arch-specific-path.patch

As an architecture-specific CMake project, the CMake config files should
be installed to an architecture-specific location.
@christian-rauch
Copy link
Collaborator

Thanks for pointing that out. The Debian patch changes that install path to lib/${CMAKE_LIBRARY_ARCHITECTURE}/cmake/${PROJECT_NAME}, you are proposing to change it to ${CMAKE_INSTALL_LIBDIR}/${PROJECT_NAME}/cmake. With this PR applied, doesn't this mean that Debian will still need to manually patch the CMakeLists.txt? Would it make more sense to just apply the patch that Debian is already applying? Then, they could directly use the upstream version.

@cottsay
Copy link
Contributor Author

cottsay commented Dec 21, 2024

doesn't this mean that Debian will still need to manually patch the CMakeLists.txt?

On Debian, CMAKE_INSTALL_LIBDIR will resolve to lib/${CMAKE_LIBRARY_ARCHITECTURE}. I'm not really sure why the package maintainers patched it like that, but CMAKE_INSTALL_LIBDIR should do the same thing on Debian, but will also do the right thing on Fedora and RedHat where we want it to be lib64.

This should mean that neither Debian nor Fedora maintainers need to apply a patch for this.

@christian-rauch christian-rauch merged commit fc2a7b2 into AprilRobotics:master Dec 22, 2024
39 checks passed
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.

2 participants