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

Remove usage of NO_CMAKE_FIND_ROOT_PATH to enable cross compilation. #1145

Open
wants to merge 1 commit into
base: noetic-devel
Choose a base branch
from

Conversation

windelbouwman
Copy link

It's not fully clear to me why NO_CMAKE_FIND_ROOT_PATH was used. I think it was done for consistency, since it's used everywhere in the same way. I believe this option is not needed, and even should not be there.

The option not being used has these advantages:

  • Less code in the cmake files
  • Cross compilation works as expected when using a sysroot folder

@meyerj
Copy link
Contributor

meyerj commented Jul 11, 2022

I agree with @windelbouwman that the NO_CMAKE_FIND_ROOT_PATH flags should probably be removed: Without, cross-compilation or staged installation of a full workspace would work more or less out of the box, by setting the DESTDIR environment variable and the CMAKE_FIND_ROOT_PATH CMake variable to the location of the staged install tree. Unless someone explicitly sets CMAKE_FIND_ROOT_PATH, adding NO_CMAKE_FIND_ROOT_PATH should have no effect anyway. And if it is set, than finding packages, libraries, headers etc. at that location is exactly what we want, even though CMAKE_PREFIX_PATH or <pkg>_DIR may say something different.

From the documentation of find_package():

The CMake variable CMAKE_FIND_ROOT_PATH specifies one or more directories to be prepended to all other search directories. This effectively "re-roots" the entire search under given locations. Paths which are descendants of the CMAKE_STAGING_PREFIX are excluded from this re-rooting, because that variable is always a path on the host system. By default the CMAKE_FIND_ROOT_PATH is empty.

The CMAKE_SYSROOT variable can also be used to specify exactly one directory to use as a prefix. Setting CMAKE_SYSROOT also has other effects. See the documentation for that variable for more.

These variables are especially useful when cross-compiling to point to the root directory of the target environment and CMake will search there too.

So why would one want to explicitly disable that behavior?

But the occurence of NO_CMAKE_FIND_ROOT_PATH in pkgConfig.cmake.in is not the only one, and also others in catkinConfig.cmake would need to be removed. Next to that, in some places catkin or also other packages' CMake code (e.g. genmsg) expect to find files at paths below an entry of CMAKE_PREFIX_PATH or CATKIN_WORKSPACES, without using find_*() commands and without also checking the same paths prefixed by CMAKE_FIND_ROOT_PATH. That is doable with a few more patches, for example by including the prefixed paths to CATKIN_WORKSPACES if those directories exist, and keep CMAKE_PREFIX_PATH as a list of underlays without the staging prefix, that is used by CMake internally according to the docs and for generating setup and config files.

There is no problem with the current version of catkin to build and install package by package with DESTDIR, and to install each package individually before building the next one. In this case catkin and CMake can find already installed packages at their final location. But in order to build a full workspace at once, each package needs to be able to find its dependencies that have been installed to DESTDIR before. And even if they can be found, because CMAKE_PREFIX_PATH or _CATKIN_SETUP_DIR is set such that they include the staging dir, those path components must be stripped again from all generated files. If I only set DESTDIR with catkin_make_isolated the build succeeds (likely thanks to , but after copying the resulting install-space to the final location and sourcing setup.sh, CMAKE_PREFIX_PATH and other environment variables contain both, the final locations and the original staging locations.

Or does anyone have a working solution without the need to patch catkin? Older issues and especially #490 suggest that this use case should already be supported. But I could not get it to work without setting CMAKE_FIND_ROOT_PATH and without still finding traces of the DESTDIR in the installed files, especially in <prefix>/_setup_util.py and generated <pkg>Config.cmake. I remember that it was already working for me in the past (#553), but maybe I did not pay attention to the content of generated files back then.

CMAKE_STAGING_PREFIX may be another approach to divert the install space and affect find_*() commands. It is not clear to me yet to what degree it differs in its intended usage from DESTDIR and CMAKE_FIND_ROOT_PATH. Both seem to have similar goals. If only CMAKE_STAGING_PREFIX is set, but not DESTDIR, some files are still generated in CMAKE_INSTALL_PREFIX directly.

Is there still interest in such a patch, given the that this pull request is already open for more than a year and the last release was in 2021? I got quite far with only minimal changes, basically those removals of NO_CMAKE_FIND_ROOT_PATH and checking CMAKE_FIND_ROOT_PATH when populating CATKIN_WORKSPACES from CMAKE_PREFIX_PATH here. But it would require some time to finalize and document the patch, and eventually consider other variables like CMAKE_SYSROOT, too.

I personally do not have the use case of cross-compiling in mind, as mentioned in the title of this PR. Instead I want to build full workspace binary distributions for later installation, eventually in multiple stages without mixing the new packages with others already installed at the final destination using docker multi-stage builds. I assume that the requirements are very similar, and that both use cases would work if catkin could distinguish cleanly between paths used at build time (e.g. DESTDIR, CMAKE_FIND_ROOT_PATH, CATKIN_WORKSPACES, CMAKE_SYSROOT, ...) and paths where the packages will later be installed and found (CMAKE_PREFIX_PATH, CMAKE_INSTALL_PREFIX, ...).

Related issues:

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