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

migrate away from ffmpeg_3 #120705

Closed
dotlambda opened this issue Apr 26, 2021 · 40 comments
Closed

migrate away from ffmpeg_3 #120705

dotlambda opened this issue Apr 26, 2021 · 40 comments
Labels
1.severity: security Issues which raise a security issue, or PRs that fix one 3.skill: good-first-bug This seems like it's fixable by a non-expert
Milestone

Comments

@dotlambda
Copy link
Member

dotlambda commented Apr 26, 2021

ffmpeg_3 has many open vulnerabilities (see #94003 and #120372). There seems to be no effort to add patches for these, so we should drop ffmpeg_3 or at least mark it as insecure.
In #89264, ffmpeg_3 was made the de facto default by making every package that depends on ffmpeg depend on ffmpeg_3 instead. I think that was a bad idea given that the Ffmpeg packages aren't well maintained.
Most packages should build just fine with ffmpeg but someone needs to test them.

Is there an easy way to obtain a list of packages using ffmpeg_3 and ping their maintainers?

cc @doronbehar @codyopel

Here's a list of affected packages:

Please remove the list of maintainers from packages that are done because GitHub won't allow me to ping more than a certain number of people.

script that I used to generate this list

The manually obtained file packages contains one attribute per line.

pkgs=$(cat packages)
for pkg in $pkgs; do
    pings=$(nix eval "(with import ./. { }; lib.concatStringsSep \" \" (map (m: \"@\" + m.github) ($pkg.meta.maintainers or [ ])))" --raw)
    if [ -z "$pings" ]; then
        echo "- [ ] $pkg" >> packages-with-maintainers
    else
        echo "- [ ] $pkg ($pings)" >> packages-with-maintainers
    fi
done
@dotlambda dotlambda added the 1.severity: security Issues which raise a security issue, or PRs that fix one label Apr 26, 2021
@Radvendii
Copy link
Contributor

Radvendii commented Apr 26, 2021

Ring daemon is already broken. You're welcome to switch the ffmpeg version, and if I ever get around to fixing that package, I'll make sure it works (or downgrade the ffmpeg version if absolutely necessary)

I can do it myself, but I'm assuming it's better to do one large PR changing as many as possible at once, than a million tiny PRs. Let me know if that's an incorrect assumption, and I should bump the ffmpeg version in ring-daemon

@dotlambda
Copy link
Member Author

dotlambda commented Apr 26, 2021

Ring daemon is already broken. You're welcome to switch the ffmpeg version, and if I ever get around to fixing that package, I'll make sure it works (or downgrade the ffmpeg version if absolutely necessary)

If a package is broken for a long time, we should consider removing it. But given that you want to fix it at some point, I guess we should keep this one.

I can do it myself, but I'm assuming it's better to do one large PR changing as many as possible at once, than a million tiny PRs. Let me know if that's an incorrect assumption, and I should bump the ffmpeg version in ring-daemon

It's easier to do in small PRs because nobody will be able to test whether all these packages still work after switching to a newer Ffmpeg.

expipiplus1 added a commit to expipiplus1/nixpkgs that referenced this issue Apr 26, 2021
Drop ffmpeg_3 dependency in favor of ffmpeg: NixOS#120705
@magnetophon
Copy link
Member

Ardour builds and runs with regular ffmpeg.
A new release is expected any day now, is it OK if I fix it in the PR for that?

@andrewrk
Copy link
Member

FYI I'm working on libgroove upstream to bump the dependency. If we want to remove libgroove from nix and resubmit it later when I've completed that work that would be fine with me. Afaik the only package that uses it is groove basin, which I am also the upstream author of and also in the (long) process of rebooting it. IMO these packages should be removed from nixpkgs for now and I will resubmit them in the future.

AluisioASG added a commit to AluisioASG/nixpkgs that referenced this issue Apr 26, 2021
See NixOS#120705 for the rationale.
ffmpeg-python doesn't seem to be strongly bound to any ffmpeg version
so we can just migrate away.
AluisioASG added a commit to AluisioASG/nixpkgs that referenced this issue Apr 26, 2021
@AndersonTorres
Copy link
Member

AndersonTorres commented Apr 26, 2021

Ardour builds and runs with regular ffmpeg.
A new release is expected any day now, is it OK if I fix it in the PR for that?

If it is a security update, then it is better to fix it now and backpropagate to current stable releases.

If upstream Ardour sees no update in one week, update it regardless.

@AndersonTorres
Copy link
Member

FYI I'm working on libgroove upstream to bump the dependency. If we want to remove libgroove from nix and resubmit it later when I've completed that work that would be fine with me. Afaik the only package that uses it is groove basin, which I am also the upstream author of and also in the (long) process of rebooting it. IMO these packages should be removed from nixpkgs for now and I will resubmit them in the future.

Is this a distant future? If it is broken but you are notifying us that it will be fixed, therefore I would suggest mark meta.broken = true; followed with a comment upstream notified a new version is coming or something like that.

Ma27 added a commit to Ma27/nixpkgs that referenced this issue Apr 27, 2021
See also NixOS#120705. `ffmpeg` is only used to search for metadata and
chapters in video files (such as `.mkv`, `.mp4` etc.). Confirmed that
with both `ffmpeg` versions the same result is provided.
@Ma27 Ma27 mentioned this issue Apr 27, 2021
10 tasks
@ScarletHg
Copy link
Member

I cannot submit a PR in a reasonable amount of time due to my current employer, but I can confirm that get_iplayer version 3.27 (SHA-256: 077y31gg020wjpx5pcivqgkqawcjxh5kjnvq97x2gd7i3wwc30qi) builds and smoke tests fine using the ffmpeg package.

If someone would like to submit a PR updating the version and making the ffmpeg change, I would appreciate it greatly. ❤️

@minijackson
Copy link
Member

Opened an issue for Carla here: falkTX/Carla#1403

@peterhoeg
Copy link
Member

I took the liberty of sorting the list to make it easier to read. I don't suggest we keep doing that - it was simply to get the the ones not done yet in order.

@samuela
Copy link
Member

samuela commented Jun 16, 2021

It looks like PRs for capture and mediatomb have been merged now.

@eduardosm
Copy link
Contributor

dr14_tmeter can be marked as done (#125241)

@hrdinka
Copy link
Contributor

hrdinka commented Jun 19, 2021

attract-mode was fixed as well in #126354

@flexagoon
Copy link
Contributor

tvheadend was already fixed in #135662

@wackbyte
Copy link
Contributor

libvdpau-va-gl was fixed in #123757

@thiagokokada
Copy link
Contributor

I went ahead and merged all remaining PRs that were already opened pointing on this issue.

@vs49688
Copy link
Contributor

vs49688 commented Jan 19, 2022

Related:
#155537
#155539

@ajs124 ajs124 mentioned this issue Jan 21, 2022
13 tasks
@ajs124
Copy link
Member

ajs124 commented Jan 21, 2022

After #155993 only grass and natron will be left, for both of which exist open PRs (#150286 and #121212 (nice)), but they sadly both seem to be stalled.

@jonringer
Copy link
Contributor

grass has been merged

@ajs124 ajs124 mentioned this issue Mar 9, 2022
13 tasks
@dasJ dasJ modified the milestones: 21.05, 22.05 Mar 22, 2022
@SuperSandro2000
Copy link
Member

and we are done here. Thank you everyone!

@dotlambda
Copy link
Member Author

and we are done here

What about natron?

@dotlambda
Copy link
Member Author

Once this is actually done, ffmpeg_3 should be removed from Nixpkgs.

@dotlambda dotlambda reopened this Mar 27, 2022
@SuperSandro2000
Copy link
Member

SuperSandro2000 commented Mar 27, 2022

We did that in #163509 I just forgot to tick the box.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1.severity: security Issues which raise a security issue, or PRs that fix one 3.skill: good-first-bug This seems like it's fixable by a non-expert
Projects
Archived in project
Development

No branches or pull requests