ffmpeg_4: fix build issue introduced by texinfo 7.1 update by afh · Pull Request #326249 · NixOS/nixpkgs

afh · 2024-07-11T06:07:40Z

Description of changes

#289690 introduced a regression that causes a ffmpeg4 build failure (see #326231). This PR uses "trofi's hack" to work around the issue.

Things done

Add a 👍 reaction to pull requests you find important.

Atemu · 2024-07-11T06:36:34Z

pkgs/development/libraries/ffmpeg/generic.nix

Suggested change

nativeBuildInputs = [ removeReferencesTo addDriverRunpath perl pkg-config texinfo yasm ]

nativeBuildInputs = [ removeReferencesTo addDriverRunpath perl pkg-config yasm ]

Atemu · 2024-07-11T06:37:07Z

ffmpeg_4 btw to get of ofBorg to work.

@ofborg build ffmpeg_4

Atemu · 2024-07-11T06:54:42Z

pkgs/development/libraries/ffmpeg/generic.nix

Suggested change

++ (if lib.versionOder "5" then [ texinfo6 ] else [ texinfo ]);

++ (if lib.versionOlder "5" then [ texinfo6 ] else [ texinfo ]);

afh · 2024-07-11T08:35:59Z

Thanks for your comments and apologies for the typos, @Atemu.

I'll update the commit message once I'm back in front of a workstation.

Atemu

Please test your code before opening a non-draft PR.

pkgs/development/libraries/ffmpeg/generic.nix

alyssais · 2024-07-11T13:46:46Z

(Looking at the commit message.) Wasn't it the update that caused the build regression, not the refactor?

afh · 2024-07-12T05:48:36Z

You are absolutely right, @Atemu, this PR is not up to (my usual) standards, as I've been hastily making the changes throughout busy days. Usually PRs are tested locally on my end, yet with this PR targeting staging and my workstation taking ages to complete, I had hoped I can cut corners on this one.

The good news: peer review once again proves to be helpful!
Thanks a ton for keeping a keen and watchful eye and apologies for the sloppiness that took up and people's valuable time and attention that might've been more useful elsewhere.

I'll keep this PR a draft until my local builds succeeds, feel free to bless it out of draft mode if the changes are now as they should be and a local build/test works for you.

afh · 2024-07-12T05:53:23Z

@alyssais I was basing the wording off this comment, which states that:

Bisect says 0626ef8 texinfo: refactor broke ffmpeg_4 build on staging as:

alyssais · 2024-07-12T07:14:28Z

Oh, well if it was a refactor that broke it, and not an update, why don't we just undo the refactor?

afh · 2024-07-12T18:32:26Z

Unfortunately this branch fails to build for me on aarch64-darwin due to timeouts in p11-kit tests (test-transport and test-transport3).
I'll look into this on x86_64-linux and double check which commit broke the build refactor or update, if my workstation lets me, that is, and doesn't melt in the process… 😅

afh · 2024-07-16T07:26:16Z

ℹ️ I've requested access to nixos-community builders in order to move forward here, unfortunately my workstation does not seem capable enough to be up for the task.

Atemu · 2024-07-16T10:52:20Z

This is just a regression in once specific component; if it works on Linux, that's good enough for me. If darwin worked before this component broke, it should work again after it's fixed.

Atemu

Builds fine for me on Linux.

The CI failure looks unrelated but let's try again just in case:

@ofborg eval

alyssais · 2024-07-16T12:00:27Z

I'm still confused why we're pinning an old version, when AIUI it used to work with the newer version, until a refactor broke the newer version. Why not just undo that refactor? Or do I misunderstand?

afh · 2024-07-16T12:20:49Z

I now have access to nix-community builder, which should help in getting a better understanding what broke when.

Kindly requesting a bit more patience until I've reported back with my findings 🙂

afh · 2024-07-17T10:47:24Z

In Brief

Important

Updating Texinfo from 7.0.3 to Texinfo 7.1 breaks ffmpeg_4 build.

In Detail

The following steps were done on the nix-community builder build01 in order to pinpoint whether refactoring (0626ef8) or updating (e6f9072) texinfo broke the ffmpeg_4 build:

Checkout 53873ee (ℹ️ This is a commit on master in order to keep build times somewhat reasonable)
git switch --detach 53873ee6c5f9

Build ffmpeg_4 to ensure it builds successfully as expected using texinfo which uses Texinfo 7.0.3:
nix build .#ffmpeg_4
✓ Succeeds

% ls -lsa result-bin/bin/
total 419
  1 dr-xr-xr-x 2 root root      5 Jan  1  1970 .
  1 dr-xr-xr-x 3 root root      3 Jan  1  1970 ..
197 -r-xr-xr-x 2 root root 317864 Jan  1  1970 ffmpeg
105 -r-xr-xr-x 2 root root 165320 Jan  1  1970 ffplay
117 -r-xr-xr-x 2 root root 198304 Jan  1  1970 ffprobe
% ./result-bin/bin/ffmpeg -version | head -2
ffmpeg version 4.4.4 Copyright (c) 2000-2023 the FFmpeg developers
built with gcc 13.3.0 (GCC)

Create pkgs/development/tools/misc/texinfo/7.1.nix:

cat << EOF >pkgs/development/tools/misc/texinfo/7.1.nix
import ./common.nix {
  version = "7.1";
  sha256 = "sha256-3u7J8Z8VngRv34rSIjGYGAbawzLMNy8cdjUErYKzCVM=";
}
EOF
git add -N pkgs/development/tools/misc/texinfo/7.1.nix

Change texinfo7 package to use Texinfo 7.1
sed -i '' -e 's@texinfo/7.0.nix@texinfo/7.1.nix@' pkgs/top-level/all-packages.nix

git diff

diff --git a/pkgs/development/tools/misc/texinfo/7.1.nix b/pkgs/development/tools/misc/texinfo/7.1.nix
new file mode 100644
index 000000000000..dff284eca89b
--- /dev/null
+++ b/pkgs/development/tools/misc/texinfo/7.1.nix
@@ -0,0 +1,4 @@
+import ./common.nix {
+  version = "7.1";
+  sha256 = "sha256-3u7J8Z8VngRv34rSIjGYGAbawzLMNy8cdjUErYKzCVM=";
+}
diff --git a/pkgs/top-level/all-packages.nix b/pkgs/top-level/all-packages.nix
index 5fbca08484c9..a0530d14e118 100644
--- a/pkgs/top-level/all-packages.nix
+++ b/pkgs/top-level/all-packages.nix
@@ -19473 +19473 @@ with pkgs;
-  texinfo7 = callPackage ../development/tools/misc/texinfo/7.0.nix { };
+  texinfo7 = callPackage ../development/tools/misc/texinfo/7.1.nix { };

Build ffmpeg_4 to test whether it builds successfully using texinfo which now uses Texinfo 7.1:
nix build .#ffmpeg_4
✗ Fails with test time out from umockdev-0.18.2
Build ffmpeg_4 again:
nix build .#ffmpeg_4
✗ Fails with > makeinfo: error parsing ./doc/t2h.pm: Undefined subroutine &Texinfo::Config::set_from_init_file called at ./doc/t2h.pm line 24.

Important

This indicates that updating Texinfo from 7.0.3 to 7.1 breaks the ffmpeg_4 build.

Apply changes propsed in the PR locally:
curl -sL https://github.com/NixOS/nixpkgs/pull/326249.patch | patch -p1

git diff pkgs/development/libraries

diff --git a/pkgs/development/libraries/ffmpeg/generic.nix b/pkgs/development/libraries/ffmpeg/generic.nix
index 6d54d63c6ca8..30b43227489e 100644
--- a/pkgs/development/libraries/ffmpeg/generic.nix
+++ b/pkgs/development/libraries/ffmpeg/generic.nix
@@ -1 +1 @@
-{ lib, stdenv, buildPackages, removeReferencesTo, addOpenGLRunpath, pkg-config, perl, texinfo, yasm
+{ lib, stdenv, buildPackages, removeReferencesTo, addOpenGLRunpath, pkg-config, perl, texinfo, texinfo6, yasm
@@ -754 +754,3 @@ stdenv.mkDerivation (finalAttrs: {
-  nativeBuildInputs = [ removeReferencesTo addOpenGLRunpath perl pkg-config texinfo yasm ]
+  nativeBuildInputs = [ removeReferencesTo addOpenGLRunpath perl pkg-config yasm ]
+ # Texinfo version 7 introduced breaking changes, which older versions of ffmpeg do not handle.
+  ++ (if versionOlder version "5" then [ texinfo6 ] else [ texinfo ])

Build ffmpeg_4 to test whether it builds successfully using texinfo which now uses Texinfo 7.1 and the fix proposed here:
nix build .#ffmpeg_4
✓ Succeeds

% ls -lsa result-bin/bin/
total 419
  1 dr-xr-xr-x 2 root root      5 Jan  1  1970 .
  1 dr-xr-xr-x 3 root root      3 Jan  1  1970 ..
197 -r-xr-xr-x 2 root root 317864 Jan  1  1970 ffmpeg
105 -r-xr-xr-x 2 root root 165320 Jan  1  1970 ffplay
117 -r-xr-xr-x 2 root root 198304 Jan  1  1970 ffprobe
% ./result-bin/bin/ffmpeg -version | head -2
ffmpeg version 4.4.4 Copyright (c) 2000-2023 the FFmpeg developers
built with gcc 13.3.0 (GCC)

Important

This shows that the changes proposed in this PR fix the ffmpeg_4 build issues.

Are the steps shown above correct or have I overlooked something that skews the results?
Is there any additional "proof" apart from the writeup above that I can offer (.drv or .log files) that would help folks verify the claims made here?
Would folks prefer if other approaches are presented, e.g. add texinfo 7.1 package, but keep texinfo7 at Texinfo 7.0.3 or try to understand ffmpeg_4 documentation build process and provide a patch to address the Texinfo 7.1 issue?

@alyssais with the information presented above are you on board with keeping the texinfo package refactoring changes and address the ffmpeg_4 build issues as proposed in this PR?

@Atemu with the findings above are you alright with this draft PR being promoted to a proper PR?

alyssais · 2024-07-17T10:59:50Z

@alyssais with the information presented above are you on board with keeping the texinfo package refactoring changes and address the ffmpeg_4 build issues as proposed in this PR?

Yes, and thanks for updating the commit message already!

afh · 2024-07-17T11:52:16Z

Thanks for the quick and affirmative response, @alyssais 🙂

I'd appreciate an official approval if that's something you feel comfortable with, if not that's perfectly alright with me. Your comments have already been very helpful to assess and scrutinise the situation and have helped me gain a better understanding in general and confidence in the refactoring changes and the ones proposed here.

alyssais · 2024-07-17T13:29:04Z

pkgs/development/libraries/ffmpeg/generic.nix

Shouldn't this say version 7.1?

To my knowledge Texinfo version 7 already introduced breaking changes in their Perl API, what we are seeing here with Texinfo version 7.1 and ffmpeg_4 specifically appears to be just a continuation of that.
If you see value in being more specific here, I'll happily amend the version, @alyssais 🙂

Since this is the ffmpeg package, and ffmpeg worked fine with 7.1 (right?), I think 7.1 should be the version mentioned.

Please hold while I double check; be right with you… 🙂

Wasn't it that it was deprecated in 7 and they then removed it in 7.1?

Yes, @alyssais, building the ffmpeg package worked and works fine with texinfo 7.1.

By "it", do you mean the Perl API, @Atemu ? If yes, at first glance it appears to be quite alive to me, see https://www.gnu.org/software/texinfo/manual/texi2any_api/texi2any_api.html

Sorry, I meant the breaking change. IIRC something was deprecated in 7.0 and then removed/broken in 7.1.

Thanks for the clarification, @Atemu, I haven't found a definite answer on that (yet).

What is left to do on this PR in order to move forward? 🙂
Just the comment change requested above?

Change the comment to

Suggested change

# Texinfo version 7 introduced breaking changes, which older versions of ffmpeg do not handle.

# Texinfo version 7.1 introduced breaking changes, which older versions of ffmpeg do not handle.

because that's where it broke. From our perspective, it doesn't really matter when it was deprecated, just when it broke.

Done ✅ 🙂

nixos-discourse · 2024-07-18T18:04:56Z

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

https://discourse.nixos.org/t/prs-already-reviewed/2617/1837

afh · 2024-07-18T19:22:57Z

Glad to see this got merged. Thanks everyone for you help on this, very much appreciated! 🥳

afh mentioned this pull request Jul 11, 2024

(staging) Build failure: ffmpeg_4 #326231

Closed

Atemu reviewed Jul 11, 2024

View reviewed changes

afh changed the title ~~ffmpeg4: fix regression introduced by texinfo refactor~~ ffmpeg_4: fix regression introduced by texinfo refactor Jul 11, 2024

afh force-pushed the fix-ffmpeg4 branch from aa8ba72 to 9fcb0cb Compare July 11, 2024 12:20

afh requested a review from Atemu July 11, 2024 12:21

Atemu requested changes Jul 11, 2024

View reviewed changes

pkgs/development/libraries/ffmpeg/generic.nix Outdated Show resolved Hide resolved

Atemu marked this pull request as draft July 11, 2024 12:55

afh force-pushed the fix-ffmpeg4 branch from 9fcb0cb to a215b85 Compare July 11, 2024 13:15

ofborg bot requested review from Atemu, arthsmn, jopejoe1 and justinas July 11, 2024 13:48

afh force-pushed the fix-ffmpeg4 branch from a215b85 to c559c96 Compare July 12, 2024 05:36

afh mentioned this pull request Jul 16, 2024

nixos/community-builder: add user afh nix-community/infra#1373

Merged

Atemu marked this pull request as ready for review July 16, 2024 11:02

Atemu approved these changes Jul 16, 2024

View reviewed changes

arthsmn approved these changes Jul 16, 2024

View reviewed changes

wegank added the 12.approvals: 2 This PR was reviewed and approved by two persons. label Jul 16, 2024

Atemu marked this pull request as draft July 16, 2024 13:49

afh force-pushed the fix-ffmpeg4 branch from c559c96 to 22a6f77 Compare July 17, 2024 10:00

afh force-pushed the fix-ffmpeg4 branch from 22a6f77 to 5caf06a Compare July 17, 2024 10:57

afh changed the title ~~ffmpeg_4: fix regression introduced by texinfo refactor~~ ffmpeg_4: fix build issue introduced by texinfo 7.1 update Jul 17, 2024

ofborg bot requested review from Atemu and arthsmn July 17, 2024 11:35

afh mentioned this pull request Jul 17, 2024

Refactor texinfo and update 7.0.3 -> 7.1 #289690

Merged

19 tasks

7c6f434c approved these changes Jul 17, 2024

View reviewed changes

alyssais reviewed Jul 17, 2024

View reviewed changes

ffmpeg_4: fix build issue introduced by texinfo 7.1 update

1e07a3b

afh force-pushed the fix-ffmpeg4 branch from 5caf06a to 1e07a3b Compare July 18, 2024 17:32

afh marked this pull request as ready for review July 18, 2024 17:32

Atemu approved these changes Jul 18, 2024

View reviewed changes

alyssais approved these changes Jul 18, 2024

View reviewed changes

afh added 12.approvals: 3+ This PR was reviewed and approved by three or more persons. and removed 12.approvals: 2 This PR was reviewed and approved by two persons. labels Jul 18, 2024

arthsmn approved these changes Jul 18, 2024

View reviewed changes

ofborg bot requested review from Atemu and arthsmn July 18, 2024 18:42

Atemu merged commit 04df288 into NixOS:staging Jul 18, 2024

afh deleted the fix-ffmpeg4 branch July 18, 2024 19:21

	nativeBuildInputs = [ removeReferencesTo addDriverRunpath perl pkg-config texinfo yasm ]
	nativeBuildInputs = [ removeReferencesTo addDriverRunpath perl pkg-config yasm ]

	++ (if lib.versionOder "5" then [ texinfo6 ] else [ texinfo ]);
	++ (if lib.versionOlder "5" then [ texinfo6 ] else [ texinfo ]);

	# Texinfo version 7 introduced breaking changes, which older versions of ffmpeg do not handle.
	# Texinfo version 7.1 introduced breaking changes, which older versions of ffmpeg do not handle.

Uh oh!

Conversation

afh commented Jul 11, 2024

Description of changes

Things done

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Atemu commented Jul 11, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

afh commented Jul 11, 2024

Uh oh!

Atemu left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

alyssais commented Jul 11, 2024

Uh oh!

afh commented Jul 12, 2024

Uh oh!

afh commented Jul 12, 2024

Uh oh!

alyssais commented Jul 12, 2024

Uh oh!

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

Uh oh!

Uh oh!

afh commented Jul 16, 2024

Uh oh!

Atemu commented Jul 16, 2024

Uh oh!

Atemu left a comment

Choose a reason for hiding this comment

Uh oh!

alyssais commented Jul 16, 2024

Uh oh!

afh commented Jul 16, 2024

Uh oh!

afh commented Jul 17, 2024

In Brief

In Detail

Uh oh!

alyssais commented Jul 17, 2024

Uh oh!

afh commented Jul 17, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nixos-discourse commented Jul 18, 2024

Uh oh!

afh commented Jul 18, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

afh commented Jul 12, 2024 •

edited

Loading