added 2 commits
December 10, 2024 11:43Merged
ahojnnes added a commit that referenced this pull request
Dec 10, 2024Currently, 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.
HernandoR pushed a commit to HernandoR/colmap that referenced this pull request
Dec 30, 2024Currently, 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, 2024The 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