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

Cmake paths patch 1/? (boost, aws-sdk-cpp) #85254

Merged
merged 2 commits into from May 3, 2020
Merged

Conversation

@thequux
Copy link
Contributor

thequux commented Apr 14, 2020

Motivation for this change

Many packages generate CMake configuration files that try to figure out the paths to include and library directories from the location of the CMake file; this breaks in NixOS derivations with a dev output because the CMake package gets installed to $out/lib/cmake, but then gets moved into the dev output by the _multioutDevs fixup hook.

This needs to be fixed on a case-by-case basis, because this usually involves custom logic. So far, I've fixed boost (>= 1.70; before that, it didn't install cmake files as far as I can tell) and aws-sdk-cpp.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip" (I hoped Hydra would do that for me)
  • Tested execution of all binary files (usually in ./result/bin/) (no binaries, only libraries, and they should be unaffected)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
    (within 10KiB for both)
  • Ensured that relevant documentation is up to date (I couldn't find any)
  • Fits CONTRIBUTING.md.
thequux added 2 commits Apr 14, 2020
AWS's SDK by default tries to prepend its install root to each of the
library paths; this obviously fails with the absolute paths that Nix
gives it. Worse, it computes the installation root by walking up the
filesystem from its cmake file, so even if the AWSSDK_ROOT_DIR is
explicitly set to the root directory, it gets replaced with the path
to the derivation's dev output.

This is all fixed with a patch to the cmake files that generate the
installed configuration.

Once this is fixed, it *still* doesn't work because the export
generator built into cmake insists on adding `$out/include` to the
header search path; when importing this configuration in another
package, cmake will fail because `$out/include` doesn't exist (After
all, it was relocated by a fixup hook). A small postFixupHook will
recreate the directory and make cmake happy.
Boost generates its installed cmake configuration using custom logic
in its own build system; while this logic *knows* where it should be
installed, the generated config overrides the correct information with
new paths based on the location of the cmake configuration file in an
attempt to let the package be relocated after installation.

This patch simply undoes that.
@thequux thequux changed the title Cmake paths 1 Cmake paths patch 1/? (boost, aws-sdk-cpp) Apr 14, 2020
@ofborg ofborg bot requested review from edolstra and peti Apr 14, 2020
@veprbl veprbl mentioned this pull request Apr 14, 2020
4 of 10 tasks complete
@veprbl
Copy link
Member

veprbl commented Apr 14, 2020

This looks useful! @thequux, do you think these patches or parts of them can be sent upstream?

@thequux
Copy link
Contributor Author

thequux commented Apr 14, 2020

The aws-cpp-sdk one can be; it's designed to mangle the logic as little as possible. However, the boost patch is very nix-specific; bjam is not exactly my jam, so I didn't try to do a nice job of it, but rather make something that would be robust to upstream changes.

@veprbl
Copy link
Member

veprbl commented Apr 14, 2020

@thequux Could you please do that? The reason for upstreaming patches (especially bigger ones like the one you add to aws-sdk-cpp) is so that we don't take extra effort to maintain them in the future, plus we get them looked at by the upstream authors, and one other thing is that we can then use fetchpatch to fetch them from an url instead of piling them in git history. For the boost one, it would be preferred to at least file an upstream bug.

@thequux
Copy link
Contributor Author

thequux commented Apr 14, 2020

Sure. I'll send a patch for aws-sdk-cpp first thing tomorrow morning, but for boost, I need to first verify that we're actually giving bjam all of the correct paths at build time, rather than depending on the preFixup hook to move things into the correct output.

@thequux
Copy link
Contributor Author

thequux commented Apr 15, 2020

I submitted a slightly improved version of the patch in aws/aws-sdk-cpp#1365 ; the differences between this patch and that one are irrelevant for Nix's purposes (specifically, the upstream patch will generate valid pkg-config files even if the install dirs aren't absolute).

I get the impression that the AWS SDK hasn't been focusing much on pull requests recently, as upstream hasn't touched any PRs since 6 Jan, so it may be best to use an in-tree patch until they get through their backlog.

@veprbl veprbl mentioned this pull request Apr 24, 2020
5 of 10 tasks complete
@doronbehar
Copy link
Contributor

doronbehar commented May 3, 2020

Just wanted to say that both this and #73940 work, as mentioned in #85922 . What I've found out today though, was that with the workaround supplied at: #85922 (comment)

Makes (e.g) mtxclient reference boost.dev. With either this or the other patch, it doesn't.

@nixos-discourse
Copy link

nixos-discourse commented May 3, 2020

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/prs-in-distress/3604/20

@bhipple
Copy link
Contributor

bhipple commented May 3, 2020

Thanks for doing this! I had stumbled on the broken aws-sdk-cpp pcfiles a while ago, but hacked it up with a local overlay fix using sed since I didn't have the time to stitch together the proper, clean fix.

I've built and verified this fixes the issue; can you send a backport PR to 20.03 as well?

Separately, it'd be nice if we had some kind of automated pkg-config file validation check, like making sure includedirs and libdirs actually exist, which would fail the build if not.

@bhipple bhipple merged commit 405909f into NixOS:master May 3, 2020
16 checks passed
16 checks passed
Evaluation Performance Report Evaluator Performance Report
Details
aws-sdk-cpp, aws-sdk-cpp.passthru.tests, boost, boost.passthru.tests on aarch64-linux Success
Details
aws-sdk-cpp, aws-sdk-cpp.passthru.tests, boost, boost.passthru.tests on x86_64-linux Success
Details
grahamcofborg-eval ^.^!
Details
grahamcofborg-eval-check-maintainers matching changed paths to changed attrs...
Details
grahamcofborg-eval-check-meta config.nix: checkMeta = true
Details
grahamcofborg-eval-darwin nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="777df0b"; rev="777df0b4a548720e862b498d53bd1827b5743469"; } ./pkgs/t
Details
grahamcofborg-eval-lib-tests nix-build --arg pkgs import ./. {} ./lib/tests/release.nix
Details
grahamcofborg-eval-nixos nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="777df0b"; rev="777df0b4a548720e862b498d53bd1827b5743469"; } ./nixos/
Details
grahamcofborg-eval-nixos-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="777df0b"; rev="777df0b4a548720e862b498d53bd1827b5743469"; } ./nixos/
Details
grahamcofborg-eval-nixos-options nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="777df0b"; rev="777df0b4a548720e862b498d53bd1827b5743469"; } ./nixos/
Details
grahamcofborg-eval-nixpkgs-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="777df0b"; rev="777df0b4a548720e862b498d53bd1827b5743469"; } ./pkgs/t
Details
grahamcofborg-eval-nixpkgs-tarball nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="777df0b"; rev="777df0b4a548720e862b498d53bd1827b5743469"; } ./pkgs/t
Details
grahamcofborg-eval-nixpkgs-unstable-jobset nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="777df0b"; rev="777df0b4a548720e862b498d53bd1827b5743469"; } ./pkgs/t
Details
grahamcofborg-eval-package-list nix-env -qa --json --file .
Details
grahamcofborg-eval-package-list-no-aliases nix-env -qa --json --file . --arg config { allowAliases = false; }
Details
@DIzFer DIzFer mentioned this pull request May 7, 2020
11 of 37 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.