-
-
Notifications
You must be signed in to change notification settings - Fork 14.5k
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
CMake incorrect absolute include/lib paths tracking issue #144170
Comments
Fixes a "double prefix" issue, where parts of the include files for hhvm where located in `$out/$out/include` instead of `$out/include`.
Linking from #81091 (comment) the upstream issue: https://gitlab.kitware.com/cmake/cmake/-/issues/19568 @jtojnar Do you understand the upstream MRs that closed that issue? |
The cmake project is more general but has some similar issues: https://github.com/NixOS/nixpkgs/projects/32 |
The usptream cmake feature that closed that issue is cmake_path (available starting with cmake 3.20) |
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 For that reason, using the The
So it might accidentally preserve drive letter ( 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 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. |
I also found a similar issue in paths in I added these to the issue description. |
The bpp-* issues are apparently due to them setting their INTERFACE_INCLUDE_DIRECTORIES to |
#172347 adds a hook to catch broken |
Fixes NixOS#192617. The breakage is introduced by a glslang update. Now `${glslang}/lib/cmake/OSDependentTargets.cmake` points to `${glslang}/share/glslang/glslang-targets.cmake`. The latter requires the `SPIRV-Tools-opt` target. So the two lines in patch should be moved before `OSDependentTargets.cmake`. This also fixes a breakage introduced by cmake described in NixOS#144170.
Fixes: ``` slang> Broken paths found in a .pc file! /nix/store/23f6pl4nhwl4xx1h35hs64d1kqh9g5sk-slang-1.0g20221020_fdf27a0/share/pkgconfig/sv-lang.pc slang> The following lines have issues (specifically '//' in paths). slang> 2:includedir="${prefix}//nix/store/23f6pl4nhwl4xx1h35hs64d1kqh9g5sk-slang-1.0g20221020_fdf27a0/include" slang> 3:libdir="${prefix}//nix/store/23f6pl4nhwl4xx1h35hs64d1kqh9g5sk-slang-1.0g20221020_fdf27a0/lib" slang> It is very likely that paths are being joined improperly. slang> ex: "${prefix}/@CMAKE_INSTALL_LIBDIR@" should be "@CMAKE_INSTALL_FULL_LIBDIR@" slang> Please see NixOS/nixpkgs#144170 for more details. error: builder for '/nix/store/4w5nq781aw161b9lrk1f59j914ibk368-slang-1.0g20221020_fdf27a0.drv' failed with exit code 1; ```
258: Fix CMake install dir usage in pkgconfig, honour CMAKE_INSTALL_INCLUDEDIR r=AlanGriffiths a=OPNA2608 1. Current assumption about `CMAKE_INSTALL_<dir>` being relative to `CMAKE_INSTALL_PREFIX` is wrong. ``` Broken paths found in a .pc file! /nix/store/0lfs3w5mgjh5rdlrzwm4h18g2jpmmygx-wlcs-1.4.0/lib/pkgconfig/wlcs.pc The following lines have issues (specifically '//' in paths). 2:exec_prefix=${prefix}//nix/store/0lfs3w5mgjh5rdlrzwm4h18g2jpmmygx-wlcs-1.4.0/bin 3:libexecdir=${prefix}//nix/store/0lfs3w5mgjh5rdlrzwm4h18g2jpmmygx-wlcs-1.4.0/libexec It is very likely that paths are being joined improperly. ex: "${prefix}/`@CMAKE_INSTALL_LIBDIR@"` should be "`@CMAKE_INSTALL_FULL_LIBDIR@"` Please see NixOS/nixpkgs#144170 for more details. ``` `CMAKE_INSTALL_<dir>` must not be assumed to be relative to `CMAKE_INSTALL_PREFIX`, [according to the CMake documentation](https://cmake.org/cmake/help/v3.25/module/GNUInstallDirs.html?highlight=gnuinstalldirs): > It should typically be a path relative to the installation prefix so that it can be converted to an absolute path in a relocatable way (see `CMAKE_INSTALL_FULL_<dir>`). **However, an absolute path is also allowed.** CMake has special `_FULL_` variants of the variables which apply the prefix when needed: > The absolute path generated from the corresponding `CMAKE_INSTALL_<dir>` value. If the value is not already an absolute path, an absolute path is constructed typically by prepending the value of the [`CMAKE_INSTALL_PREFIX`](https://cmake.org/cmake/help/latest/variable/CMAKE_INSTALL_PREFIX.html#variable:CMAKE_INSTALL_PREFIX) variable. --- 2. `CMAKE_INSTALL_INCLUDEDIR` isn't honoured. CMake has `CMAKE_INSTALL_INCLUDEDIR` for specifying where header files shall be installed to. It defaults to `include` (relative to `CMAKE_INSTALL_PREFIX`) so when not specifying an include dir, this is functionally identical to the current hardcoding. Co-authored-by: OPNA2608 <christoph.neidahl@gmail.com>
For others running into this, slapping this into the derivation code worked for my issue:
It should be pretty robust against false-positives, although it only works when the prefix is literally "prefix" |
Found a couple more instances of similar issues just out of the files that weren't garbage collected on my laptop:
not sure about this one:
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 |
This patch fixes Capstone 5 build on NixOS. NixOS's build infrastructure sets CMAKE_INSTALL_{LIB,INCLUDE}DIR to absolute paths. If you append it to ${prefix}, you get the wrong path. NixOS automatically detects it and links this issue: NixOS/nixpkgs#144170
This patch fixes Capstone 5 build on NixOS. NixOS's build infrastructure sets CMAKE_INSTALL_{LIB,INCLUDE}DIR to absolute paths. If you append it to ${prefix}, you get the wrong path. NixOS automatically detects it and links this issue: NixOS/nixpkgs#144170
* Add Python bindings for WASM * Update Python bindings for m68k * Update Python bindings for mos65xx * Update Python bindings for x86 * Add Python bindings for SH * Update CS_* constants in Python bindings * Update constants from ARM auto-sync patch * Fixing TriCore disasm instructions (#2088) * allow absolute CMAKE_INSTALL_*DIR (#2134) This patch fixes Capstone 5 build on NixOS. NixOS's build infrastructure sets CMAKE_INSTALL_{LIB,INCLUDE}DIR to absolute paths. If you append it to ${prefix}, you get the wrong path. NixOS automatically detects it and links this issue: NixOS/nixpkgs#144170 --------- Co-authored-by: Peace-Maker <peace-maker@wcfan.de> Co-authored-by: Bastian Koppelmann <bkoppelmann@users.noreply.github.com> Co-authored-by: chayleaf <chayleaf@protonmail.com>
* Add Python bindings for WASM * Update Python bindings for m68k * Update Python bindings for mos65xx * Update Python bindings for x86 * Add Python bindings for SH * Update CS_* constants in Python bindings * Update constants from ARM auto-sync patch * Fixing TriCore disasm instructions (#2088) * allow absolute CMAKE_INSTALL_*DIR (#2134) This patch fixes Capstone 5 build on NixOS. NixOS's build infrastructure sets CMAKE_INSTALL_{LIB,INCLUDE}DIR to absolute paths. If you append it to ${prefix}, you get the wrong path. NixOS automatically detects it and links this issue: NixOS/nixpkgs#144170 * Disable swift binding const generate * update bindings const * update capstone version * update ChangeLog --------- Co-authored-by: Peace-Maker <peace-maker@wcfan.de> Co-authored-by: Bastian Koppelmann <bkoppelmann@users.noreply.github.com> Co-authored-by: chayleaf <chayleaf@protonmail.com>
Looks like we have this problem with libspatialindex as well. |
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) |
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: The The PR that referred this issue (capstone-engine/capstone#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 This variable can even be set at install-time:
So all uses of the value of |
You are mistaken.
The table just lists the defaults.
The next paragraph to the one you highlighted explicitly says absolute paths are allowed.
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 But upstream projects often do not care about relocatability so they opt for a simpler build script using |
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
The documentation specifically says that absolute paths are allowed, but discouraged because it does not work with other CMake features:
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
Thanks for the resource, I will try to implement a proper fix in the capstone repository. |
Yes, that is the reason. Here is an example from another package:
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.
Unfortunately, no:
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.
Right – but those features are irrelevant for us. We already know prefix at configure time and do not use
Some packages in Nixpkgs do indeed use 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 |
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
For me the big problem here is that the proposed 'solution' (to use 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 My solution for the capstone project (which works since at least CMake 3.0)
# 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
And
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 Since CMake 3.7 there is also the
This might be the case for 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
The meaning of 'portable' is also overloaded here, but using
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
Wouldn't it be an idea to let Nix parser and rewrite the |
(didn't realize there was already a patch; only saw ^ in the cross-refs for NixOS#144170) Note that this commit intentionally references the commit via `MikePopoloski/slang` instead of `dtzWill/slang` just in case the latter (fork repo) is deleted.
In cases where CMAKE_INSTALL_LIBDIR is already an absolut path adding a $prefix will be incorrect. See also issue on nix pkgs: NixOS/nixpkgs#144170 And current workaround: https://github.com/NixOS/nixpkgs/blob/7eee17a8a5868ecf596bbb8c8beb527253ea8f4d/pkgs/development/libraries/magic-enum/default.nix#L21
In cases where CMAKE_INSTALL_LIBDIR is already an absolut path adding a $prefix will be incorrect. See also issue on nix pkgs: NixOS/nixpkgs#144170 And current workaround: https://github.com/NixOS/nixpkgs/blob/7eee17a8a5868ecf596bbb8c8beb527253ea8f4d/pkgs/development/libraries/magic-enum/default.nix#L21
There are various issues where
cmake
does not work with nix store paths given asprefix
es, generating invalid paths intopkg-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'
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:
hhvm
: 029c3f9srt
: srt: Fix wrongsrt.pc
include path #71669exiv2
: exiv2: fix exiv2.pc file #81091.cmake
paths (see #144170 (comment)):bpp-seq
bpp-core
bpp-phyl
bpp-popgen
The text was updated successfully, but these errors were encountered: