Skip to content

rav1e: remove build-time dependency on libgit2#355325

Merged
emilazy merged 2 commits intoNixOS:masterfrom
tjni:rav1e-remove-libgit
Nov 12, 2024
Merged

rav1e: remove build-time dependency on libgit2#355325
emilazy merged 2 commits intoNixOS:masterfrom
tjni:rav1e-remove-libgit

Conversation

@tjni
Copy link
Contributor

@tjni tjni commented Nov 12, 2024

It has a dependency on an old, vendored version of libgit2, which was causing some pain for work updating to clang 19. When investigating, we noticed that it only seems to use libgit2 at build-time to encode and later print out the commit hash as part of the version string. This can be disabled from the list of default features.

The main difference from what I can see is that the current version is printed out as:

❯ result/bin/rav1e --version
rav1e 0.7.1 () (release)
rustc 1.82.0 (f6e511eec 2024-10-15) (built from a source tarball) aarch64-apple-darwin
Compiled CPU Features: aes,crc,dit,dotprod,dpb,dpb2,fcma,fhm,flagm,fp16,frintts,jsconv,lor,lse,neon,paca,pacg,pan,pmuv3,ras,rcpc,rcpc2,rdm,sb,sha2,sha3,ssbs,vh
Runtime Assembly Support: Enabled
Runtime Assembly Level: NEON
Threading: Enabled
Unstable Features: Disabled
Compiler Flags: -Ctarget-feature=-crt-static

while afterward we have:

❯ result/bin/rav1e --version
rav1e 0.7.1 (UNKNOWN) (release)
rustc 1.82.0 (f6e511eec 2024-10-15) (built from a source tarball) aarch64-apple-darwin
Compiled CPU Features: aes,crc,dit,dotprod,dpb,dpb2,fcma,fhm,flagm,fp16,frintts,jsconv,lor,lse,neon,paca,pacg,pan,pmuv3,ras,rcpc,rcpc2,rdm,sb,sha2,sha3,ssbs,vh
Runtime Assembly Support: Enabled
Runtime Assembly Level: NEON
Threading: Enabled
Unstable Features: Disabled
Compiler Flags: -Ctarget-feature=-crt-static

Note that we don't even currently get the git revision, probably because we fetch from crate.io instead of from GitHub while keeping the .git directory intact.

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.

@tjni tjni marked this pull request as draft November 12, 2024 03:09
@tjni tjni force-pushed the rav1e-remove-libgit branch from 4f57f07 to 8750c8c Compare November 12, 2024 04:37
@tjni tjni marked this pull request as ready for review November 12, 2024 04:37
@tjni tjni force-pushed the rav1e-remove-libgit branch from 8750c8c to c4a4f95 Compare November 12, 2024 04:38
Copy link
Contributor

@paparodeo paparodeo left a comment

Choose a reason for hiding this comment

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

LGTM -- maybe target llvm-19 branch

Copy link
Member

@emilazy emilazy left a comment

Choose a reason for hiding this comment

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

Will handle the merge myself.

@emilazy emilazy merged commit 18e6680 into NixOS:master Nov 12, 2024
@paparodeo
Copy link
Contributor

Will handle the merge myself.

this triggers 1001 -> 2500 rebuilds. #355124 so shouldn't be in master.

@emilazy
Copy link
Member

emilazy commented Nov 12, 2024

Fixed in #355456, sorry. Will reapply on llvm-19. I didn’t see any big red labels but it turns out that ofborg took over 10 hours to even get to the eval :/

@paparodeo
Copy link
Contributor

Fixed in #355456, sorry. Will reapply on llvm-19. I didn’t see any big red labels but it turns out that ofborg took over 10 hours to even get to the eval :/

yeah, I could've mentioned it it my approval message

@tjni
Copy link
Contributor Author

tjni commented Nov 12, 2024

Sorry about that! I didn’t anticipate so many dependencies but this library is popular!

@tjni tjni deleted the rav1e-remove-libgit branch November 12, 2024 16:15
@emilazy
Copy link
Member

emilazy commented Nov 12, 2024

I think it’s via FFmpeg, great aggregator of mass rebuilds :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants