handbrake: refactor (fx, clenup, description, cosmetics, add maintainer) #39117

Anton-Latukha · 2018-04-18T17:48:08Z

Motivation for this change

fx: ffmpeg was one of default buildInputs
by request adding myself as a maintainer of the package
description was outdated. Rewrote to much more precise modernized description.
version.txt patch no longer needed in 1.0.7. Moreover in the version 1.1.0 this patching should be removed.

Things done

Tested using sandboxing
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 nox --run "nox-review wip"
Tested execution of all binary files (usually in ./result/bin/)
Fits CONTRIBUTING.md.

Anton-Latukha · 2018-04-18T17:49:44Z

pkgs/applications/video/handbrake/default.nix


  buildInputs = [
    fribidi fontconfig freetype jansson zlib
    libass libiconv libsamplerate libxml2 bzip2
    libogg libopus libtheora libvorbis libdvdcss a52dec libmkv
-    lame ffmpeg libdvdread libdvdnav libbluray mp4v2 mpeg2dec x264 x265 libvpx
+    lame libdvdread libdvdnav libbluray mp4v2 mpeg2dec x264 x265 libvpx


I missed to contribute rm of this in the previous commit, that made switch from ffmpeg to libav_12.

Anton-Latukha · 2018-04-18T17:50:28Z

pkgs/applications/video/handbrake/default.nix

@@ -97,13 +93,17 @@ stdenv.mkDerivation rec {

  meta = with stdenv.lib; {
    homepage = http://handbrake.fr/;
-    description = "A tool for ripping DVDs into video files";
+    description = "A tool for converting video files and ripping DVDs";


It is really not only DVD ripping software. It is a way much more.

Anton-Latukha · 2018-04-18T17:55:07Z

pkgs/applications/video/handbrake/default.nix

    '';
    license = licenses.gpl2;
-    maintainers = with maintainers; [ wmertens ];
+    maintainers = with maintainers; [ Anton-Latukha wmertens ];


Added myself as a maintainer of package, by suggestion from peterhoeg in #38811 (comment).

jtojnar · 2018-04-19T00:20:08Z

pkgs/applications/video/handbrake/default.nix

@@ -37,15 +37,13 @@ stdenv.mkDerivation rec {

  nativeBuildInputs = [
    cmake python2 pkgconfig yasm autoconf automake libtool m4


With cmake, it is a good idea to also use ninja.

Are you sure autoconf and automake are still needed with cmake/ninja?

I have old mild knowledge of C++ builds. I used to C++ years ago, when Ninja was not around.

So:
If I add ninja to nativeBuildInputs system tries to use it and fails:

Build is configured! You may now cd into ./build and run make (/nix/store/h9w5yn61phv86iirl273bpw5ksq98fzz-gnumake-4.2.1/bin/make). building no build.ninja, doing nothing glibPreInstallPhase installing install flags: install ninja: error: loading 'build.ninja': No such file or directory builder for '/nix/store/38fygqhfawrhni6f166dl5ds8akhdgyb-handbrake-1.0.7.drv' failed with exit code 1

If I, while adding ninja, also remove autoconf and automake - it just falls flat the same way.

Obviously there is more steps needed here. Or HandBrake dudes do it the old way.

So from the hip, it did not went.

From first look at different NixPkgs sources - there is all that Ninja needs, to switch to it.

In a time I can try to understand more of what are you talking about or take your guidance.

It can be next PRs. Or in this one if you think so.

I can't find any CMakeLists.txt file in the source distribution so you might be able to rip out cmake.

peterhoeg · 2018-04-21T07:59:07Z

As I recall, upstream also recommends fetching a tarball from their site instead of pulling from github so you'll probably want to change fetchFromGitHub to fetchurl.

Anton-Latukha · 2018-04-21T10:20:10Z

It is for the later PRs.

Everything gradually.
I just waiting on this merge.

Ok, long story.

1.0.7 works from GitHub.

In 1.1.0 they changed the versioning model. Now they include different versions of different things for project. That makes manual patching, as was done in an old package, that I removed, - it makes manual patching a very cumbersome model.

And now, with 1.1.0 - distribution package build needs to take info from GitHub meta by itself during config/build.
Or build from URL that has versioning info with it in version.txt file.

Upstream wants that we used their-own bundle of libraries they distribute with sources, Windows-like. And that requirement is contrary to how we do things here.

1.1.0 falls during build in Nix with C++ error. And it has nothing to do with versioning. It stumbles on undefined variables in C++ code.

I created a new bugreport there on HB, where I talk about this code-library issue in C++.
And I am negotiating with them.
I am preparing package until we solve that in upstream.

HandBrake team does everything in their-own way, and most distributions AFAIK got softly banned from upstream support.

It is a pretty thin margin to collaborate with them.

Anton-Latukha · 2018-04-23T10:56:18Z

@jtojnar @peterhoeg

Hey, this can be merged. This is a cosmetics, non-tech changes.

Let's merge and move further.

After merge I can continue work on technical changes.
As most of guys, I need our package to be fixed with my ongoing PRs, so I can talk about it with HandBrake upstream team and refer them to it.

wmertens · 2018-04-23T13:47:09Z

Ok, I was the one that originally tried to split everything up because they had a bunch of useful patches but no upstream to send it to. If they want to do their own bundle so be it, it will just make the build slower and bigger, but ok.

In the meantime, this lgtm 👍

Anton-Latukha commented Apr 18, 2018

View reviewed changes

Anton-Latukha force-pushed the handbrake-rw branch from 43bea80 to 04ea8f8 Compare April 18, 2018 18:10

handbrake: refactor (fx, clenup, description, cosmetics, add maintainer)

9b4b21c

Anton-Latukha force-pushed the handbrake-rw branch from 04ea8f8 to 9b4b21c Compare April 18, 2018 18:16

GrahamcOfBorg added 10.rebuild-darwin: 0 10.rebuild-linux: 1-10 labels Apr 18, 2018

jtojnar reviewed Apr 19, 2018

View reviewed changes

wmertens merged commit 7c3dc2f into NixOS:master Apr 23, 2018

Anton-Latukha mentioned this pull request May 10, 2018

Cleanup of HandBrake related patches #40280

Merged

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

handbrake: refactor (fx, clenup, description, cosmetics, add maintainer) #39117

handbrake: refactor (fx, clenup, description, cosmetics, add maintainer) #39117

Anton-Latukha commented Apr 18, 2018

Anton-Latukha Apr 18, 2018 •

edited

Anton-Latukha Apr 18, 2018

Anton-Latukha Apr 18, 2018 •

edited

jtojnar Apr 19, 2018

peterhoeg Apr 19, 2018

Anton-Latukha Apr 21, 2018 •

edited

peterhoeg Apr 21, 2018

peterhoeg commented Apr 21, 2018

Anton-Latukha commented Apr 21, 2018 •

edited

Anton-Latukha commented Apr 23, 2018

wmertens commented Apr 23, 2018

		@@ -37,15 +37,13 @@ stdenv.mkDerivation rec {

		nativeBuildInputs = [
		cmake python2 pkgconfig yasm autoconf automake libtool m4

handbrake: refactor (fx, clenup, description, cosmetics, add maintainer) #39117

handbrake: refactor (fx, clenup, description, cosmetics, add maintainer) #39117

Conversation

Anton-Latukha commented Apr 18, 2018

Motivation for this change

Things done

Anton-Latukha Apr 18, 2018 • edited

Choose a reason for hiding this comment

Anton-Latukha Apr 18, 2018

Choose a reason for hiding this comment

Anton-Latukha Apr 18, 2018 • edited

Choose a reason for hiding this comment

jtojnar Apr 19, 2018

Choose a reason for hiding this comment

peterhoeg Apr 19, 2018

Choose a reason for hiding this comment

Anton-Latukha Apr 21, 2018 • edited

Choose a reason for hiding this comment

peterhoeg Apr 21, 2018

Choose a reason for hiding this comment

peterhoeg commented Apr 21, 2018

Anton-Latukha commented Apr 21, 2018 • edited

Anton-Latukha commented Apr 23, 2018

wmertens commented Apr 23, 2018

Anton-Latukha Apr 18, 2018 •

edited

Anton-Latukha Apr 18, 2018 •

edited

Anton-Latukha Apr 21, 2018 •

edited

Anton-Latukha commented Apr 21, 2018 •

edited