NixOS / nixpkgs

Nix Packages collection & NixOS
MIT License
17.35k stars 13.59k forks source link

CMake incorrect absolute include/lib paths tracking issue #144170

Open nh2 opened 2 years ago

nh2 commented 2 years ago

There are various issues where cmake does not work with nix store paths given as prefixes, generating invalid paths into pkg-config .pc files and others.

This issue is to track such cases and their workarounds, so that we can ideally find a general solution, and to figure out whether it's the cmake usage in the upstream packages that's wrong, or something else.

If you find such a case, please post here.

The wrong paths look like this:

NIX_PATH=nixpkgs=. nix-shell -p libyamlcpp -p pkg-config --run 'pkg-config --cflags yaml-cpp'
-I/nix/store/bq106wng1cqk8r4y1y7yh5h7cz49jxpv-libyaml-cpp-0.7.0//nix/store/bq106wng1cqk8r4y1y7yh5h7cz49jxpv-libyaml-cpp-0.7.0/include

Note the concatenation of a store path /nix/store/bq106wng1cqk8r4y1y7yh5h7cz49jxpv-libyaml-cpp-0.7.0/ and the same store path again, and then /include.

Cases

Include/library paths:

.cmake paths (see https://github.com/NixOS/nixpkgs/issues/144170#issuecomment-960302417):

nh2 commented 2 years ago

CC from other issues: @andir @j0sh @dtzWill @bennofs @cole-h @jtojnar

nh2 commented 2 years ago

Linking from https://github.com/NixOS/nixpkgs/pull/81091#issuecomment-593095785 the upstream issue:

https://gitlab.kitware.com/cmake/cmake/-/issues/19568

@jtojnar Do you understand the upstream MRs that closed that issue?

r-burns commented 2 years ago

The cmake project is more general but has some similar issues: https://github.com/NixOS/nixpkgs/projects/32

r-burns commented 2 years ago

The usptream cmake feature that closed that issue is cmake_path (available starting with cmake 3.20)

jtojnar commented 2 years ago

The upstream PR basically allows us to simplify the example code in https://github.com/jtojnar/cmake-snips#concatenating-paths-when-building-pkg-config-files to:

cmake_path(APPEND libdir_for_pc_file "\${prefix}" "${CMAKE_INSTALL_LIBDIR}")

configure_file(
  "${PROJECT_SOURCE_DIR}/my-project.pc.in"
  "${PROJECT_BINARY_DIR}/my-project.pc"
  @ONLY)

The downside, as observed above, is that it is only available in CMake 3.20, while projects often have cmake_minimum_required(VERSION 3.x) where x is close to 0. Not sure if they just did not have time to modernize their build systems (modern CMake builds should be based around targets but projects often still use CMake 2 style systems) or they intentionally try to support ancient distros (for example, Debian Jessie whose support ended in June 2020 is stuck on 3.0).

For that reason, using the JoinPaths module from cmake-snips might be still needed for a long time.

The cmake_path also has the disadvantage that the paths are in the format of the build platform (I suspect the docs are using host platform where they mean build platform and target platform where they mean host plaform in GNU terminology):

Note: The cmake_path command handles paths in the format of the build system (i.e. the host platform), not the target system. When cross-compiling, if the path contains elements that are not representable on the host platform (e.g. a drive letter when the host is not Windows), the results will be unpredictable.

So it might accidentally preserve drive letter ( root-name in CMake terminology) when building on Windows for Unix host – for example, when the following project is configured with -DCMAKE_INSTALL_LIBDIR=/usr/lib (and CMAKE_INSTALL_PREFIX defaults to c:/Program Files/${PROJECT_NAME}", ${pkglibdir} will contain c:/usr/lib/my-library.

project(my-library)
include(GNUInstallDirs)
cmake_path(APPEND pkglibdir ${CMAKE_INSTALL_PREFIX} ${CMAKE_INSTALL_LIBDIR} ${PROJECT_NAME})

Or the other way round, on Linux, it will happily append c:/ to an existing path.

Though, to be fair, the function from cmake-snips avoided the first issue by only supporting Unix paths and suffered from the second issue as well.

nh2 commented 2 years ago

I also found a similar issue in paths in .cmake files:

https://github.com/NixOS/nixpkgs/pull/144561#issuecomment-960296043

I added these to the issue description.

r-burns commented 2 years ago

The bpp-* issues are apparently due to them setting their INTERFACE_INCLUDE_DIRECTORIES to $<INSTALL_INTERFACE:$<INSTALL_PREFIX>/${CMAKE_INSTALL_INCLUDEDIR}>, once again appending a not-necessarily-relative directory to the install prefix. This is unnecessary anyway; a relative INSTALL_INTERFACE include dir is already interpreted as relative to the install prefix so $<INSTALL_INTERFACE:${CMAKE_INSTALL_INCLUDEDIR}> would be more correct.

Artturin commented 2 years ago

https://github.com/NixOS/nixpkgs/pull/181875

r-burns commented 2 years ago

https://github.com/NixOS/nixpkgs/issues/180054

Artturin commented 2 years ago

https://github.com/NixOS/nixpkgs/pull/172347 adds a hook to catch broken .pc files

StillerHarpo commented 1 year ago

https://github.com/NixOS/nixpkgs/pull/206265

bcdarwin commented 1 year ago

207042

bcdarwin commented 1 year ago

210399

jeff-hykin commented 1 year ago

For others running into this, slapping this into the derivation code worked for my issue:

  installPhase =  ''
    ${pkgs.sd}/bin/sd --string-mode '$${"{prefix}//nix/store"}' '/nix/store' **/*.pc
  '';

It should be pretty robust against false-positives, although it only works when the prefix is literally "prefix"

2xsaiko commented 1 year ago

I encountered one of these when setting up installing CMake config for one of my own projects today. That project is not in nixpkgs but might as well describe it here in case anyone else has to deal with something similar:

It happens for libraries (like this one) that install headers via FILE_SET HEADERS and also install exported targets (the CMake-native mechanism, not pkgconfig). The generated *Targets.cmake file will have a section setting up the file set, which will look something like this, with the wrong paths (should be FILES "${_IMPORT_PREFIX}/include/foo.h"):


if(NOT CMAKE_VERSION VERSION_LESS "3.23.0")
  target_sources(foo
    INTERFACE
      FILE_SET "HEADERS"
      # ...
      FILES "${_IMPORT_PREFIX}//nix/store/.../include/foo.h"
  )
endif()

A workaround I found is to just set CMAKE_INSTALL_INCLUDEDIR to be relative to CMAKE_INSTALL_PREFIX if it's an absolute path. (This might actually be a bug in how CMake generates the file set section of the targets file, since it should probably leave away the ${_IMPORT_PREFIX}/ if the path that comes after is absolute, but I might just as well be doing something wrong that I didn't catch, since I don't know a lot about how this works.)

chayleaf commented 1 year ago

Found a couple more instances of similar issues just out of the files that weren't garbage collected on my laptop:

/nix/store/hlz9dcyn78bdyf7r67phcqdachgi0h9v-ruby2.7.8-msgpack-1.5.1/nix-support/setup-hook:
addToSearchPath RUBYLIB /nix/store/hlz9dcyn78bdyf7r67phcqdachgi0h9v-ruby2.7.8-msgpack-1.5.1/lib/ruby/gems/2.7.0/gems/msgpack-1.5.1//nix/store/hlz9dcyn78bdyf7r67phcqdachgi0h9v-ruby2.7.8-msgpack-1.5.1/lib/ruby/gems/2.7.0/extensions/x86_64-linux/2.7.0/msgpack-1.5.1
/nix/store/wwf32hnxdiagl2zphjbvgwxnqq3h4f0b-opencolorio-2.2.0/share/ocio/setup_ocio.sh:

export DYLD_LIBRARY_PATH="/nix/store/wwf32hnxdiagl2zphjbvgwxnqq3h4f0b-opencolorio-2.2.0//nix/store/wwf32hnxdiagl2zphjbvgwxnqq3h4f0b-opencolorio-2.2.0/lib:${DYLD_LIBRARY_PATH}"
export LD_LIBRARY_PATH="/nix/store/wwf32hnxdiagl2zphjbvgwxnqq3h4f0b-opencolorio-2.2.0//nix/store/wwf32hnxdiagl2zphjbvgwxnqq3h4f0b-opencolorio-2.2.0/lib:${LD_LIBRARY_PATH}"
export PATH="/nix/store/wwf32hnxdiagl2zphjbvgwxnqq3h4f0b-opencolorio-2.2.0//nix/store/wwf32hnxdiagl2zphjbvgwxnqq3h4f0b-opencolorio-2.2.0/bin:${PATH}"
export PYTHONPATH="/nix/store/wwf32hnxdiagl2zphjbvgwxnqq3h4f0b-opencolorio-2.2.0//nix/store/wwf32hnxdiagl2zphjbvgwxnqq3h4f0b-opencolorio-2.2.0/lib/python3.10/site-packages:${PYTHONPATH}"
/nix/store/5l495fv1y253z1xqyvg3xf2i5i9psqbs-frei0r-plugins-1.7.0/lib/pkgconfig/frei0r.pc:
libdir=/nix/store/5l495fv1y253z1xqyvg3xf2i5i9psqbs-frei0r-plugins-1.7.0//nix/store/5l495fv1y253z1xqyvg3xf2i5i9psqbs-frei0r-plugins-1.7.0/lib

not sure about this one:

/nix/store/zzab1mabj0xjsq05py3n1lldycwdsxad-rocm-comgr-5.4.4/lib/cmake/amd_comgr/amd_comgr-config.cmake:
include("${AMD_COMGR_PREFIX}//nix/store/zzab1mabj0xjsq05py3n1lldycwdsxad-rocm-comgr-5.4.4/lib/cmake/amd_comgr/amd_comgr-targets.cmake")
/nix/store/vv3vq7fnkbddkln85xm3pl87dbxs9f0x-rocm-opencl-runtime-5.4.4/opencl/include/CL/<header name>:
#include "../../..//nix/store/vv3vq7fnkbddkln85xm3pl87dbxs9f0x-rocm-opencl-runtime-5.4.4/include/CL/<header name>
/nix/store/yavqyq3l299avzqagx64gsf9l1pc2sxy-opencolorio-2.2.0/share/ocio/setup_ocio.sh:
export DYLD_LIBRARY_PATH="/nix/store/yavqyq3l299avzqagx64gsf9l1pc2sxy-opencolorio-2.2.0//nix/store/yavqyq3l299avzqagx64gsf9l1pc2sxy-opencolorio-2.2.0/lib:${DYLD_LIBRARY_PATH}"

from what I've seen, most false positives were either "file://" URLs or cases where the prefix was indeed prepended to an absolute path, but was empty instead of "/nix/store/...". Would it make sense to further increase the checking to all text files? This would be quite a far reaching change

imincik commented 3 months ago

Looks like we have this problem with libspatialindex as well.

2xsaiko commented 3 months ago

FWIW this is what I do in one of my projects to get it to output correctly. Should be useful generally, but needs to be inserted into the toplevel CMakeLists so it's probably not something that can be autopatched.

From https://git.dblsaiko.net/nucom/tree/CMakeLists.txt

# This fixes generated target file to not include wrong paths for FILE_SET
# HEADERS when CMAKE_INSTALL_INCLUDEDIR is absolute (i.e. when building with
# Nix)
cmake_path(IS_ABSOLUTE CMAKE_INSTALL_INCLUDEDIR _is_includedir_absolute)
if (_is_includedir_absolute)
    # cmake_path(RELATIVE_PATH ...) is supposed to be the replacement for this,
    # but I couldn't get it to give any reasonable results.
    file(RELATIVE_PATH CMAKE_INSTALL_INCLUDEDIR ${CMAKE_INSTALL_PREFIX} ${CMAKE_INSTALL_INCLUDEDIR})
endif ()
unset(_is_includedir_absolute)
mrexodia commented 1 month ago

In my opinion this is a bug in Nix. The table in the documentation clearly shows the CMAKE_INSTALL_XXXDIR variables are supposed to be relative paths:

image

The GNUInstallDirs documentation confirms this: https://cmake.org/cmake/help/latest/module/GNUInstallDirs.html#module:GNUInstallDirs:

image

The PR that referred this issue (https://github.com/capstone-engine/capstone/pull/2134) actually broke the fundamental assumption that CMake install prefixes are relocatable (at least they are supposed to be). Documentation around CMake packages is rather lacking, but also this page explains that all these paths are relative to the CMAKE_INSTALL_PREFIX: https://cmake.org/cmake/help/latest/guide/importing-exporting/index.html

This variable can even be set at install-time:

cmake --install build --prefix build/install

So all uses of the value of CMAKE_INSTALL_PREFIX at configure-time are almost guaranteed to be incorrect (https://cmake.org/cmake/help/latest/variable/CMAKE_INSTALL_PREFIX.html).

jtojnar commented 1 month ago

In my opinion this is a bug in Nix.

You are mistaken.

The table in the documentation clearly shows the CMAKE_INSTALL_XXXDIR variables are supposed to be relative paths:

The table just lists the defaults.

The GNUInstallDirs documentation confirms this: cmake.org/cmake/help/latest/module/GNUInstallDirs.html#module:GNUInstallDirs:

The next paragraph to the one you highlighted explicitly says absolute paths are allowed.

The PR that referred this issue (https://github.com/capstone-engine/capstone/pull/2134) actually broke the fundamental assumption that CMake install prefixes are relocatable (at least they are supposed to be).

There is nothing fundamental about that assumption. Sometimes relocatability is just is not possible. For example, the pkg-config file will always have an absolute prefix hardcoded.

However, you are right that using CMAKE_INSTALL_FULL_<dir> form is suboptimal. The proper form for pkg-config is joining the non-_full_ variable to ${prefix} literal string as described in https://github.com/jtojnar/cmake-snips#concatenating-paths-when-building-pkg-config-files

But upstream projects often do not care about relocatability so they opt for a simpler build script using _full_. Though in the PR you linked, it does not look like the proper solution was discussed.

mrexodia commented 1 month ago

You are mistaken.

Forgive my ignorance, I mostly used Windows and sometimes Linux but I never used Nix. Why is there a need to specify an absolute path for CMAKE_INSTALL_XXDIR in Nix? Are the include and lib paths ever completely separated from each other on the filesystem? And if so, isn't there a relative form that would achieve the same result? My understanding is that Nix installs every package version in its own 'store' based on the package hash/name/version (like /nix/store/bq106wng1cqk8r4y1y7yh5h7cz49jxpv-libyaml-cpp-0.7.0/), so wouldn't a path relative to the (absolute) ${prefix} also work?

The next paragraph to the one you highlighted explicitly says absolute paths are allowed.

The documentation specifically says that absolute paths are allowed, but discouraged because it does not work with other CMake features:

While absolute paths are allowed, they are not recommended as they do not work with the cmake --install command's --prefix option, or with the cpack installer generators. In particular, there is no need to make paths absolute by prepending CMAKE_INSTALL_PREFIX; this prefix is used by default if the DESTINATION is a relative path.

To be clear, I do not particularly care about what is in the pkg-config file. My issue is with the CMake install prefix (eg <lib>Config.cmake), which are supposed to be fully relocatable. Does Nix care about these at all, or is pkg-config always used?

However, you are right that using CMAKE_INSTALLFULL

form is suboptimal. The proper form for pkg-config is joining the non-full variable to ${prefix} literal string as described in https://github.com/jtojnar/cmake-snips#concatenating-paths-when-building-pkg-config-files

Thanks for the resource, I will try to implement a proper fix in the capstone repository.

jtojnar commented 1 month ago

Why is there a need to specify an absolute path for CMAKE_INSTALL_XXDIR in Nix? Are the include and lib paths ever completely separated from each other on the filesystem?

Yes, that is the reason. Here is an example from another package:

-DCMAKE_INSTALL_INCLUDEDIR=/nix/store/phdxb9322csfsy113naiqm04ajdrrpwx-jsoncpp-1.9.5-dev/include
-DCMAKE_INSTALL_PREFIX=/nix/store/14av31ps0jh9m1lwx949azzc4a611bsh-jsoncpp-1.9.5

My understanding is that Nix installs every package version in its own 'store' based on the package hash/name/version (like /nix/store/bq106wng1cqk8r4y1y7yh5h7cz49jxpv-libyaml-cpp-0.7.0/)

Not just one store path – with multiple outputs one package can have multiple store paths. This allows Nix to treat parts of a package that are large and used in different context (e.g. headers, docs) independently, while preserving the simple dependency heuristic of grepping for hash in the build output. The main benefit is not having to install aforementioned development files at runtime, when they are not used.

And if so, isn't there a relative form that would achieve the same result? […] so wouldn't a path relative to the (absolute) ${prefix} also work?

Unfortunately, no:

  1. Nix requires dependency relation to form a sort of directed acyclic graph (no loops allowed except for self-loops) .
  2. We would need dev output (containing e.g. headers, pkg-config file and *Config.cmake files) to depend on out output (containing the shared library), not the other way around.
    • Otherwise you could not have out without dev at runtime.
  3. CMake does base relative paths on CMAKE_INSTALL_PREFIX so that needs to be absolute.
    • But because of (2), we would want something like -DCMAKE_INSTALL_PREFIX=${CMAKE_INCLUDE_DIR}/../../14av31ps0jh9m1lwx949azzc4a611bsh-jsoncpp-1.9.5` but that is not really possible.
  4. In fact, GNUInstallDirs does not have the concept of pkgconfigdir so we are moving the .pc files to dev output ourselves, without the knowledge of CMake.
    • To make the relative paths work properly, we would need CMake gain native support for multiple prefixes and options to specify expected dependency relation between them. This would be a moderately complex change in and of itself, from which only few projects like Nix and Guix would benefit. And even if we convinced CMake maintainers this is worthwhile and implemented that, we would need wait for the CMake version to be widely available, and then port all CMake-based projects to it. That looks to me like at least a decade of work, when some projects are still struggling to use CMake 3 features.

Alternately, we could implement it as a third-party module and promote using that. This is actually what I did with the join_paths since, until recently, there was no such function in CMake. Thankfully CMake ≥ 3.20 contains path relativization function. But it would be much larger beast not based on any existing standard and thus significantly harder to push.

The documentation specifically says that absolute paths are allowed, but discouraged because it does not work with other CMake features:

Right – but those features are irrelevant for us. We already know prefix at configure time and do not use cpack. Whereas absolute CMAKE_INCLUDE_DIR solves an issue for us.

To be clear, I do not particularly care about what is in the pkg-config file. My issue is with the CMake install prefix (eg <lib>Config.cmake), which are supposed to be fully relocatable. Does Nix care about these at all, or is pkg-config always used?

Some packages in Nixpkgs do indeed use *Config.cmake files to find the package headers and shared libraries.

But I think most people have currently given up on relocatability since that does not work in practice for most packages anyway:

Don’t get me wrong, full relocatability would be nice since it would allow us to relocate /nix/store itself, which is desirable for users that do not have write permissions to / but I am afraid achieving it would require ecosystem-wide toolchain changes for which there are currently no resources.

mrexodia commented 1 month ago

Thanks for your detailed explanation! I understand that Nix basically has multiple prefixes and there is no proper CMake support for this, so the absolute CMAKE_INSTALL_FULL_LIBDIR is suggested as a workaround to make things work in Nix.

Alternately, we could implement it as a third-party module and promote using that. This is actually what I did with the join_paths since, until recently, there was no such function in CMake. Thankfully CMake ≥ 3.20 contains path relativization function. But it would be much larger beast not based on any existing standard and thus significantly harder to push.

For me the big problem here is that the proposed 'solution' (to use CMAKE_INSTALL_FULL_*DIR) in the Nix hook is actually wrong, since it breaks relocatable packages that work on every other OS just fine. In my experience, project maintainers do not understand CMake and they do not want to either. This means that patches are accepted blindly, and/or they just copy from examples/previous projects.

Considering this, I think it is extremely important that a 'canonical' example solution is created and this is what the hooks warnings actually link to. This example should work on Nix with absolute paths, but not break the much more common scenario where relative paths are used. Your example is a start, but it only deals with pkg-config and not CMake config packages.

My solution for the capstone project (which works since at least CMake 3.0)

```cmake # Support absolute installation paths (discussion: https://github.com/NixOS/nixpkgs/issues/144170) if(IS_ABSOLUTE ${CMAKE_INSTALL_LIBDIR}) set(CAPSTONE_PKGCONFIG_INSTALL_LIBDIR ${CMAKE_INSTALL_LIBDIR}) set(CAPSTONE_CMAKE_INSTALL_LIBDIR ${CMAKE_INSTALL_LIBDIR}) else() set(CAPSTONE_PKGCONFIG_INSTALL_LIBDIR "\${prefix}/${CMAKE_INSTALL_LIBDIR}") set(CAPSTONE_CMAKE_INSTALL_LIBDIR "\${PACKAGE_PREFIX_DIR}/${CMAKE_INSTALL_LIBDIR}") endif() if(IS_ABSOLUTE ${CMAKE_INSTALL_INCLUDEDIR}) set(CAPSTONE_PKGCONFIG_INSTALL_INCLUDEDIR ${CMAKE_INSTALL_INCLUDEDIR}) set(CAPSTONE_CMAKE_INSTALL_INCLUDEDIR ${CMAKE_INSTALL_INCLUDEDIR}) else() set(CAPSTONE_PKGCONFIG_INSTALL_INCLUDEDIR "\${prefix}/${CMAKE_INSTALL_INCLUDEDIR}") set(CAPSTONE_CMAKE_INSTALL_INCLUDEDIR "\${PACKAGE_PREFIX_DIR}/${CMAKE_INSTALL_INCLUDEDIR}") endif() ``` And then these variables are expanded in the `capstone.pc.in` like this: ``` prefix=@CMAKE_INSTALL_PREFIX@ exec_prefix=${prefix} libdir=@CAPSTONE_PKGCONFIG_INSTALL_LIBDIR@ includedir=@CAPSTONE_PKGCONFIG_INSTALL_INCLUDEDIR@ ``` And `capstone-config.cmake.in`: ``` @PACKAGE_INIT@ set_and_check(capstone_INCLUDE_DIR "@CAPSTONE_CMAKE_INSTALL_INCLUDEDIR@") set_and_check(capstone_LIB_DIR "@CAPSTONE_CMAKE_INSTALL_LIBDIR@") include("${CMAKE_CURRENT_LIST_DIR}/capstone-targets.cmake") ``` This is somewhat similar to the solution by @2xsaiko (except that solution would actually break Nix packages if I understand your comments about the split `-dev` package correctly). Since CMake 3.7 there is also the [`CMAKE_INSTALL_PREFIX_INITIALIZED_TO_DEFAULT`](https://cmake.org/cmake/help/latest/variable/CMAKE_INSTALL_PREFIX_INITIALIZED_TO_DEFAULT.html), which could also be used to switch between relocatable and prefix-known-at-configure-time modes.

But I think most people have currently given up on relocatability since that does not work in practice for most packages anyway

This might be the case for pkg-config projects (although https://www.bassi.io/articles/2018/03/15/pkg-config-and-paths/ seems to suggest otherwise). But many projects using CMake's <package>-config.cmake mechanism are fully relocatable per default (LLVM, Z3, fmt, spdlog, gflags, glog, googletest).

I think you are mixing relocatable in Nix and relocatable on all other operating systems though. In practice this is working just fine (with some $ORIGIN rpath tricks for shared libraries on Linux).

There is often no way (or at least not a portable one) to get current path to the resource being executed, which makes using relative paths impossible.

The meaning of 'portable' is also overloaded here, but using std::filesystem on argv[0] to get the absolute path works just fine on all operating systems for me. For shared libraries this is a bit different and hardcoded data directories are definitely an issue, but I think this is a solvable problem if the package maintainer actually cares about relocatable packages.

Don’t get me wrong, full relocatability would be nice since it would allow us to relocate /nix/store itself

If I understand you correctly this will not ever be possible, because there is no way to relatively point to the right libraries if there is a separate -dev folder?

In fact, GNUInstallDirs does not have the concept of pkgconfigdir so we are moving the .pc files to dev output ourselves, without the knowledge of CMake.

Wouldn't it be an idea to let Nix parser and rewrite the .pc files then, instead of using an absolute path for the LIBDIR? If this splitting is done by Nix anyway, why not go all the way? You could do CMAKE_INSTALL_INCLUDEDIR=include-nix-<hash> (and similar for other dirs) and then reconstruct the package as-needed. You have the nix-<hash> you can grep for so it might be technically possible to automatically adjust packages using relative paths (since you know the absolute path to the install prefix + absolute paths you want the include/lib/whatever dir to be in)?