-
-
Notifications
You must be signed in to change notification settings - Fork 14.8k
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: make check-pc-files hook also check .cmake files #247474
base: staging
Are you sure you want to change the base?
Conversation
pkgs/development/tools/build-managers/cmake/check-improper-prefix-hook.sh
Outdated
Show resolved
Hide resolved
pkgs/development/tools/build-managers/cmake/check-improper-prefix-hook.sh
Show resolved
Hide resolved
2e7ca17
to
8b1cdbc
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ping me 48h to merge.
broken paths in my store
Gonna generate a bigger list #172150 (comment) |
list of files with issues
LLVM's have to be fixed before merge |
8b1cdbc
to
f0281f7
Compare
this will take a while, as each LLVM version needs to be changed separately (in order to create proper fixes that don't just satisfy this hook's check but are more thorough) edit: |
f0281f7
to
d0f31e2
Compare
Such llvm patches can be submitted to upstream? |
it's already upstreamed, it's only a problem on old LLVM versions (<15, but it's almost completely fixed on 14), see ex. llvm/llvm-project@ac0d1d5 |
I haven't tested whether all LLVMs build yet, but overall I think I got rid of all instances of "improper prefix usage" (in particular, in LLVM 14 it was being used indirectly to calculate install prefix based on the assumption that When it comes to the other libraries, it's better to upstream it rather than create hacks (and care needs to be taken to avoid CMake build scripts indirectly relying on relative prefixes where NixOS passes absolute ones, like LLVM 14). I'll try to do it, though I don't have that much time so it will take a while. |
554fe80
to
34ccd34
Compare
8347393
to
467a7c4
Compare
Do you have any links to discussions they had from that time? The fix proposed by the hook is to use |
Description of changes
#245527 PR author ran into the issue (#144170) where many programs expect
CMAKE_INSTALL_LIBDIR
& co. to be a relative path, but NixOS sets it to an absolute path. Fedora used to do the same until 2012, but ran into the same issue so it doesn't do that anymore.Leaving aside whether this should be reconsidered like Fedora did, the #245527 PR author saw the warning about
//nix
being in a .pc file, so they fixed the code to useCMAKE_INSTALL_FULL_LIBDIR
. This got rid of the warning, but the.cmake
files still contained//nix
. This PR adds a warning for}//nix
in.cmake
files as well (maybe//nix
check would make more sense since it's comparatively less likely that it would be}//nix
in .cmake files).Things done
sandbox = true
set innix.conf
? (See Nix manual)nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD"
. Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/
)