handbrake: allow building from checkout #88127
Conversation
|
This is hard to read. Can we have this as patches instead?
This part is only showing up in the diff because I moved them from preConfigure to postPatch. This is not otherwise new.
|
| 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, |
There was a problem hiding this comment.
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.
|
I'd preferred to align deps to their switch so there would be visual
It was auto-formatting that did that. In the near future, all formatting will hopefully be handled by nixpkgs-fmt in which case manual formatting will go away completely.
|
| # 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 = '' |
There was a problem hiding this comment.
So why not add that echo DATE command here is well?
|
|
||
| configureFlags = [ | ||
| "--harden" |
There was a problem hiding this comment.
I'm not against this because I don't know what this is but it'd be nice to have a comment above this.
|
Also, did the aarch64 build failed before this PR? |
|
So why not add that echo DATE command here is well?
Because it's too late. extraFetchHook runs after we have downloaded the file but before it gets committed to the store. During the patch phase, the files come from the store and the timestamp is now that of the unix epoch.
I'm not against this because I don't know what this is but it'd be nice to
have a comment above this.
Fair point, I'll add that in.
|
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 |
There was a problem hiding this comment.
Wait are we sure this is reproducible? It will be only if the "modtime" of the file stays the same? Maybe try:
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There's a nit new tool called hydra-check. With it, I ran: And I got: 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. |
|
There's a nit new tool called hydra-check. With it, I ran:
hydra-check --arch=aarch64-linux handbrake
Cool, didn't know that, thanks.
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.
So 1.1.2 was the last version built by hydra but it's not hydra building the PRs.
Turns out it was the `--harden` flag that broke things on ARM, so now it's conditional.
|
|
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
No, because fetchFromGitHub doesn't actually do a git checkout. It fetches a tarball which doesn't have .git.
|
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:
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
sandboxinnix.confon non-NixOS linux)nix-shell -p nixpkgs-review --run "nixpkgs-review wip"./result/bin/)nix path-info -Sbefore and after)