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: fix Darwin build #89674

Merged
merged 1 commit into from Oct 3, 2020
Merged

Conversation

@bdesham
Copy link
Contributor

@bdesham bdesham commented Jun 6, 2020

Motivation for this change

The handbrake build had broken under Darwin. This PR fixes it by

  • Omitting the numactl dependency under Darwin.
  • Preventing the configure script from checking for xcodebuild. This check always failed (because xcodebuild isn't available in the build context) but xcodebuild isn't actually needed for the build, which uses --disable-xcode.
  • Using the Nix-provided libxml2 under Darwin like we already do under Linux.
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.

cc @Anton-Latukha @wmertens

@bdesham
Copy link
Contributor Author

@bdesham bdesham commented Jun 6, 2020

@Anton-Latukha I tried adding --force to the configure options when building under Darwin, but unfortunately the configure script still stopped when it couldn’t find xcodebuild.

@Anton-Latukha
Copy link
Contributor

@Anton-Latukha Anton-Latukha commented Jun 8, 2020

Ok.

Thank you.

I approve the pull request.

Please, as you can - open a proper upstream bug report with according technical information.

@Anton-Latukha
Copy link
Contributor

@Anton-Latukha Anton-Latukha commented Jun 14, 2020

HandBrake 1.3.3 is out, I would submit PR 1.3.(2->3), there is only ver/hash change, so PRs are independently mergeable. The release has minor changes, there are no changes related to this PR.

@Anton-Latukha
Copy link
Contributor

@Anton-Latukha Anton-Latukha commented Jun 14, 2020

The reason there is no report about the issue the PR addresses - probably because their CI can just fail "optional" build, and they provide a set of prebuild packages, and for macOS also. So everyone probably uses those packages or dodges the bullet by using official package. If in 8 months nobody except us noticed, we may be the only one package management system that assembles and compiles the project directly on macOS.

@bdesham
Copy link
Contributor Author

@bdesham bdesham commented Jun 15, 2020

@Anton-Latukha It’s difficult for me to test a separate PR for the version bump because the package doesn’t build at all for me (on Darwin) without the changes in this current PR.

@Anton-Latukha
Copy link
Contributor

@Anton-Latukha Anton-Latukha commented Jun 15, 2020

No, man, I just notified you that I opened parallel PR, and that they are such that can merge in parallel in any order.

Because I am quite aware of how parallel PRs can create all sorts of stuff. And so I inform you that there is a parallel PR, and they would merge, no rebases needed.

I wish I could merge your PR myself, already 9 days. I used to snipe call the active maintainers, but for example Mic92 was active 20 minutes ago.

@bdesham
Copy link
Contributor Author

@bdesham bdesham commented Jun 16, 2020

Oh, I’m sorry, I misunderstood. I thought you were saying that I should open a new PR 🙂

Maybe I will mention this PR and #90302 on the NixOS Discourse to get some more visibility.

@nixos-discourse
Copy link

@nixos-discourse nixos-discourse commented Jun 16, 2020

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/prs-already-reviewed/2617/170

@Anton-Latukha
Copy link
Contributor

@Anton-Latukha Anton-Latukha commented Aug 1, 2020

I hope your PR gets merged faster. You somehow receive a huge timeouts on PRs merges.

Copy link
Contributor

@wmertens wmertens left a comment

LGTM, just need some more clarity about those libxml2 patches?

pkgs/applications/video/handbrake/default.nix Outdated Show resolved Hide resolved
@Anton-Latukha
Copy link
Contributor

@Anton-Latukha Anton-Latukha commented Oct 1, 2020

@danieldk

Pinging, so we close the #99317 report.

I see you were active recently.

Two package maintainers reviewed this PR, changes were discussed and PR was fit into a proper form, and PR approved by those two package maintainers.
We also did some work upstream in Handbrake also.

We basically just wait for someone to come by and merge this.

@veprbl veprbl linked an issue that may be closed by this pull request Oct 1, 2020
@bdesham bdesham force-pushed the fix-handbrake-on-darwin branch from f850480 to efc384a Oct 2, 2020
@bdesham
Copy link
Contributor Author

@bdesham bdesham commented Oct 2, 2020

@GrahamcOfBorg build handbrake

@ofborg ofborg bot requested review from Anton-Latukha and wmertens Oct 2, 2020
veprbl
veprbl approved these changes Oct 2, 2020
Copy link
Member

@veprbl veprbl left a comment

LGTM, runs on x86_64-darwin

pkgs/applications/video/handbrake/default.nix Outdated Show resolved Hide resolved
- Omit the numactl dependency under Darwin.
- Prevent the configure script from checking for xcodebuild. This check
  always failed (because xcodebuild isn't available in the build
  context) but xcodebuild isn't actually needed for the build, which
  uses --disable-xcode.
- Use the Nix-provided libxml2 under Darwin like we already do under
  Linux.
@bdesham bdesham force-pushed the fix-handbrake-on-darwin branch from efc384a to bf0c032 Oct 3, 2020
@bdesham
Copy link
Contributor Author

@bdesham bdesham commented Oct 3, 2020

@GrahamcOfBorg build handbrake

@bdesham
Copy link
Contributor Author

@bdesham bdesham commented Oct 3, 2020

This is (once again) ready to be merged.

@veprbl veprbl merged commit 0e60d79 into NixOS:master Oct 3, 2020
21 checks passed
@veprbl
Copy link
Member

@veprbl veprbl commented Oct 3, 2020

Thank you!

siraben added a commit to siraben/nixpkgs that referenced this issue Oct 5, 2020
- Omit the numactl dependency under Darwin.
- Prevent the configure script from checking for xcodebuild. This check
  always failed (because xcodebuild isn't available in the build
  context) but xcodebuild isn't actually needed for the build, which
  uses --disable-xcode.
- Use the Nix-provided libxml2 under Darwin like we already do under
  Linux.
dawidsowa added a commit to dawidsowa/nixpkgs that referenced this issue Oct 11, 2020
- Omit the numactl dependency under Darwin.
- Prevent the configure script from checking for xcodebuild. This check
  always failed (because xcodebuild isn't available in the build
  context) but xcodebuild isn't actually needed for the build, which
  uses --disable-xcode.
- Use the Nix-provided libxml2 under Darwin like we already do under
  Linux.
@bdesham bdesham deleted the fix-handbrake-on-darwin branch Jan 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

5 participants