Skip to content

Comments

nodejs: use a response file with llvm-ar#243683

Merged
toonn merged 1 commit intoNixOS:masterfrom
reckenrode:nodejs-fix-ar
Jul 16, 2023
Merged

nodejs: use a response file with llvm-ar#243683
toonn merged 1 commit intoNixOS:masterfrom
reckenrode:nodejs-fix-ar

Conversation

@reckenrode
Copy link
Contributor

Description of changes

nodejs produces a static archive in its postInstall. It detects if the ar is GNU ar and uses a response file. Otherwise, it adds the files individually. This is apparently very slow with llvm-ar, which Darwin now uses by default. Fortunately, llvm-ar also supports response files, so detect whether the ar is llvm-ar and use a response file.

I tested the build on aarch64-darwin. postInstall took less than a minute to generate a 59 MiB static archive. Comparing to the build on master, the only difference between the two archives is llvm-ar zeroes out the dates, uids, and gids by default. Compared disassembly of the archives appeared identical.

This fixes the timeouts on staging-next. #241951

https://hydra.nixos.org/build/227170390

Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • 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/)
  • 23.11 Release Notes (or backporting 23.05 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.

nodejs produces a static archive in its `postInstall`. It detects if the
`ar` is GNU ar and uses a response file. Otherwise, it adds the files
individually. This is apparently very slow with `llvm-ar`, which Darwin
now uses by default. Fortunately, `llvm-ar` also supports response
files, so detect whether the `ar` is `llvm-ar` and use a response file.

I tested the build on aarch64-darwin. `postInstall` took less than a
minute to generate a 59 MiB static archive. Comparing to the build on
master, the only difference between the two archives is `llvm-ar` zeroes
out the dates, uids, and gids by default. Compared disassembly of the
archives appeared identical.

This fixes the timeouts on staging-next. NixOS#241951

https://hydra.nixos.org/build/227170390
@github-actions github-actions bot added the 6.topic: nodejs Node.js is a free, open-source, cross-platform JavaScript runtime environment label Jul 15, 2023
Copy link
Contributor

@toonn toonn left a comment

Choose a reason for hiding this comment

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

LGTM

I'll start a build on x86_64-darwin but since this is blocking staging-next maybe we don't want to wait for that?

@ofborg ofborg bot requested review from cillianderoiste, cko, gilligan and marsam July 15, 2023 17:20
@ofborg ofborg bot added 10.rebuild-darwin: 501+ This PR causes many rebuilds on Darwin and should normally target the staging branches. 10.rebuild-darwin: 501-1000 This PR causes many rebuilds on Darwin and should normally target the staging branches. 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux. labels Jul 15, 2023
Comment on lines +163 to +164
# llvm-ar supports response files, so take advantage of it if it’s available.
if [ "$(basename $(readlink -f $(command -v ar)))" = "llvm-ar" ]; then
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks fine to me too. I wonder though if there some way to find out whether llvm-ar is expected at eval time in nix so the block becomes:

${if stdenv.buildPlatform.isGnu || stdenv.buildPlatform.isLlvmAr} then ''
    ar -cqs $libv8/lib/libv8.a @files
'' else ''
    ...
''

Copy link
Contributor

Choose a reason for hiding this comment

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

Not currently. We discussed in the Matrix room.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ideally we would set isLLVM, but Darwin isn’t a pure-LLVM platform. Maybe someday, but that’s still a ways off.

@vcunat vcunat changed the base branch from staging-next to master July 15, 2023 17:53
@toonn
Copy link
Contributor

toonn commented Jul 16, 2023

Result of nixpkgs-review pr 243683 run on x86_64-darwin 1

2 packages built:
  • nodejs
  • nodejs.libv8 (nodejs.libv8.libv8)

@toonn toonn merged commit ba7cf6a into NixOS:master Jul 16, 2023
@happysalada
Copy link
Contributor

not sure if this is a direct consequence of this PR, but I'm getting x86_64-darwin failure on building nodejs-20

> 20 libLLVM.dylib           0x000000010b01b022 llvm::CrashRecoveryContext::RunSafely(llvm::function_ref<void ()>) + 226
       > 21 libclang-cpp.11.1.dylib 0x000000010468e0c0 clang::driver::CC1Command::Execute(llvm::ArrayRef<llvm::Optional<llvm::StringRef> >, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::alloriver::Command const&, clang::driver::Command const*&) const + 433
       > 23 libclang-cpp.11.1.dylib 0x000000010465c38c clang::driver::Compilation::ExecuteJobs(clang::driver::JobList const&, llvm::SmallVectorImpl<std::__1::pair<int, clang::driver::Command const*> >&) const + 140
       > 24 libclang-cpp.11.1.dylib 0x000000010467594c clang::driver::Driver::ExecuteCompilation(clang::driver::Compilation&, llvm::SmallVectorImpl<std::__1::pair<int, clang::driver::Command const*> >&) + 428
       > 25 clang-11                0x00000001000808cb main + 10283
       > 26 dyld                    0x0000000102f9852e start + 462
       > make[1]: *** [tools/v8_gypfiles/v8_base_without_compiler.target.mk:1094: /private/tmp/nix-build-nodejs-20.4.0.drv-0/node-v20.4.0/out/Release/obj.target/v8_base_without_compiler/deps/v8/src/objects/js-objects.o] Error 1
       > rm 11378f45f6f727b4d1ef2a50a5da6aa69a68202f.intermediate 985c41846a56d0066a3464bf60ffed375bec8901.intermediate 222919265df17f467ac2cf668d8486e0182908ab.intermediate 57a81c7004bf740e697481ff919eff0eb2213386.intermediate 1f93354eaa1606beae7636815a5da2d13e1f7a6a.intermediate
       > make: *** [Makefile:134: node] Error 2

This is not an emergency for me, just saying.

@marsam marsam mentioned this pull request Jul 18, 2023
12 tasks
@reckenrode
Copy link
Contributor Author

Is there more to the crash? That looks like it happened in clang. This change was made in postInstall, so it shouldn’t have affected the build itself.

@reckenrode
Copy link
Contributor Author

reckenrode commented Jul 18, 2023

I commented in #241852, but to follow up here. It built for me locally, but the build required ~30 GiB of space. The error in the Hydra log suggests the builder lacks enough free disk space to build nodejs_20.

@happysalada
Copy link
Contributor

@vcunat sorry to ping you, but you're the only one that knows about hydra that I know.
Is it possible that the build is just lacking disk space ? Is there anything we can tweak to improve the build disk size available for it ?

@vcunat
Copy link
Member

vcunat commented Jul 19, 2023

The log says

LLVM ERROR: IO failure on output stream: No space left on device

so yes, that instance does look like it ran out of space ;-)

Let me retry the build 🤷🏽 Overall the nodejs impact of this PR looks very good on Hydra:
https://hydra.nixos.org/eval/1797621?filter=nodejs

@reckenrode reckenrode deleted the nodejs-fix-ar branch July 19, 2023 05:53
@vcunat
Copy link
Member

vcunat commented Jul 19, 2023

✅ succeeded

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

6.topic: nodejs Node.js is a free, open-source, cross-platform JavaScript runtime environment 10.rebuild-darwin: 501-1000 This PR causes many rebuilds on Darwin and should normally target the staging branches. 10.rebuild-darwin: 501+ This PR causes many rebuilds on Darwin and should normally target the staging branches. 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants