Skip to content

rav1e: fix build with updated Darwin stdenv#234868

Merged
toonn merged 1 commit intoNixOS:stagingfrom
reckenrode:rav1e-cctools
Jun 14, 2023
Merged

rav1e: fix build with updated Darwin stdenv#234868
toonn merged 1 commit intoNixOS:stagingfrom
reckenrode:rav1e-cctools

Conversation

@reckenrode
Copy link
Contributor

@reckenrode reckenrode commented May 29, 2023

Description of changes

The updated Darwin stdenv uses llvm-strip, but that causes issues for rav1e when using NASM and linking it with the cctools ld64. Stripping the debug information instead of everything fixes the problem.

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.05 Release Notes (or backporting 22.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.

@reckenrode
rav1e: fix build with updated Darwin stdenv
9893913 
The updated Darwin stdenv uses `llvm-strip`, but that causes issues for
rav1e when using NASM and linking it with the cctools ld64. Stripping
the debug information instead of everything fixes the problem.
@reckenrode reckenrode mentioned this pull request May 29, 2023
Closed
@ofborg ofborg bot added the 6.topic: darwin Running or building packages on Darwin label May 29, 2023
@toonn
Copy link
Contributor

toonn commented Jun 11, 2023

Is there a way to override stdenv with another cctools to test this?

@reckenrode
Copy link
Contributor Author

reckenrode commented Jun 12, 2023 •
edited
Loading

rav1e was updated recently, so I’m going to test to see whether this fix is still necessary. I’m not able to reproduce using the following. The build succeeds.

Note: Using a newer LLVM is required because the older one uses the strip from cctools. #237429 must be applied to fix the build with cctools-llvm and a newer LLVM.

$ nix repl . --system x86_64-darwin
nix-repl> stdenvCctoolsLlvm = overrideCC stdenv (stdenv.cc.override {
  bintools = stdenv.cc.bintools.override {
    bintools = stdenv.cc.bintools.bintools.override {
      cctools = darwin.cctools-llvm.override { llvmPackages = llvmPackages_15; };
    };
  };
  libc = pkgs.darwin.Libsystem;
})
nix-repl> :b rav1e.override { stdenv = stdenvCctoolsLlvm; }

@toonn
Copy link
Contributor

toonn commented Jun 13, 2023

I think this override is still not working, I put a type strip in the postBuild of rav1e and strip seems to still point to cctools-port.

> nix repl .
warning: future versions of Nix will require using `--file` to load a file
Welcome to Nix 2.11.1. Type :? for help.

Loading installable ''...
Added 19369 variables.
nix-repl> stdenvCctoolsLlvm = overrideCC stdenv (stdenv.cc.override {bintools = stdenv.cc.bintools.override {bintools = stdenv.cc.bintools.bintools.override { cctools = darwin.cctools-llvm.override { llvmPackages = llvmPackages_15; }; }; };})
nix-repl> :b rav1e.override { stdenv = stdenvCctoolsLlvm; }
error: builder for '/nix/store/s0zib706v36smjh066288l1v5dp37cj1-rav1e-0.6.6.drv' failed with exit code 1;
       last 10 log lines:
       >    Compiling arg_enum_proc_macro v0.3.2
       >    Compiling simd_helpers v0.1.0
       >    Compiling scan_fmt v0.2.6
       >    Compiling y4m v0.8.0
       >    Compiling git2 v0.15.0
       >    Compiling built v0.5.2
       >    Compiling rav1e v0.6.6 (/private/tmp/nix-build-rav1e-0.6.6.drv-0/rav1e-0.6.6.tar.gz)
       >     Finished release [optimized + debuginfo] target(s) in 8m 47s
       > BLAAAAH
       > strip is /nix/store/nr4b3hbhh1izmcngzryb26rhwk37gkvz-cctools-binutils-darwin-973.0.1/bin/strip
       For full logs, run 'nix log /nix/store/s0zib706v36smjh066288l1v5dp37cj1-rav1e-0.6.6.drv'.
> ls -l /nix/store/nr4b3hbhh1izmcngzryb26rhwk37gkvz-cctools-binutils-darwin-973.0.1/bin/strip
lrwxr-xr-x  1 root  nixbld  74  1 jan  1970 /nix/store/nr4b3hbhh1izmcngzryb26rhwk37gkvz-cctools-binutils-darwin-973.0.1/bin/strip@ -> /nix/store/p42pyk9rzvdpv0gxc3rx2jx9hh4pck76-cctools-port-973.0.1/bin/strip

This is on 9bb343d628ba (HEAD, upstream/master) Merge pull request #237548 from rnhmjoj/pr-fix-rel to try to reproduce the build failure.

@reckenrode
Copy link
Contributor Author

reckenrode commented Jun 13, 2023

I have a build running against my clang 16 branch. If it fails there, I’ll try to find an override that properly includes llvm-strip, so you can test.

@reckenrode
Copy link
Contributor Author

reckenrode commented Jun 13, 2023

Actually, rav1e uses buildRustPackage. Try also overriding the stdenv in the rustPkatform it’s using.

toonn
toonn reviewed Jun 13, 2023
View reviewed changes
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.

Ah, that was key. Now I get a build failure on 9bb343d628ba (HEAD, upstream/master) Merge pull request #237548 from rnhmjoj/pr-fix-rel.

> nix-build --no-out-link --pure -E 'let ps = (import ./. {}); stdenvCCtools = ps.overrideCC ps.stdenv (ps.stdenv.cc.override { bintools = ps.stdenv.cc.bintools.override { bintools = ps.stdenv.cc.bintools.bintools.override { cctools = ps.darwin.cctools-llvm.override { llvmPackages = ps.llvmPackages_15; }; }; }; libc = ps.darwin.Libsystem; }); in ps.rav1e.override { stdenv = stdenvCCtools; rustPlatform = ps.makeRustPlatform (ps.rustPackages.buildRustPackages // { stdenv = stdenvCCtools; }); }'
[...]
       >   = note: ld: in /private/tmp/nix-build-rav1e-0.6.6.drv-0/rustcGil0X1/l
ibrav1e-c6b855765ab5a133.rlib(cdef_avx2.o), in section __TEXT,__text reloc 36: s
ymbol index out of range for architecture x86_64                                
       >           clang-11: error: linker command failed with exit code 1 (use 
-v to see invocation)                                                           
       >                                                                        
       >                                                                        
       > error: could not compile `rav1e` due to previous error                 
       For full logs, run 'nix log /nix/store/b4nxhwhn82gzdzrjf256gag7hg7inlsn-r
av1e-0.6.6.drv'.

And it works with this PR cherry-picked.

Does this need to target staging? I'm not quite sure because OfBorg failed so there's no label. I'm running a nixpkgs-review to check now.

@reckenrode
Copy link
Contributor Author

reckenrode commented Jun 14, 2023 •
edited
Loading

Does this need to target staging?

Running nixpkg-review pr 234868 --system x86_64-darwin shows 566 packages updated.

@ofborg eval

@reckenrode
Copy link
Contributor Author

reckenrode commented Jun 14, 2023

Linux is showing 473 updates when I run nixpkgs-review there. I assume reason is the addition of the postPatch attribute even though it evaluates to an empty string.

I’d been using 500+ rebuilds as my threshold for staging versus master. If that’s too low, I can retarget.

@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: 501+ This PR causes many rebuilds on Linux and should normally target the staging branches. 10.rebuild-linux: 501-1000 This PR causes many rebuilds on Linux and should normally target the staging branches. labels Jun 14, 2023
toonn
toonn approved these changes Jun 14, 2023
View reviewed changes
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.

Definitely should go to staging.

LGTM

@toonn toonn merged commit 50512e6 into NixOS:staging Jun 14, 2023
@reckenrode reckenrode mentioned this pull request Dec 1, 2023
Closed
@reckenrode reckenrode mentioned this pull request Dec 1, 2023
Merged
13 tasks
reckenrode added a commit to reckenrode/nixpkgs that referenced this pull request Dec 1, 2023
@reckenrode
x264: fix runtime crash due to llvm-strip args
0f0b89f 
This is a similar issue to NixOS#234868,
but it crashes instead of failing to link. The same fix applies (using
`-S` instead of `-x` with `llvm-strip`).
github-actions bot pushed a commit that referenced this pull request Dec 2, 2023
@reckenrode @github-actions
x264: fix runtime crash due to llvm-strip args
9ce7300 
This is a similar issue to #234868,
but it crashes instead of failing to link. The same fix applies (using
`-S` instead of `-x` with `llvm-strip`).

(cherry picked from commit 0f0b89f)
reckenrode referenced this pull request Jan 11, 2024
@reckenrode
llvmPackages: 11 -> 16 on Darwin
bcbdb80
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@toonn toonn toonn approved these changes

Assignees

No one assigned

Labels

6.topic: darwin Running or building packages on Darwin 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: 501-1000 This PR causes many rebuilds on Linux and should normally target the staging branches. 10.rebuild-linux: 501+ This PR causes many rebuilds on Linux and should normally target the staging branches.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@reckenrode @toonn