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

Jump to bottom

handbrake: fix Darwin build #89674

Merged
merged 1 commit into from Oct 3, 2020
Merged

handbrake: fix Darwin build #89674

merged 1 commit into from Oct 3, 2020

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

@ofborg ofborg bot added the 6.topic: darwin Running or building packages on Darwin label Jun 6, 2020
@bdesham bdesham mentioned this pull request Jun 6, 2020
Closed
10 tasks
@ofborg ofborg bot requested review from Anton-Latukha and wmertens June 6, 2020 16:58
@bdesham
Copy link
Contributor Author

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 commented Jun 8, 2020 •
edited

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

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 commented Jun 14, 2020 •
edited

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 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 commented Jun 15, 2020 •
edited

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 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

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

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

wmertens
wmertens reviewed Sep 1, 2020
View reviewed changes
Copy link
Contributor

@wmertens wmertens left a comment

Choose a reason for hiding this comment

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

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 commented Oct 1, 2020 •
edited

@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 Oct 1, 2020 that may be closed by this pull request
handbrake cli broken on x86_64-darwin #99317
Closed
@bdesham
Copy link
Contributor Author

bdesham commented Oct 2, 2020

@GrahamcOfBorg build handbrake

veprbl
veprbl approved these changes Oct 2, 2020
View reviewed changes
Copy link
Member

@veprbl veprbl left a comment

Choose a reason for hiding this comment

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

LGTM, runs on x86_64-darwin

pkgs/applications/video/handbrake/default.nix Outdated Show resolved Hide resolved
@bdesham
handbrake: fix Darwin build
bf0c032 
- 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
Copy link
Contributor Author

bdesham commented Oct 3, 2020

@GrahamcOfBorg build handbrake

@bdesham
Copy link
Contributor Author

bdesham commented Oct 3, 2020

This is (once again) ready to be merged.

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

veprbl commented Oct 3, 2020

Thank you!

@bdesham bdesham deleted the fix-handbrake-on-darwin branch January 18, 2021 16:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@veprbl veprbl veprbl approved these changes

@Anton-Latukha Anton-Latukha Awaiting requested review from Anton-Latukha

@wmertens wmertens Awaiting requested review from wmertens

Assignees
No one assigned
Labels
6.topic: darwin Running or building packages on Darwin 10.rebuild-darwin: 1-10 10.rebuild-darwin: 1 10.rebuild-linux: 1-10 10.rebuild-linux: 1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

handbrake cli broken on x86_64-darwin
5 participants
@bdesham @Anton-Latukha @nixos-discourse @veprbl @wmertens