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

Jump to bottom

ffmpeg: default to ffmpeg_4 #89264

Merged
merged 2 commits into from Jun 12, 2020
Merged

ffmpeg: default to ffmpeg_4 #89264

merged 2 commits into from Jun 12, 2020

Conversation

doronbehar
Copy link
Contributor

@doronbehar doronbehar commented May 31, 2020 •
edited

Motivation for this change

Libraries without a version suffix should point to their latest version upstream! See https://discourse.nixos.org/t/nixpkgs-policy-regarding-libraries-available-in-multiple-versions/7026

This PR intends to make ffmpeg point to ffmpeg4. To make the review easier, I renamed the inputs where necessary for all derivations that used ffmpeg (without a version suffix) and as seen by the PR's labels, ofborg didn't detect any paths changed, besides of course ffmpeg itself.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@doronbehar
Copy link
Contributor Author

@GrahamcOfBorg eval

@doronbehar doronbehar force-pushed the ffmpeg=4 branch 4 times, most recently from a51fc88 to 1267463 Compare June 7, 2020 13:16
@doronbehar doronbehar marked this pull request as ready for review June 7, 2020 14:05
@doronbehar
ffmpeg: default to ffmpeg_4
5bfd3b1
@doronbehar
treewide: use ffmpeg_3 explicitly if not wanted otherwise
f439584 
After making `ffmpeg` point to the latest `ffmpeg_4`, all packages that
used `ffmpeg` without requiring a specific version now use ffmpeg_3
explicitly so they shouldn't change.
AluisioASG
AluisioASG approved these changes Jun 12, 2020
View reviewed changes
Copy link
Contributor

@AluisioASG AluisioASG left a comment

Choose a reason for hiding this comment

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

As far as I can tell, r128gain works with ffmpeg 4. Should we change it ourselves back when we update our packages?

@doronbehar
Copy link
Contributor Author

As far as I can tell, r128gain works with ffmpeg 4. Should we change it ourselves back when we update our packages?

If you've meant that you should do that in a separate PR then yes, but I don't want to do include this change in this PR. The goal is to reduce the scope as much as possible as it's hard enough to get such a change in Nixpkgs (not to mention the amount of time it took me to s/ffmpeg/ffmpeg_3/g in all the files in a way that ofborg won't detect changes).

The general idea, is that when a package maintainer will come to edit their package expression, it will hopefully come to their attention that they use an old version of ffmpeg as an input.

@AluisioASG
Copy link
Contributor

That's what I wanted to know, thank you.

jonringer
jonringer approved these changes Jun 12, 2020
View reviewed changes
Copy link
Contributor

@jonringer jonringer left a comment

Choose a reason for hiding this comment

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

diff and package set difference LGTM

[1 built, 1 copied (1.1 MiB), 0.2 MiB DL]
https://github.com/NixOS/nixpkgs/pull/89264
1 package built:
ffmpeg

@jonringer
Copy link
Contributor

jonringer commented Jun 12, 2020 •
edited

this PR has been open for 12 days, which I think was ample time for people to voice any strong opposition to this

@jonringer jonringer merged commit 01d4e2f into NixOS:master Jun 12, 2020
@doronbehar
Copy link
Contributor Author

🎉 Thanks!

@doronbehar doronbehar mentioned this pull request Jun 19, 2020
Merged
10 tasks
@doronbehar doronbehar mentioned this pull request Sep 24, 2020
Closed
10 tasks
@doronbehar doronbehar mentioned this pull request Oct 7, 2020
Closed
10 tasks
@dotlambda dotlambda mentioned this pull request Apr 26, 2021
Closed
@doronbehar doronbehar deleted the ffmpeg=4 branch March 2, 2023 10:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sikmir sikmir sikmir approved these changes

@AluisioASG AluisioASG AluisioASG approved these changes

@jonringer jonringer jonringer approved these changes

@astsmtl astsmtl Awaiting requested review from astsmtl

@gebner gebner Awaiting requested review from gebner

@symphorien symphorien Awaiting requested review from symphorien

@cdepillabout cdepillabout Awaiting requested review from cdepillabout

@FRidh FRidh Awaiting requested review from FRidh

@ttuegel ttuegel Awaiting requested review from ttuegel ttuegel is a code owner

Assignees
No one assigned
Labels
6.topic: haskell 6.topic: python 6.topic: qt/kde 10.rebuild-darwin: 1-10 10.rebuild-darwin: 1 10.rebuild-linux: 1-10 10.rebuild-linux: 1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@doronbehar @AluisioASG @jonringer @sikmir