Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ffmpeg_4: almost drop #336401

Merged
merged 17 commits into from
Aug 27, 2024
Merged

ffmpeg_4: almost drop #336401

merged 17 commits into from
Aug 27, 2024

Conversation

emilazy
Copy link
Member

@emilazy emilazy commented Aug 21, 2024

Description of changes

Bump almost all of the remaining dependencies on FFmpeg 4, and add a comment about its legacy status so that we can discourage further proliferation and hopefully remove it at some point.

This PR is stacked on top of the following others; please ensure any remaining PRs are reviewed and merged before landing this one:

All of the package changes that are in any way tricky or potentially controversial were split out into the above PRs. This home straight consists entirely of trivial changes of FFmpeg pins, bumps between upstream versions of the same stability level, and clean backports of simple patches that have already been committed upstream. Everything here has been confirmed to build.

After this PR and the linked ones, the remaining packages depending on ffmpeg_4 will be:

  • reaper (binary package; dependent on upstream’s whims)

  • spotify (binary package; dependent on upstream’s whims)

  • rocmPackages_{5,6}.mivisionx (both broken for a long time; maybe they’ll update before they become unbroken!)

This turned out to be a bigger project than I thought it would be when I started out, but 34 PRs later, I’m glad to finally see it (almost) completed.

Once all of these PRs are merged, I am going to bump ffmpeg to 7 on staging, and the real fun can begin.

Result of nixpkgs-review run on aarch64-linux 1

1 package marked as broken and skipped:
  • qstopmotion
1 package failed to build:
  • alephone-apotheosis-x
27 packages built:
  • alephone
  • alephone-durandal
  • alephone-eternal
  • alephone-evil
  • alephone-infinity
  • alephone-marathon
  • alephone-pathways-into-darkness
  • alephone-pheonix
  • alephone-red
  • alephone-rubicon-x
  • alephone-yuge
  • alephone.icons
  • ardour
  • ardour_7
  • electricsheep
  • guvcview
  • libsForQt5.ffmpegthumbs
  • octavePackages.video
  • olive-editor
  • pangolin
  • rustplayer
  • sumo
  • ultrastardx
  • untrunc-anthwlock
  • vulkan-cts
  • webcamoid
  • xjadeo

Result of nixpkgs-review run on x86_64-linux 1

1 package failed to build:
  • alephone-apotheosis-x
30 packages built:
  • alephone
  • alephone-durandal
  • alephone-eternal
  • alephone-evil
  • alephone-infinity
  • alephone-marathon
  • alephone-pathways-into-darkness
  • alephone-pheonix
  • alephone-red
  • alephone-rubicon-x
  • alephone-yuge
  • alephone.icons
  • ardour
  • ardour_7
  • electricsheep
  • guacamole-server
  • guvcview
  • libsForQt5.ffmpegthumbs
  • megasync
  • octavePackages.video
  • olive-editor
  • pangolin
  • qstopmotion
  • rustplayer
  • sumo
  • ultrastardx
  • untrunc-anthwlock
  • vulkan-cts
  • webcamoid
  • xjadeo

Result of nixpkgs-review run on aarch64-darwin 1

1 package marked as broken and skipped:
  • sumo
3 packages built:
  • octavePackages.video
  • pangolin
  • untrunc-anthwlock

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.11 Release Notes (or backporting 23.11 and 24.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.

Add a 👍 reaction to pull requests you find important.

@emilazy emilazy force-pushed the push-ssnkmyzwzysn branch from fe8deea to 4357940 Compare August 21, 2024 22:05
emilazy added 12 commits August 22, 2024 15:22
Builds fine with FFmpeg 7, and Arch ships it like that.
This builds with FFmpeg 6, but still needs updating for FFmpeg 7.
This builds fine with the newer version. There are apparently no
plans to support FFmpeg 7.
Includes fixes for FFmpeg 7.
Includes fixes for FFmpeg 7.
Includes fixes for FFmpeg 6. This is another `ffmpeg-sys-next` fork
situation that is going to be a pain when we bump to 7.
@emilazy emilazy force-pushed the push-ssnkmyzwzysn branch from 4357940 to 778806c Compare August 22, 2024 14:23
@emilazy
Copy link
Member Author

emilazy commented Aug 24, 2024

This is now just pending on the libresample upstream cutting a release with my merged Meson build system patch and hopefully the other PR to fix the tests, and figuring out what to do about tvheadend.

If I don’t hear back from the libresample upstream within the next couple of days, I’ll rebase my PR on top of the Git HEAD and keep the patch to fix the tests, and I can come back and bump it whenever there’s a proper tag. Similarly, if there’s no response to #332259 (comment) I’ll merge #336395. Then this will be ready and I can make the FFmpeg 7 switch happen.

Copy link
Contributor

@SigmaSquadron SigmaSquadron left a comment

Choose a reason for hiding this comment

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

Should the 24.11 release notes be edited to no longer recommend pinning ffmpeg_4?

@emilazy
Copy link
Member Author

emilazy commented Aug 24, 2024

I’m planning to fold the FFmpeg 5 removal note into the one about FFmpeg 7 becoming the default, and add more warnings about FFmpeg 4 there when I do that PR, since otherwise I’d be writing something about FFmpeg versions just to rewrite it again a few days from now. (And, anyway, we’ll still have a couple binary packages pinning ffmpeg_4, and it’s not the end of the world if out‐of‐tree users do it, so long as they know it’s on borrowed time, so I think “if necessary” is about strong enough as it stands.)

@emilazy
Copy link
Member Author

emilazy commented Aug 27, 2024

With all the other PRs dealt with, this is now ready for review and merge 🎉

@emilazy
Copy link
Member Author

emilazy commented Aug 27, 2024

By the way, we could actually drop ffmpeg_4-full already. That would save Hydra some build cycles and allow some of the conditionals in the FFmpeg package to be dropped. Not sure if it’s worth it over just waiting for Spotify and REAPER to update though. (Whenever that happens…)

@SuperSandro2000 SuperSandro2000 merged commit 58bd902 into NixOS:master Aug 27, 2024
25 checks passed
@emilazy emilazy deleted the push-ssnkmyzwzysn branch August 28, 2024 03:59
@Atemu
Copy link
Member

Atemu commented Aug 28, 2024

@SuperSandro2000 could you not unassign maintainers of the package the PR is about and merge without notice? Thanks.

@SuperSandro2000
Copy link
Member

Often people are assigned by accident to PRs because they don't know that we only really use reviews. From the thread there is no clue about the assigned should be doing and the PR is not about ffmpeg which you maintain but about usages which didn't where updated in the last years.

@emilazy
Copy link
Member Author

emilazy commented Aug 28, 2024

@SuperSandro2000 I am also one of the FFmpeg maintainers, precisely as a result of this months‐long project to get rid of old versions. You think I accidentally pinged my co‐maintainers to the latest PR in my long‐running FFmpeg project? I’m sorry, but assuming I made some beginner’s mistake in asking for review here with the amount of care and effort I’ve put into this project is just incredibly condescending. (Well, okay, yes: it was accidental that I added them as assignees and not reviewers, but what does that matter? If that’s a big deal, you could remove them as assignees and add them back as reviewers instead.)

It’s very much about FFmpeg – look at the title! – and contains an FFmpeg policy decision that I specifically wanted their sign‐off on, not to mention my talk of potentially dropping one of the FFmpeg packages in the comment immediately before your merge without comment. Did you even read the PR? I’ve been coordinating this whole project with them. Can you just admit you screwed up here rather than deflecting and making excuses…?

@Atemu @jopejoe1 I am sorry about this, and of course still open to any feedback here.

@SuperSandro2000
Copy link
Member

SuperSandro2000 commented Aug 28, 2024

Oh well derp sorry.
You seem undecided whether to drop off ffmpeg4-full already in this PR and since you wrote before that this is ready to be merged, I just did that.
I guess we drop it in another PR now. 😅

@Atemu
Copy link
Member

Atemu commented Aug 29, 2024

Sandro, all of us have been Nixpkgs maintainers for many years. We know how things work around here. It's true that we don't use assignment often (we really should) but if we do, the meaning should be very obvious: The people assigned are responsible for whatever they're assigned to and thereby "own" it. In this case, Emily didn't appear to technically mean to do that but something slightly different but the intention would have been the same.
If someone else is assigned to some task at your $work, you don't go around and take over their task without asking either, do you?

I think that we should make these rules more explicit for clarity but don't you think it's at least worth asking when a situation is, in your opinion, unclear?

As a maintainer, I feel disregarded here and I regret showing support of restoring your merge privilege a little.

For the record, I generally agree that we should depend on ffmpeg_4 as little as possible but I wouldn't go as far as banning its use in source packages. I'd prefer if people attempted to patch their packages for compat but it shouldn't block the package's inclusion (SHOULD) as I think that, while it's reasonable to ask people to patch their package in such a way, you cannot expect them to do so. Emily went above and beyond to patch these packages in a way we cannot expect any maintainer to do so IMHO.

@emilazy
Copy link
Member Author

emilazy commented Aug 29, 2024

I can agree that it’s more of a SHOULD than a MUST. That said, in my experience the packages that actually needed patching (rather than just updating or undoing a pin) were almost universally ones that were pretty crusty and hadn’t seen any upstream activity in many years. (The exceptions were mostly packages that seemed to be moving away from using FFmpeg at all, or stuff like Qt WebEngine – though even there it was a symptom of WebEngine being based on an old Chromium and needing the FFmpeg 7 patches from Chromium upstream backporting.)

My assessment was that it would be questionable to add those packages that needed non‐trivial work in 2024. We couldn’t add a package that depends on versions of FFmpeg older than 4 without patching, but even with patching I think it would give us pause – do we really want to add something to the tree that it seems clear off the bat is not going to get any upstream maintenance to deal with any future issues? As a result it seemed unlikely to me that there’d be a compelling new source package that can’t support FFmpeg 6+, and that we shouldn’t add more baggage that will make it harder to drop the version when the lagging binary packages finally catch up.

I’m happy to reword the message if you think I could capture those nuances better, or discuss it further if there’s still disagreement.

@emilazy
Copy link
Member Author

emilazy commented Aug 29, 2024

Another bit of colour I should add is that many of the packages could easily have been bumped from FFmpeg 4 to FFmpeg 6 with just an unpinning and perhaps an update – part of why this was so much effort is that I tried to get things on the newer FFmpeg 7 if possible to smooth over the upcoming default version migration rather than turning 6 into the new 4. So since depending on FFmpeg 6 for a new package is reasonable (if perhaps suboptimal), even a large portion of the packages that required some active work on my part would have been able to be added without any issues or patching.

@emilazy emilazy mentioned this pull request Sep 26, 2024
13 tasks
@emilazy emilazy mentioned this pull request Nov 20, 2024
13 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

6 participants