Avoid absolute path for the include directory installation #3024
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This change makes COLMAP installation compatible with `--prefix` option from CMake `cmake --install` command.
For setting the instalation directories, CMake recommends to use the variables from GNUInstallDirs module.
|
Thanks, the fixes look good to me. There seems to be some unrelated build failures caused by missing secrets. Not sure why this is happening. |
|
OK, it seems like Github actions does not allow for access to secrets from pull requests originating from forks. This needs to be fixed separately and is unrelated to your PR. |
|
Thank you for letting me know. |
|
This should hopefully be fixed with this PR: #3027 |
ahojnnes
added a commit
that referenced
this pull request
Dec 10, 2024
Currently, PR from forks cannot run our CI pipeline, because secrets are not accessible for PRs originating from forks. See e.g. #3024. This PR fixes the issue by letting PRs from forks read but not write to the cache.
ahojnnes
approved these changes
Dec 11, 2024
HernandoR
pushed a commit
to HernandoR/colmap
that referenced
this pull request
Dec 30, 2024
Currently, PR from forks cannot run our CI pipeline, because secrets are not accessible for PRs originating from forks. See e.g. colmap#3024. This PR fixes the issue by letting PRs from forks read but not write to the cache.
HernandoR
pushed a commit
to HernandoR/colmap
that referenced
this pull request
Dec 30, 2024
The installation path for the COLMAP's include directory is currently
set to `${CMAKE_INSTALL_PREFIX}/include` in [this
line](https://github.com/colmap/colmap/blob/2686b86526fa0c9ff4213bd90cd4af5c149bfcae/CMakeLists.txt#L374).
This works perfectly if during the configuration the cache variable
`-DCMAKE_INSTALL_PREFIX=custom/path/` is set. However, if we don't set
it, and after building the project we want to use the `--prefix` option
of the `cmake --install` command to set a custom installation prefix,
the include directory won't be set properly in the
`colmap-targets.cmake` file. This means that the property
`INTERFACE_INCLUDE_DIRECTORIES` of the `colmap::colmap` target will be
affected by the default value of `CMAKE_INSTALL_PREFIX` (`/usr/local`)
and it will be set to the absolute path `/usr/local/include` on Linux
when `CMAKE_INSTALL_PREFIX` is not provided. This will produce include
errors when using COLMAP as a library if we intended to set a different
path with `--prefix`. In the [CMake
documentation](https://cmake.org/cmake/help/latest/command/install.html#introduction)
it's also recommended to avoid absolute paths when setting the
destination directories, because they are not compatible with the
`--prefix` option of the `cmake --install` command.
Before the introduced changes, if we install COLMAP by using `cmake
--install . --prefix custom/path` (without setting
`-DCMAKE_INSTALL_PREFIX`) we will get in the `colmap-targets.cmake`
file:
```
set_target_properties(colmap::colmap PROPERTIES
INTERFACE_INCLUDE_DIRECTORIES "/urs/local/include"
INTERFACE_LINK_LIBRARIES ...
)
```
and after the proposed changes:
```
set_target_properties(colmap::colmap PROPERTIES
INTERFACE_INCLUDE_DIRECTORIES "${_IMPORT_PREFIX}/include"
INTERFACE_LINK_LIBRARIES ...
)
```
where `_IMPORT_PREFIX` is the installation prefix estimated relative to
the `colmap-targets.cmake` file. This fix the include headers errors.
In the CMake docs it's also recommended the use of the
[`GNUInstallDirs`](https://cmake.org/cmake/help/latest/module/GNUInstallDirs.html)
module which provides some variables to set the installation
directories. They are also used in Ceres (example
[here](https://github.com/ceres-solver/ceres-solver/blob/42475eec77c7b32a5ab88715577c55e78bef3cc6/internal/ceres/CMakeLists.txt#L321))
and in the proposed changes I also used these variables to set the
`DESTINATION` paths. With this module we have that `share/` is
equivalent to `CMAKE_INSTALL_DATAROOTDIR`, `include/` is equivalent to
`CMAKE_INSTALL_INCLUDEDIR`, `bin/` is equivalent to
`CMAKE_INSTALL_BINDIR`, etc.
---------
Co-authored-by: Johannes Schönberger <jsch@demuc.de>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The installation path for the COLMAP's include directory is currently set to
${CMAKE_INSTALL_PREFIX}/includein this line. This works perfectly if during the configuration the cache variable-DCMAKE_INSTALL_PREFIX=custom/path/is set. However, if we don't set it, and after building the project we want to use the--prefixoption of thecmake --installcommand to set a custom installation prefix, the include directory won't be set properly in thecolmap-targets.cmakefile. This means that the propertyINTERFACE_INCLUDE_DIRECTORIESof thecolmap::colmaptarget will be affected by the default value ofCMAKE_INSTALL_PREFIX(/usr/local) and it will be set to the absolute path/usr/local/includeon Linux whenCMAKE_INSTALL_PREFIXis not provided. This will produce include errors when using COLMAP as a library if we intended to set a different path with--prefix. In the CMake documentation it's also recommended to avoid absolute paths when setting the destination directories, because they are not compatible with the--prefixoption of thecmake --installcommand.Before the introduced changes, if we install COLMAP by using
cmake --install . --prefix custom/path(without setting-DCMAKE_INSTALL_PREFIX) we will get in thecolmap-targets.cmakefile:and after the proposed changes:
where
_IMPORT_PREFIXis the installation prefix estimated relative to thecolmap-targets.cmakefile. This fix the include headers errors.In the CMake docs it's also recommended the use of the
GNUInstallDirsmodule which provides some variables to set the installation directories. They are also used in Ceres (example here) and in the proposed changes I also used these variables to set theDESTINATIONpaths. With this module we have thatshare/is equivalent toCMAKE_INSTALL_DATAROOTDIR,include/is equivalent toCMAKE_INSTALL_INCLUDEDIR,bin/is equivalent toCMAKE_INSTALL_BINDIR, etc.