Skip to content

Comments

libcxxrt: add static lib to output#306238

Closed
ghost wants to merge 1 commit intomasterfrom
unknown repository
Closed

libcxxrt: add static lib to output#306238
ghost wants to merge 1 commit intomasterfrom
unknown repository

Conversation

@ghost
Copy link

@ghost ghost commented Apr 23, 2024

Description of changes

allow static linking libc++. libcxxrt.a is 220K when build for x64 linux, 198K x64 freebsd
tested with #305876

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 24.05 Release Notes (or backporting 23.05 and 23.11 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

@ghost
Copy link
Author

ghost commented Apr 23, 2024

@ofborg eval

@ofborg ofborg bot requested a review from alyssais April 23, 2024 11:00
@ofborg ofborg bot added 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-darwin: 1 This PR causes 1 package to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. 10.rebuild-linux: 1 This PR causes 1 package to rebuild on Linux. labels Apr 23, 2024
Comment on lines 19 to +20
cp lib/libcxxrt${stdenv.hostPlatform.extensions.library} $out/lib
cp lib/libcxxrt.a $out/lib
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apologies, what I said in #305876 wasn't quite right; stdenv.hostPlatform.extensions.library will yield .a when targeting a static platform:

staticLibrary =
/**/ if final.isWindows then ".lib"
else ".a";
library =
/**/ if final.isStatic then final.extensions.staticLibrary
else final.extensions.sharedLibrary;
executable =
/**/ if final.isWindows then ".exe"
else "";

This means that nix-build --arg crossSystem '{ system = "x86_64-freebsd13"; useLLVM = true; isStatic = true; }' -A libcxxrt would1 yield a libcxxrt that has lib/libcxxrt.a.


Given that the staticlib is relatively small, providing it even when !hostPlatform.isStatic seems reasonable to me (especially since libc++.a isn't gated on isStatic) but I'll defer to @alyssais on this.

If we do want libcxxrt to expose both the shared and static versions of the library I think the above should become something like:

Suggested change
cp lib/libcxxrt${stdenv.hostPlatform.extensions.library} $out/lib
cp lib/libcxxrt.a $out/lib
cp lib/libcxxrt${stdenv.hostPlatform.extensions.staticLibrary} $out/lib
'' + lib.optionalString stdenv.hostPlatform.hasSharedLibraries ''
cp lib/libcxxrt${stdenv.hostPlatform.extensions.sharedLibrary} $out/lib

Footnotes

  1. currently this fails with an unrelated error while building libc; pkgsStatic.libcxxrt for x86_64-linux also fails because ld cannot produce lib/libcxxrt.so (shared object) with musl's crtbeginT.o

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this seems slightly overly pedantic to me tho i suppose what i have breaks in the windows case. but this is more work then i care to do so closing.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm sorry. I appreciate the work you're doing and did not mean to discourage you.

Agreed that the above isn't an important change; if it's okay with the libcxxrt package maintainer, I'm happy with this PR as is.

@ghost ghost closed this Apr 23, 2024
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-darwin: 1 This PR causes 1 package to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. 10.rebuild-linux: 1 This PR causes 1 package to rebuild on Linux.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant