chromaprint: add patch for FFmpeg 7#331795
Merged
NickCao merged 1 commit intoNixOS:stagingfrom Aug 12, 2024
Merged
Conversation
Member
Author
|
@NickCao I see we had the same idea :) |
ofborg
bot
added
10.rebuild-darwin: 11-100
Member
Let's try dropping that. |
Member
Author
|
Not so good an idea, apparently: I guess it prefers the Apple thing for FFT but still wants FFmpeg for other things. |
NickCao
reviewed
Aug 3, 2024
Member
NickCao
left a comment
NickCao
left a comment
There was a problem hiding this comment.
Arch is pulling in another patch for ffmpeg 7: https://gitlab.archlinux.org/archlinux/packaging/packages/chromaprint/-/blob/main/ffmpeg-7.patch
This builds successfully with FFmpeg 7 and so do the various full FFmpeg builds that depend on it. Not tested beyond that as the number of rebuilds is of course very large.
emilazy
force-pushed
the
push-pqtlslzywrpk
branch
from
0b40cf3 to
9dddba4
Compare
emilazy
changed the title
Member
Author
|
I thought it was bad enough how much FFmpeg breaks ABI compatibility, but now you’re telling me they have stringly‐typed APIs that break compatibility without any compile‐time indication?! Pushed the patch. |
Member
|
Running nixpkgs-review now. Last time we tried this it went smoothly, hopefully the same this time! Edit: amdgpu decided to die halfway again, I gave up. |
Member
|
Oh and regarding ninja, AFAIK it might not actually speed up the build compared to gnumake, ninja's strength is incremental compilation, not from scratch. |
Member
Author
|
My experience is that it speeds up large from‐scratch builds meaningfully. Probably not relevantly so in this case, but Make is janky in general. Ideally our CMake hook would just propagate Ninja by default. |
Member
In #331858 @AndersonTorres is thinking of detaching hooks from the packages themselves, maybe a |
Member
I tend to disagree. Our Meson hook does not propagate Ninja by default. |
Member
Author
|
Emitting Xcode is not really useful for Nix, so I don’t see that as a very compelling argument as to how the hooks should work, especially if the hooks were split off from the build system generator packages themselves. CMake supports Makefiles, Ninja, Visual Studio, Xcode, and whatever “Green Hills MULTI” is. Of those, Makefiles and Ninja are the only ones likely to ever be used in a Nix derivation using the CMake hook to drive the build, and wherever Ninja works it’s preferable. (There are a few CMake edge‐case antipatterns that can make Ninja builds not work properly, so of course there would have to be a fallback.) |
Member
Author
|
On the other hand, if you had to explicitly write |
Member
It was an example that came to my mind at that moment. Further, we do not really know if and when this or that backend can be useful in the future.
It implies two things:
This is a historical baggage CMake respects. Indeed the gmake backend is arguably more tested. Further, GNU Make comes for free because of our default |
Member
Author
|
I don’t find any of your arguments convincing.
Yes, it’s not unreasonable to support such bugs to upstream users of CMake. As you said, CMake is intended to be backend‐agnostic; the few things that can cause Ninja builds to not work correctly are caused by incorrect CMake files. The vast majority of CMake packages work just fine, and build faster, with Ninja. Opting out for the few that don’t would be better than the status quo, and as simple as
The situation is not analogous. Meson does not support a build tool that is in stdenv by default, so basically every single use explicitly enumerates We already impose a default; it’s just an inferior one. A world in which we impose no default at all may be better – although I’d argue that the use cases that want other generators are sufficiently niche that it’s by no means a clear win – but is not practical because of stdenv backwards‐compatibility limitations.
That is the historical baggage I refer to. It’s not a good thing. There’s no reason stdenv should privilege @NickCao Any luck with the |
Member
|
Although this being too off-topic, I want to vent some extra things.
My point is to not choose any winner. Winners always come and winners always go. Indeed Python 2 was one of those superior winners that fade away - and because of it we no longer distribute Palemoon from sources. But I digress...
IMHO decoupling Meson from Ninja was a fortuitously correct design decision, and it should be set as the example, despite the historical context that lead to this. In other words, "We do this way because it was not possible to do otherwise in the distant past" is not relevant.
My point is we should not impose a default, even a superior one. If Cmake+Ninja is really superior, it is not hard to write something like
Ehr... What if NixOS project started today? It would not be an overstatement to say anything in the open source territories depends on GCC, in terms of indirect/bootstrap dependencies.
And GCC will not abandon Autotools in any reasonably short amount of time - despite themselves recognizing the drawbacks. So, I think that a preliminary version of NixOS written today from scratch would deal with But this is going to be too off-topic. |
Member
Author
|
Result of 113 packages marked as broken and skipped:
1 package blacklisted:
106 packages failed to build:
2783 packages built:
My regressions‐finding script says: of which |
Member
|
I still prefer dropping ninja, and we should test darwin builds. Overall looks good! |
Member
Author
|
I’ve confirmed that The build is consistently ~3 s faster with NInja: I can remove it if you really don’t like it, but I think it’s a clear improvement to use a more tightly‐scoped, faster build tool that has better semantics. The CMake developers themselves have even considered changing the default. |
Member
Great!
3s isn't that much, but let's stop the bike shedding here. chromaprint is quite deep down the dependency chain thus bringing in ninja to the build closure (which it most likely is already in) is not a big problem.
Hopefully this do come true someday. |
NickCao
approved these changes
Aug 12, 2024
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description of changes
This builds successfully with FFmpeg 7 and so do the various full FFmpeg builds that depend on it. Not tested beyond that as the number of rebuilds is of course very large.
By the way, I’m not sure this needs to depend on FFmpeg on macOS.
Things done
nix.conf? (See Nix manual)sandbox = relaxedsandbox = truenix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)Add a 👍 reaction to pull requests you find important.