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

Add CMake option to force installation of path.d-related files in yarp_configure_external_installation #2444

Closed
traversaro opened this issue Dec 23, 2020 · 9 comments
Labels
Component: CMake Issue Type: Feat/Enh Req This issue requests some new feature or enhancement Resolution: Other

Comments

@traversaro
Copy link
Member

Is your feature request related to a problem? Please describe.
When packaging packages that depend on YARP and use yarp_configure_external_installation, it useful to be able to force the generation of the path.d files, as the packager may know that YARP and the downstream package (for example iCub) will be installed in the same prefix, even if the CMAKE_INSTALL_PREFIX used is not the same one of YARP during packaging.

Describe the solution you'd like
A CMake option for the downstream projects (something like YARP_FORCE_PATH_D_INSTALLATION) that permits to force the execution of the logic in https://github.com/robotology/yarp/blob/master/cmake/YarpInstallationHelpers.cmake#L230 . This can be done similarly to other options that are already defined by yarp_configure_external_installation, i.e. YARP_FORCE_DYNAMIC_PLUGINS .

Describe alternatives you've considered
An alternative that is in place in https://github.com/robotology/icub-packaging/blob/5768d6c1768a26c583ecb6866b67494fe37841c5/linux/build-iCub-packages.sh#L387 is to manually generate the iCub.ini file in the correct location, but this is a bit brittle as it requires some custom logic for each packaging system, and effectively duplicate the CMake logic already contained in the CMake macro.

@drdanz
Copy link
Member

drdanz commented Feb 2, 2021

I don't agree with this need, mostly because setting a different CMAKE_INSTALL_PREFIX, and then relocating the package is not right, and will cause lots of issues with lots of packages that are not fully relocatable.

Proper packaging systems, when building a CMake package, set the right CMAKE_INSTALL_PREFIX, and then make a temporary installation using DESTDIR.

The path.d mechanism is already abused, I don't think we should extend that.

@traversaro
Copy link
Member Author

I don't agree with this need, mostly because setting a different CMAKE_INSTALL_PREFIX, and then relocating the package is not right, and will cause lots of issues with lots of packages that are not fully relocatable.

In which sense it is not "right"? Making fully relocatable packages is explicitly recommended in the CMake documentation (https://cmake.org/cmake/help/latest/manual/cmake-packages.7.html#creating-relocatable-packages), and as far as I know all the our packages that use yarp_configure_external_installation are indeed fully relocatable.

Proper packaging systems, when building a CMake package, set the right CMAKE_INSTALL_PREFIX, and then make a temporary installation using DESTDIR.

Interesting, I think this would fix the specific problem raised in https://github.com/robotology/icub-packaging/blob/5768d6c1768a26c583ecb6866b67494fe37841c5/linux/build-iCub-packages.sh#L387, but unfortunately other packaging system (like vcpkg) also install packages in a temporary directory and then move the installation files to a different directory without using DESTDIR .

The path.d mechanism is already abused, I don't think we should extend that.

In which sense it is abused? In my experience most users just use YARP_DATA_DIRS for the existing problems in mixing path.d and YARP_DATA_DIRS.

@drdanz
Copy link
Member

drdanz commented Feb 6, 2021

Interesting, I think this would fix the specific problem raised in https://github.com/robotology/icub-packaging/blob/5768d6c1768a26c583ecb6866b67494fe37841c5/linux/build-iCub-packages.sh#L387, but unfortunately other packaging system (like vcpkg) also install packages in a temporary directory and then move the installation files to a different directory without using DESTDIR .

The use of DESTDIR has been around for ages (it was available also in automake/autoconf and it has been a recommended convention in manual makefiles), and as far as I know, most packaging system use it (for sure they are used to create .deb and .rpm and inside cpack)
Also I think (even though I'm not 100% sure) that both vcpkg and conda are using DESTDIR,

The path.d mechanism is already abused, I don't think we should extend that.

In which sense it is abused? In my experience most users just use YARP_DATA_DIRS for the existing problems in mixing path.d and YARP_DATA_DIRS.

For reference, the problem you describe is #336.

I'm referring mostly to #522 (path.d folder should not be searched using ResourceFinder) which is partially related, since ResourceFinder if I remember correctly, uses YARP_DATA_DIRS to find the path.d directory. The actual path that should be checked, for such variable, is, probably XDG_CONFIG_DIRS/YARP_CONFIG_DIRS which defaults to /etc/xdg/yarp (or maybe /etc/yarp? I'm not 100% sure about this [1, 2]), but this does not allow to install it in the install prefix, since /etc is not inside the prefix. The alternative explored in #522 is to make the path.d folder relative to the YARP_os library or to the executable in case of static library. I'm not very happy with this solution though, that's why I stopped investigating this.

Basically the path.d mechanism is not something that is supposed to be used by the developers, but something that should be configured by the system administrator or by the packager. For example, you should install your path.d file in a folder in your path, and the sysamin/packager decides whether to enable it or not by making a symbolic link to your file in /etc/yarp/path.d.

But it was decided to extend this in order to simplify the installation for the user, and to allow the developer to decide in which subfolder they wanted to put their files.

This complicates the things, since the path.d files are supposed to contain the absolute path, which makes them necessarily not-relocatable, and that, as you pointed out, is not recommended. The same reasoning applies with some changes to the ".ini" files that contains the plugin paths.

Also, due to the fact that it was decided that the path.d folder should is searched using ResourceFinder, it was decided, in order to limit the damage and confusion that this was creating, that the the path.d file would have been installed only if the package was installed in the same prefix as YARP, otherwise there would have been folders named share/yarp/config/path.d around, in unexpected places, and these would not have been considered, unless you extended the YARP_DATA_DIRS, but that doesn't make sense, because if you have to extend the variable, then you don't need the path.d.

For example, iCub would install

  • <prefix>/share/icub/... containing all files
  • <prefix>/share/yarp/config/path.d

If you have to extend YARP_DATA_DIRS in order to find the path.d file that points to <prefix>/share/icub/, you can just put <prefix>/share/icub/ in YARP_DATA_DIRS, and forget about the path.d.

Now, if we add a YARP_FORCE_PATH_D_INSTALLATION, I'm afraid this will lead to bad practices (i.e. not installing in temporary directories using DESTDIR, and it will be something that users will just start enabling because... "why not?"

I'm not sure if I managed to explain what I really mean by abused here... It's actually not path.d that was abused, it is the whole configuration system that was over complicated in order to allow the developer to do the system administrator job, with the goal of simplifying (too much in my opinion) the job of the person installing the software.

@traversaro
Copy link
Member Author

traversaro commented Feb 6, 2021

It's actually not path.d that was abused, it is the whole configuration system that was over complicated in order to allow the developer to do the system administrator job, with the goal of simplifying (too much in my opinion) the job of the person installing the software.

I see what you mean, then yes I totally agree that the existing system is quite complicated, what we are doing about this is effectively ignoring the path.d stuff, and always use YARP_DATA_DIRS.

Also I think (even though I'm not 100% sure) that both vcpkg and conda are using DESTDIR,

I am afraid what you link are just some extremely package-specific scripts , and do not reflect what is happening for most packages.

For example, for what regards vcpkg, the vcpkg_build_make is only used for autotools projects. For CMake projects, vcpkg_build_cmake/vcpkg_install_cmake is used instead, that do not use DESTDIR at all.
In particular, during the build process the CMAKE_INSTALL_PREFIX is set to vcpkg/packages/<port>_<triplet> directory for release builds and to vcpkg/packages/<port>_<triplet>/debug for debug builds (see https://github.com/microsoft/vcpkg/blob/2020.11/scripts/cmake/vcpkg_configure_cmake.cmake#L272).
After the build process has been completed, then the contents of the vcpkg/packages/<port>_<triplet> directory are copied manually (without any DESTDIR) in the vcpkg/installed/<triplet> directory, that is the location in which the packages are meant to be consumed. Note that the packages could be further relocated in a different directory if the binary cache is used (https://vcpkg.readthedocs.io/en/latest/specifications/binarycaching/).

On the other hand, conda is less afraid of having absolute hardcoded installation path because those are automatically handled by conda itself, see https://docs.conda.io/projects/conda-build/en/latest/resources/make-relocatable.html . However, fortunately in conda when building packages the CMAKE_INSTALL_PREFIX is the same of the CMAKE_PREFIX_PATH, so the existing path.d machinery should work fine, differently from vcpkg in which CMAKE_INSTALL_PREFIX is vcpkg/packages/<port>_<triplet> and CMAKE_PREFIX_PATHis vcpkg/installed/<triplet>.

Interesting, I think this would fix the specific problem raised in https://github.com/robotology/icub-packaging/blob/5768d6c1768a26c583ecb6866b67494fe37841c5/linux/build-iCub-packages.sh#L387, but unfortunately other packaging system (like vcpkg) also install packages in a temporary directory and then move the installation files to a different directory without using DESTDIR .

The use of DESTDIR has been around for ages (it was available also in automake/autoconf and it has been a recommended convention in manual makefiles), and as far as I know, most packaging system use it (for sure they are used to create .deb and .rpm and inside cpack)

As far as I know, DESTDIR is not used at all if you use CPack to generate a QtIFW installer in which the installation prefix can be specified by the user when it installs the QtIFW installer. For this reason, if you don't make your CMake package relocatable but you instead rely on DESTDIR, you will have problems if you try to include your CMake project as part of a QtIFW installer on Windows.

@drdanz
Copy link
Member

drdanz commented Feb 6, 2021

As far as I know, DESTDIR is not used at all if you use CPack to generate a QtIFW installer in which the installation prefix can be specified by the user when it installs the QtIFW installer. For this reason, if you don't make your CMake package relocatable but you instead rely on DESTDIR, you will have problems if you try to include your CMake project as part of a QtIFW installer on Windows.

It is used by CPack, since when you run CPack (for any generator) it creates an installation inside your build directory (using CMAKE_BUILD_DIR + _CPack_Packages + target operating system + package type + package name as DESTDIR, but keeping the configuration from CMAKE_INSTALL_PREFIX). This installation is then packaged in different ways, depending on the packaging/installation framework, but internally, all non-relocatable files contain the CMAKE_INSTALL_PREFIX as prefix. QtIFW and other installation frameworks allow to change the installation path, but this relocation has nothing to do with CPack. This is why our windows packages are forced to run some scripts to fix the paths during the installation.
This would be of course not needed for fully relocatable packages, but path.d and plugin path files contain the full path, and there is not much that can be done about this without changing a lot of stuff in the configuration system.

I see what you mean, then yes I totally agree that the existing system is quite complicated, what we are doing about this is effectively ignoring the path.d stuff, and always use YARP_DATA_DIRS.

My personal advice for the superbuild is to use path.d, or eventually XDG_DATA_DIRS. YARP_DATA_DIRS is a mechanism that should be used only when you don't install all the software, and need to pass around paths that are scattered in different build directories.

@traversaro
Copy link
Member Author

It is used by CPack,

Ah, I got what you mean now, sorry. To recap if I understood correctly, DESTDIR is used in a way to avoid the problem related to path.d-related files generation, but it can't help in making the package relocatable.

I see what you mean, then yes I totally agree that the existing system is quite complicated, what we are doing about this is effectively ignoring the path.d stuff, and always use YARP_DATA_DIRS.

My personal advice for the superbuild is to use path.d, or eventually XDG_DATA_DIRS. YARP_DATA_DIRS is a mechanism that should be used only when you don't install all the software, and need to pass around paths that are scattered in different build directories.

We tried to use path.d in the past, but unfortunately due to #336 this is not feasible. A common workflow for users is to use the robotology-superbuild for install all the robotology packages that they want to use (using it as a sort of "distribution") and then building manually their project, outside of the superbuild but using the superbuild-provided libraries. If we just used path.d, as soon as the user would append a value YARP_DATA_DIRS to point to its own directories, finding the configuration files of YARP, iCub and all other YARP-based projects contained in the superbuild would fail, unless their directory are added as well to YARP_DATA_DIRS (and they are not a few of them, check https://github.com/robotology/robotology-superbuild/blob/master/cmake/template/setup.sh.in for YARP_DATA_DIRS occurence). To avoid this problem, it is just simpler to directly always use YARP_DATA_DIRS.

@traversaro
Copy link
Member Author

traversaro commented Feb 8, 2021

However, if you think what is proposed in the issue increase the complexity too much, we can close the issue and manually path.d files in the situations in which it is necessary. At a first glance, this may be necessary if someone wants to build vcpkg recipe for a YARP-dependent project that installs conf files (see robotology/robotology-vcpkg-ports#15), but I don't think anyone is planning to work on this.

@drdanz
Copy link
Member

drdanz commented Feb 17, 2021

It would be pretty easy to write the files manually anyway...
I'll close this, but if we realise that this is actually needed somewhere, we can open it again...

@traversaro
Copy link
Member Author

Revisiting this, the actual problem is not to either generate automatically the file or to write it manually (like in https://github.com/robotology-legacy/icub-packaging/blob/5768d6c1768a26c583ecb6866b67494fe37841c5/linux/build-iCub-packages.sh#L387), but the fact that the file installed in the path.d folder contains the absolute path to the /usr/share/iCub directory or similar, and in general if you want to create a relocatable package you do not know where the package is going to be installed. A possible solution is to extend YARP to also support having a relative_path defined in the path.d ini files, in the way that we do not need to hardcode the installation directory in the .ini file, a bit like it was proposed in #2445 for the .ini files generated by yarp_configure_plugins_installation CMake macro.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: CMake Issue Type: Feat/Enh Req This issue requests some new feature or enhancement Resolution: Other
Projects
None yet
Development

No branches or pull requests

2 participants