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: allow building from checkout #88127

Merged
merged 3 commits into from May 20, 2020
Merged

handbrake: allow building from checkout #88127

merged 3 commits into from May 20, 2020

Conversation

@peterhoeg
Copy link
Member

peterhoeg commented May 19, 2020

Motivation for this change

Further to #86857, there is a bit of an anti-pattern with how the source is handled, that I would like to rectify:

  1. Use fetchFromGitHub and construct version.txt so we can point $src at something else (a fork, a local checkout, etc) and still be able to build it.
  2. Build in parallel to speed things up.
  3. Do the patching in postPatch where it belongs as opposed to preConfigure
  4. Build with additional hardening in case we try to process a wonky file

Cc: @Anton-Latukha @doronbehar

We really only need DATE and HASH in version.txt for things to work -
the rest are just to be make the "About" window look nicer.

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.
@peterhoeg
Copy link
Member Author

peterhoeg commented May 19, 2020

intltool ? null,
glib ? null,
gtk3 ? null,
libappindicator-gtk3 ? null,
libnotify ? null,
gst_all_1 ? null,
dbus-glib ? null,
udev ? null,
libgudev ? null,
hicolor-icon-theme ? null,
intltool ? null,
glib ? null,
gtk3 ? null,
libappindicator-gtk3 ? null,
libnotify ? null,
gst_all_1 ? null,
dbus-glib ? null,
udev ? null,
libgudev ? null,
hicolor-icon-theme ? null,
Comment on lines -33 to +42

This comment has been minimized.

Copy link
@Anton-Latukha

Anton-Latukha May 19, 2020

Contributor

I'd preferred to align deps to their switch or have newline separation at least so there would be visual guidance of what switch controls it.

This is a minor style question and unimportant.

Copy link
Contributor

Anton-Latukha left a comment

Great work!

@peterhoeg
Copy link
Member Author

peterhoeg commented May 19, 2020

# we put as little as possible in src.extraPostFetch as it's much easier to
# add to it here without having to fiddle with src.sha256
# only DATE and HASH are absolutely necessary
postPatch = ''

This comment has been minimized.

Copy link
@doronbehar

doronbehar May 19, 2020

Contributor

So why not add that echo DATE command here is well?


configureFlags = [
"--harden"

This comment has been minimized.

Copy link
@doronbehar

doronbehar May 19, 2020

Contributor

I'm not against this because I don't know what this is but it'd be nice to have a comment above this.

@doronbehar
Copy link
Contributor

doronbehar commented May 19, 2020

Also, did the aarch64 build failed before this PR?

@peterhoeg
Copy link
Member Author

peterhoeg commented May 19, 2020

@ofborg ofborg bot requested a review from Anton-Latukha May 19, 2020
@peterhoeg
Copy link
Member Author

peterhoeg commented May 19, 2020

Also, did the aarch64 build failed before this PR?

No clue - and I don't know where I would check...

rev = version;
sha256 = "04z3hcy7m5yvma849rlrsx2wdqmkilkl1qds9yrzr2ydpw697f85";
extraPostFetch = ''
echo "DATE=$(date +"%F %T %z" -r $out/NEWS.markdown)" > $out/version.txt

This comment has been minimized.

Copy link
@doronbehar

doronbehar May 19, 2020

Contributor

Wait are we sure this is reproducible? It will be only if the "modtime" of the file stays the same? Maybe try:

https://github.com/HandBrake/HandBrake/blob/0b6b6847631881bd16027c90b079c06adadf666f/make/configure.py#L890

This comment has been minimized.

Copy link
@peterhoeg

peterhoeg May 20, 2020

Author Member

Wait are we sure this is reproducible?

Yep.

It will be only if the "modtime" of the file stays the same?

And it will at least until a new version is installed in which case we want it to change.

Running repo-info.sh doesn't work as we don't have a full git checkout - this is the whole reason for generating version.txt manually.

This comment has been minimized.

Copy link
@doronbehar

doronbehar May 20, 2020

Contributor

Running repo-info.sh doesn't work as we don't have a full git checkout - this is the whole reason for generating version.txt manually.

You will be able to run that script if you'll tell fetchFromGitHub to leaveDotgit = true (I think this is how it's called) and you'll also be able to get rid of the other echo commands then. I think this is much cleaner so it's worth a try.

@doronbehar
Copy link
Contributor

doronbehar commented May 19, 2020

Also, did the aarch64 build failed before this PR?

No clue - and I don't know where I would check...

There's a nit new tool called hydra-check. With it, I ran:

hydra-check --arch=aarch64-linux handbrake

And I got:

Build Status for nixpkgs.handbrake.aarch64-linux on unstable
✔ handbrake-1.1.2 from 2018-12-22 - https://hydra.nixos.org/build/86216172

Which is damn ancient so I don't really know what's going on. It's also worth mentioning that this didn't fail in the CI of #86857 so I don't know how to judge this.

@ofborg ofborg bot requested a review from Anton-Latukha May 20, 2020
@peterhoeg
Copy link
Member Author

peterhoeg commented May 20, 2020

@peterhoeg
Copy link
Member Author

peterhoeg commented May 20, 2020

@peterhoeg peterhoeg merged commit a0d7927 into master May 20, 2020
16 checks passed
16 checks passed
Evaluation Performance Report Evaluator Performance Report
Details
grahamcofborg-eval ^.^!
Details
grahamcofborg-eval-check-maintainers matching changed paths to changed attrs...
Details
grahamcofborg-eval-check-meta config.nix: checkMeta = true
Details
grahamcofborg-eval-darwin nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="423296f"; rev="423296fc354341ba1d81239f5d5757265bec0619"; } ./pkgs/t
Details
grahamcofborg-eval-lib-tests nix-build --arg pkgs import ./. {} ./lib/tests/release.nix
Details
grahamcofborg-eval-nixos nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="423296f"; rev="423296fc354341ba1d81239f5d5757265bec0619"; } ./nixos/
Details
grahamcofborg-eval-nixos-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="423296f"; rev="423296fc354341ba1d81239f5d5757265bec0619"; } ./nixos/
Details
grahamcofborg-eval-nixos-options nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="423296f"; rev="423296fc354341ba1d81239f5d5757265bec0619"; } ./nixos/
Details
grahamcofborg-eval-nixpkgs-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="423296f"; rev="423296fc354341ba1d81239f5d5757265bec0619"; } ./pkgs/t
Details
grahamcofborg-eval-nixpkgs-tarball nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="423296f"; rev="423296fc354341ba1d81239f5d5757265bec0619"; } ./pkgs/t
Details
grahamcofborg-eval-nixpkgs-unstable-jobset nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="423296f"; rev="423296fc354341ba1d81239f5d5757265bec0619"; } ./pkgs/t
Details
grahamcofborg-eval-package-list nix-env -qa --json --file .
Details
grahamcofborg-eval-package-list-no-aliases nix-env -qa --json --file . --arg config { allowAliases = false; }
Details
handbrake, handbrake.passthru.tests on aarch64-linux Success
Details
handbrake, handbrake.passthru.tests on x86_64-linux Success
Details
@Mic92 Mic92 deleted the f/handbrake branch May 20, 2020
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.

None yet

4 participants
You can’t perform that action at this time.