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

handbrake: 1.6.1 -> 1.7.3 #297984

Merged
merged 1 commit into from
Mar 25, 2024
Merged

Conversation

uninsane
Copy link
Contributor

@uninsane uninsane commented Mar 22, 2024

Description of changes

  • fixes existing build break
  • upstream changelogs: https://github.com/HandBrake/HandBrake/releases
    • gtk component is ported from autotools to meson
    • upstream mentions a new library, libdovi: i haven't acted on this; seems to be optional
  • tested on x86_64-linux:
    • nix build '.#handbrake.tests.basic-conversion'
    • checked that both the gui ('.#handbrake') and headless (useGtk = false) versions build
    • transcoded a short H.264 video through the GUI
    • did not test darwin build

Closes #268104.

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

@uninsane uninsane force-pushed the pr-handbrake-1.7.3 branch 2 times, most recently from adaa3c2 to e7f5296 Compare March 22, 2024 13:48
@khaneliman
Copy link
Contributor

Result of nixpkgs-review pr 297984 run on aarch64-darwin 1

1 package failed to build:
  • handbrake

@khaneliman
Copy link
Contributor

khaneliman commented Mar 23, 2024

Looks like darwin requires xcrun added to nativeBuildInputs.
Something like this probably.

++ lib.optionals (stdenv.isDarwin) [
    xcbuild.xcrun
  ]

EDIT: looks like it needs more. i'll try and fix it up for darwin and paste my patch here for you.

xcrun metal -O2 -std=macos-metal2.4 -c -frecord-sources ../libhb/platform/macosx/shaders/bwdif_vt.metal -o libhb/platform/macosx/shaders/bwdif_vt.air;
       > /nix/store/hbxknmypkd2lb0az1mvalxv99qs6yk2w-xcodebuild-0.1.2-pre/bin/xcrun: line 42: exec: metal: not found

@wegank
Copy link
Member

wegank commented Mar 23, 2024

If metal is required for the build, this means that upstream now has Xcode as a non-free dependency, so the package should be marked as broken on darwin.

@@ -26,7 +26,7 @@
, numactl
, writeText
# Processing, video codecs, containers
, ffmpeg_5-full
, ffmpeg-full
Copy link
Member

Choose a reason for hiding this comment

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

Isn't the default standard ffmpeg not enough? And if so why?

Copy link
Member

Choose a reason for hiding this comment

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

See #271863 for context

Copy link
Contributor Author

Choose a reason for hiding this comment

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

here's the commit which switched Handbrake to ffmpeg-full. the explanation was:

FFMpeg-full has more codecs/containers for input and more processing possibilities, which seems sane for the rich video transcoder.

i could test a build with stock ffmpeg if you'd like to revisit this, but i don't have any test suite to judge the practical effects of such a change.

pkgs/applications/video/handbrake/default.nix Outdated Show resolved Hide resolved
Co-authored-by: Anton Mosich <[email protected]>
@uninsane uninsane force-pushed the pr-handbrake-1.7.3 branch from e7f5296 to af4164c Compare March 23, 2024 14:12
@SuperSandro2000 SuperSandro2000 merged commit bcb6ebe into NixOS:master Mar 25, 2024
23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update request: handbrake 1.6.1 → 1.7.3
6 participants