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 invalid CMAKE_INSTALL_INCLUDEDIR concatenation. #481

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

chpatrick
Copy link

@chpatrick chpatrick commented Oct 3, 2022

See: https://github.com/jtojnar/cmake-snips#assuming-cmake_install_dir-is-relative-path

This fixes importing the CMake config on NixOS.

@CLAassistant
Copy link

CLAassistant commented Oct 3, 2022

CLA assistant check
All committers have signed the CLA.

@imciner2
Copy link
Member

imciner2 commented Oct 7, 2022

Thank you for spotting this. We don't merge directly into the master branch though, and we are currently working on the develop-1.0 branch for the next release. Can you retarget this PR to be against the develop-1.0 branch and its CMake system (we have made some changes to how the CMake is organized).?

@chpatrick chpatrick changed the base branch from master to develop-1.0 October 12, 2022 23:55
@chpatrick
Copy link
Author

Done, please take a look.

@imciner2
Copy link
Member

Thanks for rebasing this. I have one question now about how this fits into the new CMake flow we have in the develop-1.0 branch. In that branch we now use configure_package_config_file to configure a CMake config file that we install on the system, so then any projects using CMake can get the libraries through this. According to the CMake documentation (https://cmake.org/cmake/help/latest/module/CMakePackageConfigHelpers.html), this should remove the absolute paths and make a relocatable package. Does this work on the Nix system? Do we need to have this change in addition to the osqp-config.cmake file?

@chpatrick
Copy link
Author

I just checked, the develop-1.0 without this change still produces:

  INTERFACE_INCLUDE_DIRECTORIES "${_IMPORT_PREFIX}//nix/store/z58s4i1nbyy9cmnh6bj40icdkpfnvkkl-osqp-0.6.2/include/osqp"

in osqp-targets.cmake, which isn't right.

If I use "$<INSTALL_INTERFACE:${CMAKE_INSTALL_FULL_INCLUDEDIR}/osqp>", I get:

  INTERFACE_INCLUDE_DIRECTORIES "/nix/store/glr55pijnwjkz8bxpnby21807gjw94q1-osqp-0.6.2/include/osqp

Which works on Nix (and I guess on Linux in general) but probably isn't relocatable.

If I set "$<INSTALL_INTERFACE:$<INSTALL_PREFIX>/include/osqp>", I get:

  INTERFACE_INCLUDE_DIRECTORIES "${_IMPORT_PREFIX}/include/osqp"

Which I suppose is relocatable. Would that work for you?

@imciner2
Copy link
Member

Thanks for the experiments, that is definitely useful. What is concerning to me though is the fact we then have to hardcode the include in the path in the third example, which goes against the idea of using the GNUInstallDir package and the install command into CMAKE_INSTALL_INCLUDEDIR later in the file. I did a bit of digging in the docs/issue tracker and found this comment: https://gitlab.kitware.com/cmake/cmake/-/issues/20250#note_685387. It does agree with your statement that we shouldn't use the prefix in that interface statement, but it suggests to instead just leave it out. Can you try instead just removing the $<INSTALL_PREFIX>/ from the interfaces and seeing if that makes it relocatable and work on Nix?

@notalltim
Copy link

I would love to get this fixed to clarify @imciner2 you would like "$<INSTALL_INTERFACE:osqp>"?

@imciner2
Copy link
Member

imciner2 commented Jun 7, 2023

I would love to get this fixed to clarify @imciner2 you would like "$<INSTALL_INTERFACE:osqp>"?

My concern is still the relocatability of the pathing. Can you run the experiments on Nix to see if that does fix the issue?

@notalltim
Copy link

created a new pr #567 because I added another change that was required for nix to work

@imciner2 imciner2 changed the base branch from develop-1.0 to master April 4, 2024 19:38
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.

None yet

4 participants