xev{d,e}: Fix darwin build by toonn · Pull Request #328422 · NixOS/nixpkgs

toonn · 2024-07-19T12:19:55Z

Description of changes

I'm bundling the changes for xevd and xeve because they are so similar. If preferable I can split them into two separate PRs.

This is a prerequisite to fixing ffmpeg_7-full on Darwin.
I'm still in the process of upstreaming this work so it's quite a number of patches to vendor, some of which might technically be possible to fetchpatch after upstreaming.
The patches are likely to change before being accepted upstream. So it mostly depends on how quickly we want ffmpeg_7-full working on Darwin (and potentially aarch64-linux, couldn't test).

Things done

Add a 👍 reaction to pull requests you find important.

SuperSandro2000 · 2024-07-19T12:21:39Z

pkgs/by-name/xe/xevd/package.nix

Suggested change

cmakeFlags = let inherit (lib) cmakeBool cmakeFeature optional;

inherit (stdenv.hostPlatform) isAarch64 isDarwin;

in optional isAarch64 (cmakeBool "ARM" true)

++ optional isDarwin

(cmakeFeature "CMAKE_SYSTEM_NAME" "Darwin");

cmakeFlags = lib.optional stdenv.hostPlatform.isAarch64 (lib.cmakeBool "ARM" true)

++ lib.optional stdenv.hostPlatform.isDarwin (lib.cmakeFeature "CMAKE_SYSTEM_NAME" "Darwin");

SuperSandro2000 · 2024-07-19T12:22:22Z

pkgs/by-name/xe/xeve/package.nix

Suggested change

cmakeFlags = let inherit (lib) cmakeBool cmakeFeature optional;

inherit (stdenv.hostPlatform) isAarch64 isDarwin;

in optional isAarch64 (cmakeBool "ARM" true)

++ optional isDarwin

(cmakeFeature "CMAKE_SYSTEM_NAME" "Darwin");

cmakeFlags = lib.optional stdenv.hostPlatform.isAarch64 (lib.cmakeBool "ARM" true)

++ lib.optional stdenv.hostPlatform.isDarwin (lib.cmakeFeature "CMAKE_SYSTEM_NAME" "Darwin");

less and boring code is always better :)

It's an existing pattern in Nixpkgs, the systemd stuff in nixos/modules makes heavy use of it.
It's just as explicit as fully qualified names and it makes the code more readable IMO so I'd prefer not to change this.

pkgs/by-name/xe/xevd/0001-xevdm_util-Avoid-skipping-cnt_tmp-values.patch

toonn · 2024-07-22T20:08:01Z

I marked this as draft because upstream moved. Restructuring the patches in this PR tomorrow.

toonn · 2024-07-23T14:47:17Z

I switched to fetching all the upstream patches I could, except for the cnt_tmp patches as those might change the behavior, instead adding NIX_CFLAGS_COMPILE disabling any warnings that remained errors.

Note that this should fix building with Clang on Linux too, as a consequence of fixing the Clang build for Darwin. Similarly it should now be compatible with aarch64-linux.

emilazy

Looks good to me; a small nit but I won’t hold this up if you don’t agree. Is mpeg5/xevd#67 not required?

pkgs/by-name/xe/xevd/package.nix

pkgs/by-name/xe/xeve/package.nix

emilazy

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

6 packages built:

xevd
xevd.dev
xevd.lib
xeve
xeve.dev
xeve.lib

pkgs/by-name/xe/xevd/package.nix

emilazy

The build is broken on x86_64-linux. I don’t know why, as I thought unknown warning parameters are meant to be ignored by default. Maybe we also need a -Wno-error=something-something-unknown-warnings, or to conditionalize on Clang.

xeve

-- Check for working C compiler: /nix/store/kp2j7yn0wzwq5piy494r54dafrh83s6s-gcc-wrapper-13.3.0/bin/gcc
-- Check for working C compiler: /nix/store/kp2j7yn0wzwq5piy494r54dafrh83s6s-gcc-wrapper-13.3.0/bin/gcc - broken
�[31mCMake Error at /nix/store/ll2b6cslly4v30im7qk34f9y21ziiz1i-cmake-3.29.6/share/cmake-3.29/Modules/CMakeTestCCompiler.cmake:67 (message):
  The C compiler

    "/nix/store/kp2j7yn0wzwq5piy494r54dafrh83s6s-gcc-wrapper-13.3.0/bin/gcc"

  is not able to compile a simple test program.

  It fails with the following output:

    Change Dir: '/build/source/build/CMakeFiles/CMakeScratch/TryCompile-E24bnB'
    
    Run Build Command(s): /nix/store/ll2b6cslly4v30im7qk34f9y21ziiz1i-cmake-3.29.6/bin/cmake -E env VERBOSE=1 /nix/store/m9288777y854d5k0xkxgacc8rslrhb8r-gnumake-4.4.1/bin/make -f Makefile cmTC_4b231/fast
    /nix/store/m9288777y854d5k0xkxgacc8rslrhb8r-gnumake-4.4.1/bin/make  -f CMakeFiles/cmTC_4b231.dir/build.make CMakeFiles/cmTC_4b231.dir/build
    make[1]: Entering directory '/build/source/build/CMakeFiles/CMakeScratch/TryCompile-E24bnB'
    Building C object CMakeFiles/cmTC_4b231.dir/testCCompiler.c.o
    /nix/store/kp2j7yn0wzwq5piy494r54dafrh83s6s-gcc-wrapper-13.3.0/bin/gcc    -o CMakeFiles/cmTC_4b231.dir/testCCompiler.c.o -c /build/source/build/CMakeFiles/CMakeScratch/TryCompile-E24bnB/testCCompiler.c
    cc1: error: '-Wno-error=for-loop-analysis': no option '-Wfor-loop-analysis'
    cc1: error: '-Wno-error=parentheses-equality': no option '-Wparentheses-equality'
    cc1: error: '-Wno-error=unknown-warning-option': no option '-Wunknown-warning-option'
    make[1]: *** [CMakeFiles/cmTC_4b231.dir/build.make:78: CMakeFiles/cmTC_4b231.dir/testCCompiler.c.o] Error 1
    make[1]: Leaving directory '/build/source/build/CMakeFiles/CMakeScratch/TryCompile-E24bnB'
    make: *** [Makefile:127: cmTC_4b231/fast] Error 2

xevd

-- Check for working C compiler: /nix/store/kp2j7yn0wzwq5piy494r54dafrh83s6s-gcc-wrapper-13.3.0/bin/gcc
-- Check for working C compiler: /nix/store/kp2j7yn0wzwq5piy494r54dafrh83s6s-gcc-wrapper-13.3.0/bin/gcc - broken
�[31mCMake Error at /nix/store/ll2b6cslly4v30im7qk34f9y21ziiz1i-cmake-3.29.6/share/cmake-3.29/Modules/CMakeTestCCompiler.cmake:67 (message):
  The C compiler

    "/nix/store/kp2j7yn0wzwq5piy494r54dafrh83s6s-gcc-wrapper-13.3.0/bin/gcc"

  is not able to compile a simple test program.

  It fails with the following output:

    Change Dir: '/build/source/build/CMakeFiles/CMakeScratch/TryCompile-fGA7JO'
    
    Run Build Command(s): /nix/store/ll2b6cslly4v30im7qk34f9y21ziiz1i-cmake-3.29.6/bin/cmake -E env VERBOSE=1 /nix/store/m9288777y854d5k0xkxgacc8rslrhb8r-gnumake-4.4.1/bin/make -f Makefile cmTC_0e187/fast
    /nix/store/m9288777y854d5k0xkxgacc8rslrhb8r-gnumake-4.4.1/bin/make  -f CMakeFiles/cmTC_0e187.dir/build.make CMakeFiles/cmTC_0e187.dir/build
    make[1]: Entering directory '/build/source/build/CMakeFiles/CMakeScratch/TryCompile-fGA7JO'
    Building C object CMakeFiles/cmTC_0e187.dir/testCCompiler.c.o
    /nix/store/kp2j7yn0wzwq5piy494r54dafrh83s6s-gcc-wrapper-13.3.0/bin/gcc    -o CMakeFiles/cmTC_0e187.dir/testCCompiler.c.o -c /build/source/build/CMakeFiles/CMakeScratch/TryCompile-fGA7JO/testCCompiler.c
    cc1: error: '-Wno-error=for-loop-analysis': no option '-Wfor-loop-analysis'
    cc1: error: '-Wno-error=sometimes-uninitialized': no option '-Wsometimes-uninitialized'; did you mean '-Wmaybe-uninitialized'?
    cc1: error: '-Wno-error=unknown-warning-option': no option '-Wunknown-warning-option'
    make[1]: *** [CMakeFiles/cmTC_0e187.dir/build.make:78: CMakeFiles/cmTC_0e187.dir/testCCompiler.c.o] Error 1
    make[1]: Leaving directory '/build/source/build/CMakeFiles/CMakeScratch/TryCompile-fGA7JO'
    make: *** [Makefile:127: cmTC_0e187/fast] Error 2

toonn · 2024-07-23T16:01:09Z

Ah, the reason my upstream PR did conditional appending of compiler options. Should've realized the same problem would crop up again. I'll think a bit about a fix. Might not be for tonight though.

emilazy · 2024-07-23T16:08:04Z

Looks like it’s a known GCC issue and I don’t know if there’s a workaround. I think just making the flags conditional on Clang would be fine, as they can all hopefully be removed on the next upstream version bump. But whatever you decide on is okay with me.

toonn · 2024-07-23T19:07:12Z

@emilazy, #gcc said -Wno-* variants might work, they shouldn't cause a problem unless there's other errors.

emilazy

Still weird on Linux, I’m afraid.

Result of nixpkgs-review pr 328422 run on x86_64-linux 1

8 packages failed to build:

ffmpeg_7-full
ffmpeg_7-full.bin
ffmpeg_7-full.data
ffmpeg_7-full.dev
ffmpeg_7-full.doc
ffmpeg_7-full.lib
ffmpeg_7-full.man
handbrake

6 packages built:

xevd
xevd.dev
xevd.lib
xeve
xeve.dev
xeve.lib

ERROR: xevd >= 0.4.1 not found using pkg-config

If you think configure made a mistake, make sure you are using the latest
version from Git.  If the latest version fails, report the problem to the
ffmpeg-user@ffmpeg.org mailing list or IRC #ffmpeg on irc.libera.chat.
Include the log file "ffbuild/config.log" produced by configure as this will help
solve the problem.

pkgs/by-name/xe/xeve/package.nix

pkgs/by-name/xe/xevd/package.nix

jopejoe1 · 2024-07-23T19:50:01Z

Still weird on Linux, I’m afraid.

Result of nixpkgs-review pr 328422 run on x86_64-linux 1
8 packages failed to build:
6 packages built:

ERROR: xevd >= 0.4.1 not found using pkg-config

If you think configure made a mistake, make sure you are using the latest
version from Git.  If the latest version fails, report the problem to the
ffmpeg-user@ffmpeg.org mailing list or IRC #ffmpeg on irc.libera.chat.
Include the log file "ffbuild/config.log" produced by configure as this will help
solve the problem.

That was the error I solved with adding -lm to cflags.

toonn · 2024-07-23T20:15:26Z

@emilazy, re mpeg5/xevd#67, since I can't respond to a top-level review comment specifically. That's worked around with the disabling of the warnings, which is simpler and upstream might opt to go for explicit branching on the compiler like they did for Xeve so I don't really care about carrying that patch.

Still looking into the pkg-config problem but my builder is wimpy and since ffmpeg_7-full is the package displaying the issue it'll have to be for tomorrow.

If desirable I can drop the dropping of the -lm flag but my interest is piqued.

emilazy · 2024-07-23T20:20:33Z

No rush; feel free to shave those yaks :)

FWIW it fails right at the start of -full; I only had to compile the packages you’re directly touching for it to hit that error, thankfully.

toonn · 2024-07-25T17:09:06Z

Ok, dropping the dropping of -lm for now, I'm getting to the bottom of it but it should probably be a separate PR anyway. I originally dropped it because Clang complains about it but since we're disabling warnings already anyway...

I'll squash everything if this is finally ready for merge. I'm not sure fixup commits are actually at all helpful for GitHub's review UI compared to force pushes?

dariusz-f · 2024-07-26T12:13:28Z

Hi,
we applied all patches to xeve and xevd that you're submitted except changes in cmake, because we prefer different approach. If you have any further problems, please feel free to create an issue or submit a PR.

emilazy

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

6 packages built:

xevd
xevd.dev
xevd.lib
xeve
xeve.dev
xeve.lib

Result of nixpkgs-review pr 328422 run on x86_64-linux 1

14 packages built:

ffmpeg_7-full
ffmpeg_7-full.bin
ffmpeg_7-full.data
ffmpeg_7-full.dev
ffmpeg_7-full.doc
ffmpeg_7-full.lib
ffmpeg_7-full.man
handbrake
xevd
xevd.dev
xevd.lib
xeve
xeve.dev
xeve.lib

Result of nixpkgs-review pr 328422 run on aarch64-linux 1

6 packages failed to build:

xevd
xevd.dev
xevd.lib
xeve
xeve.dev
xeve.lib

The aarch64-linux failure is odd:

-- Check for working C compiler: /nix/store/pxgjyjhzrqyjqgx4b4pqc5xc550agj1w-gcc-wrapper-13.3.0/bin/gcc
-- Check for working C compiler: /nix/store/pxgjyjhzrqyjqgx4b4pqc5xc550agj1w-gcc-wrapper-13.3.0/bin/gcc - broken
CMake Error at /nix/store/hyr25rm1gxiz04bbdvdk7a6291anq685-cmake-3.29.6/share/cmake-3.29/Modules/CMakeTestCCompiler.cmake:67 (message):
  The C compiler

    "/nix/store/pxgjyjhzrqyjqgx4b4pqc5xc550agj1w-gcc-wrapper-13.3.0/bin/gcc"

  is not able to compile a simple test program.

  It fails with the following output:

    Change Dir: '/build/source/build/CMakeFiles/CMakeScratch/TryCompile-Ny56VP'
    
    Run Build Command(s): /nix/store/hyr25rm1gxiz04bbdvdk7a6291anq685-cmake-3.29.6/bin/cmake -E env VERBOSE=1 /nix/store/70s3bcvm1fjrn9lkxzkihrngxm73y82j-gnumake-4.4.1/bin/make -f Makefile cmTC_15b1f/fast
    /nix/store/70s3bcvm1fjrn9lkxzkihrngxm73y82j-gnumake-4.4.1/bin/make  -f CMakeFiles/cmTC_15b1f.dir/build.make CMakeFiles/cmTC_15b1f.dir/build
    make[1]: Entering directory '/build/source/build/CMakeFiles/CMakeScratch/TryCompile-Ny56VP'
    Building C object CMakeFiles/cmTC_15b1f.dir/testCCompiler.c.o
    /nix/store/pxgjyjhzrqyjqgx4b4pqc5xc550agj1w-gcc-wrapper-13.3.0/bin/gcc   -flax-vector-conversions  -std=gnu99 -o CMakeFiles/cmTC_15b1f.dir/testCCompiler.c.o -c /build/source/build/CMakeFiles/CMakeScratch/TryCompile-Ny56VP/testCCompiler.c
    Linking C executable cmTC_15b1f
    /nix/store/hyr25rm1gxiz04bbdvdk7a6291anq685-cmake-3.29.6/bin/cmake -E cmake_link_script CMakeFiles/cmTC_15b1f.dir/link.txt --verbose=1
    /nix/store/pxgjyjhzrqyjqgx4b4pqc5xc550agj1w-gcc-wrapper-13.3.0/bin/gcc  -flax-vector-conversions  -static  CMakeFiles/cmTC_15b1f.dir/testCCompiler.c.o -o cmTC_15b1f
    /nix/store/1jbqn7z7rh2q23nda4r66hr601z0w753-binutils-2.42/bin/ld: cannot find -lm: No such file or directory
    /nix/store/1jbqn7z7rh2q23nda4r66hr601z0w753-binutils-2.42/bin/ld: cannot find -lc: No such file or directory
    collect2: error: ld returned 1 exit status
    make[1]: *** [CMakeFiles/cmTC_15b1f.dir/build.make:99: cmTC_15b1f] Error 1
    make[1]: Leaving directory '/build/source/build/CMakeFiles/CMakeScratch/TryCompile-Ny56VP'
    make: *** [Makefile:127: cmTC_15b1f/fast] Error 2

This is clearly an improvement on the status quo, so I’m happy to merge as‐is. But it seems like we won’t be able to flip it back on for non‐x86 Linux and it might have something to do with the -lm stuff. Have you tested this on aarch64-darwin?

About GitHub reviews: the UI is just bad in general, there’s no good option. I think i prefer rebasing in‐place except when there’s already a lot of changes in the PR and the fixup diff is clearly reviewable by itself.

toonn · 2024-07-26T16:19:28Z

Ah, shucks. I'm currently awaiting upstream's response to another pair of PRs that should solve the -lm problem but I don't expect that to fix the build for aarch64-linux.

I'll move this back to draft until I have more info.

toonn · 2024-07-29T11:40:12Z

Waiting to see response to #330842 before deciding what to do here.

toonn · 2024-07-31T15:52:44Z

@emilazy, @jopejoe1, this should be ready.

emilazy

Result of nixpkgs-review pr 328422 run on aarch64-darwin 1

6 packages built:

xevd
xevd.dev
xevd.lib
xeve
xeve.dev
xeve.lib

Result of nixpkgs-review pr 328422 run on x86_64-linux 1

15 packages built:

ffmpeg_7-full
ffmpeg_7-full.bin
ffmpeg_7-full.data
ffmpeg_7-full.dev
ffmpeg_7-full.doc
ffmpeg_7-full.lib
ffmpeg_7-full.man
handbrake
nixpkgs-manual
xevd
xevd.dev
xevd.lib
xeve
xeve.dev
xeve.lib

jopejoe1 · 2024-07-31T17:16:24Z

Thanks for the work.

SuperSandro2000 reviewed Jul 19, 2024

View reviewed changes

This was referenced Jul 19, 2024

ffmpeg_7-full: Enable xev{d,e} on Darwin systems #328428

Merged

ffmpeg_7: fix darwin build #322724

Merged

ofborg bot added the 6.topic: darwin Running or building packages on Darwin label Jul 19, 2024

emilazy requested changes Jul 19, 2024

View reviewed changes

pkgs/by-name/xe/xevd/0001-xevdm_util-Avoid-skipping-cnt_tmp-values.patch Outdated Show resolved Hide resolved

ofborg bot requested a review from jopejoe1 July 19, 2024 13:16

ofborg bot added 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. labels Jul 19, 2024

toonn marked this pull request as draft July 22, 2024 20:07

toonn force-pushed the xev{d,e}-darwin branch from cb27931 to d278dde Compare July 23, 2024 14:42

toonn marked this pull request as ready for review July 23, 2024 14:43

emilazy approved these changes Jul 23, 2024

View reviewed changes

pkgs/by-name/xe/xevd/package.nix Outdated Show resolved Hide resolved

pkgs/by-name/xe/xeve/package.nix Outdated Show resolved Hide resolved

emilazy requested changes Jul 23, 2024

View reviewed changes

pkgs/by-name/xe/xevd/package.nix Outdated Show resolved Hide resolved

emilazy requested changes Jul 23, 2024

View reviewed changes

toonn force-pushed the xev{d,e}-darwin branch from 569a7fe to 601ec63 Compare July 23, 2024 19:05

emilazy requested changes Jul 23, 2024

View reviewed changes

pkgs/by-name/xe/xeve/package.nix Outdated Show resolved Hide resolved

pkgs/by-name/xe/xevd/package.nix Outdated Show resolved Hide resolved

emilazy reviewed Jul 26, 2024

View reviewed changes

toonn marked this pull request as draft July 26, 2024 16:19

toonn force-pushed the xev{d,e}-darwin branch from d2e0840 to 4f1b1c5 Compare July 29, 2024 22:15

toonn added 2 commits July 30, 2024 00:17

xevd: Fix Darwin build

2b430b6

xeve: Fix Darwin build

d95b967

toonn force-pushed the xev{d,e}-darwin branch from 4f1b1c5 to d95b967 Compare July 29, 2024 22:17

toonn marked this pull request as ready for review July 29, 2024 22:18

emilazy approved these changes Jul 31, 2024

View reviewed changes

emilazy merged commit 4c878ab into NixOS:master Jul 31, 2024

-  cmakeFlags = let inherit (lib) cmakeBool cmakeFeature optional;
-                   inherit (stdenv.hostPlatform) isAarch64 isDarwin;
-                in optional isAarch64 (cmakeBool "ARM" true)
-                ++ optional isDarwin
-                            (cmakeFeature "CMAKE_SYSTEM_NAME" "Darwin");
+  cmakeFlags = lib.optional stdenv.hostPlatform.isAarch64 (lib.cmakeBool "ARM" true)
+    ++ lib.optional stdenv.hostPlatform.isDarwin (lib.cmakeFeature "CMAKE_SYSTEM_NAME" "Darwin");

Uh oh!

Conversation

toonn commented Jul 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description of changes

Things done

Uh oh!

SuperSandro2000 Jul 19, 2024

Choose a reason for hiding this comment

Uh oh!

SuperSandro2000 Jul 19, 2024

Choose a reason for hiding this comment

Uh oh!

toonn Jul 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

toonn commented Jul 22, 2024

Uh oh!

toonn commented Jul 23, 2024

Uh oh!

emilazy left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

emilazy left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

emilazy left a comment

Choose a reason for hiding this comment

xeve

xevd

Uh oh!

toonn commented Jul 23, 2024

Uh oh!

emilazy commented Jul 23, 2024

Uh oh!

toonn commented Jul 23, 2024

Uh oh!

emilazy left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jopejoe1 commented Jul 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

toonn commented Jul 23, 2024

Uh oh!

emilazy commented Jul 23, 2024

Uh oh!

toonn commented Jul 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dariusz-f commented Jul 26, 2024

Uh oh!

emilazy left a comment

Choose a reason for hiding this comment

Uh oh!

toonn commented Jul 26, 2024

Uh oh!

toonn commented Jul 29, 2024

Uh oh!

toonn commented Jul 31, 2024

Uh oh!

emilazy left a comment

Choose a reason for hiding this comment

Uh oh!

jopejoe1 commented Jul 31, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

toonn commented Jul 19, 2024 •

edited

Loading

toonn Jul 19, 2024 •

edited

Loading

jopejoe1 commented Jul 23, 2024 •

edited

Loading

toonn commented Jul 25, 2024 •

edited

Loading