Skip to content

handbrake: 1.3.1 -> 1.3.2#86857

Merged
wmertens merged 1 commit intoNixOS:masterfrom
Anton-Latukha:handbrake-1.3.2
May 18, 2020
Merged

handbrake: 1.3.1 -> 1.3.2#86857
wmertens merged 1 commit intoNixOS:masterfrom
Anton-Latukha:handbrake-1.3.2

Conversation

@Anton-Latukha
Copy link
Contributor

@Anton-Latukha Anton-Latukha commented May 4, 2020

Motivation for this change

Starting to use GitHub CDN.
Every time there is any HandBrake release - it gets published on their website after a timeout of arbitrary days. And I want to be able to respond to notifications of release right away, and not postpone, stagger, keep checking wait website tarball and remember that I still need to update.

Also update to 1.3.2.

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.

@ofborg ofborg bot requested a review from wmertens May 4, 2020 23:59
@ofborg ofborg bot added 11.by: package-maintainer This PR was created by a maintainer of all the package it changes. 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-darwin: 1 This PR causes 1 package to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. 10.rebuild-linux: 1 This PR causes 1 package to rebuild on Linux. labels May 4, 2020
@peterhoeg
Copy link
Member

What happens when you build from the checkout as opposed to the release tarball?

@peterhoeg
Copy link
Member

and add autoreconfHook to nativeBuildInputs.

@doronbehar
Copy link
Contributor

Also, some parts of the preConfigure are not needed since:

HandBrake/HandBrake#2690

HandBrake/HandBrake#2712

@Anton-Latukha
Copy link
Contributor Author

@peterhoeg:

and add autoreconfHook to nativeBuildInputs.

Results:

source root is HandBrake-1.3.2
setting SOURCE_DATE_EPOCH to timestamp 1588534233 of file HandBrake-1.3.2/version.txt
patching sources
autoreconfPhase
autoreconf: 'configure.ac' or 'configure.in' is required
builder for '/nix/store/dvg6dlm5pwrxq8pfbljjqjfky6v5hsra-handbrake-1.3.2.drv' failed with exit code 1

@Anton-Latukha
Copy link
Contributor Author

Anton-Latukha commented May 5, 2020

@peterhoeg:

What happens when you build from the checkout as opposed to the release tarball?

Git checkout

I was discussing this issue with them upstream a couple of years ago. They have invented their-own versioning inside a project:

nix-shell . -A handbrake
unpackPhase
cd source
patchPhase
scripts/repo-info.sh    # source: https://github.com/HandBrake/HandBrake/blob/8e2a08267705f09e92f06289820ad6ebbfc9040e/scripts/repo-info.sh

Output:

URL=
HASH=ccb2e7234e01a2a07f537512045f7511583deaf9
SHORTHASH=ccb2e7234e0
REV=224188
BRANCH=handbrake-1.3.2
REMOTE=
DATE=2020-05-05 12:44:02 +0300

And this returned information results into probe: repo info...(fail) code 1

Results in build log:

source root is source
patching sources
configuring
...
probe: repo info...(fail) code 1
probe: version.txt...(fail)
ERROR: HandBrake is missing version information it needs to build properly.
Clone the official git repository at https://github.com/HandBrake/HandBrake.git
or download an official source archive from https://handbrake.fr
; configure stop.
compute: project data...
builder for '/nix/store/vwispj413x66pgh419awlgd6z9vj3nnv-handbrake-1.3.2.drv' failed with exit code 1

I got no description on what this situation is about, threads I've participated in or read resolve things replying "use website tarball".

Tarball package

Running same script in itL

scripts/repo-info.sh

Also results into probe: repo info...(fail) code 1, but now of the other kind

fatal: not a git repository (or any of the parent directories): .git
fatal: not a git repository (or any of the parent directories): .git
Not a valid repository.

And that situation "works" in the tarball:

probe: repo info...(fail) code 1
probe: version.txt...(pass)
...

If to give the long answer.

As I understand that there does not exist the case to succesfully go there proselytize and rip-out their internal distribution & versioning - I do not worry about this too much.

@Anton-Latukha
Copy link
Contributor Author

Anton-Latukha commented May 5, 2020

Also, some parts of the preConfigure are not needed since:

HandBrake/HandBrake#2690

HandBrake/HandBrake#2712

@doronbehar

Reduced the patches.

@doronbehar
Copy link
Contributor

BTW, I'm getting this when running it:

** Message: 14:08:09.629: Couldn't initialize gstreamer. Disabling live preview.

I think you can fix it with something similar to:

https://github.com/Anton-Latukha/nixpkgs/blob/f9a7d6c4ed32bb78d0df7d25dd1a3b72f658dbe4/pkgs/applications/audio/transcribe/default.nix#L46-L47

While some gst-plugins are located in the build inputs so wrapGAppsHook will pick them up.

But it's also possible that gappsWrapperArgs+=( is not needed...

M  pkgs/applications/video/handbrake/default.nix
@Anton-Latukha
Copy link
Contributor Author

Anton-Latukha commented May 5, 2020

BTW, I'm getting this when running it:

** Message: 14:08:09.629: Couldn't initialize gstreamer. Disabling live preview.

@doronbehar

I specifically tested that live preview works.

And with https://github.com/Anton-Latukha/nixpkgs/blob/f9a7d6c4ed32bb78d0df7d25dd1a3b72f658dbe4/pkgs/applications/audio/transcribe/default.nix#L46-L47 ghb still throws that same warning. And just as before - in ghb preview still works despite of that warning.

@doronbehar
Copy link
Contributor

@doronbehar
I specifically tested that live preview works.
And with https://github.com/Anton-Latukha/nixpkgs/blob/f9a7d6c4ed32bb78d0df7d25dd1a3b72f658dbe4/pkgs/applications/audio/transcribe/default.nix#L46-L47 ghb still throws that same warning. And just as before - in ghb live preview still works despite of that warning.

OK I guess then it's not that bad (I'm not using Handbrake actively, I only tested it once in the past). If someone will notice some missing functionality that may be related to this message, he'll put the effort to debug this.

@Anton-Latukha
Copy link
Contributor Author

Anton-Latukha commented May 5, 2020

@doronbehar

It makes in Preview the Live Preview button appear.
Now I remember, it encodes some seconds of video and then allows to play/watch it in the internal player window.
Like here: https://handbrake.fr/docs/en/latest/workflow/preview-settings.html

I also do not use HandBrake mutch these days, Preview is needed, I almost never run Live Preview, run the encoding for some %s, and then looked at that sample.

Wrapper configuration of GST_PLUGIN_SYSTEM_PATH is not successful in enabling feature.
I may dig into it, make a deeper assessment, maybe look how they --prefix LD_LIBRARY_PATH : "${libPath}" and assemble its libPath variable.

Thank you.

@Anton-Latukha
Copy link
Contributor Author

I do not understand why if one maintainer changes to the package, and already went through the review process - why other same-level maintained should "mandatory" review it.

The Mertens for example probably or too busy or do not care for it long time ago.

The review is done.

@doronbehar
Copy link
Contributor

Yea this is how things are in this repo unfortunately. Sometimes a maintainer with a merge bit goes through the reviewed pulls sorted from old to new and they may encounter this PR at some point, but don't hold your breath for this to happen.

@wmertens
Copy link
Contributor

Sorry, I missed that it was ok to merge. Indeed, I don't use handbrake any more, so feel free to sign yourself as a maintainer

@wmertens wmertens merged commit 548872b into NixOS:master May 18, 2020
@Anton-Latukha
Copy link
Contributor Author

Anton-Latukha commented May 18, 2020

@wmertens I am also a maintainer of this package for 2-3 years already.

We chatted with you back in the day about the package.

I am talking that I am a maintainer, process requires review from the higher-order maintainer, like doronbehar.

But I am maintainer that passes review.
I am just puzzled why "process" requires review from same-level package maintainers.

People tend to not remove themself from maintainer fields and not care and not show up.
And so - there are packages that have 5-6 "maintainers" but no one shows up to maintain it. So why that package should have all 5-6 maintainers sign-off between each other every time? It is not like this is Linux Kernel development, and as I know they do multistage reviews, but do not require such multicross peer review signoff all the time, because nothing would get done then.

@Anton-Latukha
Copy link
Contributor Author

Anton-Latukha commented May 18, 2020

@doronbehar

Thanks to you a lot. You are a great guy.

How is auto update bot there.

I see that it still not pipelined to autotrack and automerge

Why? It seems really strange.

I think most people, and you, would've preferred for minor updates to go without bothering them.

@doronbehar
Copy link
Contributor

Thanks to you a lot. You are a great guy.

💚

It is not like this is Linux Kernel development, and as I know they do multistage reviews, but do not require such multicross peer review signoff all the time, because nothing would get done then.

How is auto update bot there.
I see that it still not pipelined to autotrack and automerge
Why? It seems really strange.
I think most people, and you, would've preferred for minor updates to go without bothering them.

Your phrasing is not that good but I think I understand you. See:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-darwin: 1 This PR causes 1 package to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. 10.rebuild-linux: 1 This PR causes 1 package to rebuild on Linux. 11.by: package-maintainer This PR was created by a maintainer of all the package it changes.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants