jhacsonmeza · GitHub

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.

Merged

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

@ahojnnes

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>

Merged

Read the original on github.com ↗