Initial VDR integration #32050

ck3d · 2017-11-25T22:28:25Z

Motivation for this change

Integrated VDR and some plugins into NixOS.

Things done

Tested using sandboxing (nix.useSandbox on NixOS, or option build-use-sandbox in nix.conf on non-NixOS)
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.

matthewbauer

Merge conflict

ck3d · 2018-04-22T07:07:23Z

Thanks for reviewing. I updated to nixos-unstable and added two patches.

matthewbauer · 2018-04-22T18:04:03Z

@GrahamcOfBorg build vdr

GrahamcOfBorg · 2018-04-22T18:04:33Z

Failure on x86_64-linux (full log)

Attempted: vdr

Partial log (click to expand)

   abs(__GLIBCXX_TYPE_INT_N_0 __x) { return __x >= 0 ? __x : -__x; }
   ^~~
/nix/store/ng5kbr6kmh979qaachifbsbblydrcmqc-gcc-7.3.0/include/c++/7.3.0/bits/std_abs.h:102:3: note: candidate: constexpr __float128 std::abs(__float128)
   abs(__float128 __x)
   ^~~
g++ -g -O3 -Wall -Werror=overloaded-virtual -Wno-parentheses -fPIC -c -D_GNU_SOURCE -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -D_LARGEFILE64_SOURCE -DREMOTE_KBD -DBIDI -DSDNOTIFY -DLIRC_DEVICE=\"/var/run/lirc/lircd\" -DVIDEODIR=\"/srv/vdr/video\" -DCONFDIR=\"/var/lib/vdr\" -DARGSDIR=\"/etc/vdr/conf.d\" -DCACHEDIR=\"/var/cache/vdr\" -DRESDIR=\"/usr/local/share/vdr\" -DPLUGINDIR=\"/usr/local/lib/vdr\" -DLOCDIR=\"/usr/local/share/locale\" -I/nix/store/cywh34sn8b00qil2g112bdhgdklkh2ca-fontconfig-2.12.6-dev/include -I/nix/store/dpydsfzmxm2dfmsr1c2llvrra9y663qx-expat-2.2.5-dev/include -I/nix/store/11mvvhykc4svx4f5p5ssw5l87h7v99q6-freetype-2.9-dev/include/freetype2  -I/nix/store/1anjdss6di9zqlqnp9836ymsq5nfbnmx-fribidi-0.19.7/include/fribidi -I/nix/store/a32dmays1bbjswd63rzw4mqrbwbwzxn2-systemd-238-dev/include -o dvbspu.o dvbspu.c
make: *** [Makefile:124: dvbdevice.o] Error 1
make: *** Waiting for unfinished jobs....
builder for '/nix/store/3vy97sf6sgj04imgfq07sm3vfkiiic0s-vdr-2.2.0.drv' failed with exit code 2
�[31;1merror:�[0m build of '/nix/store/3vy97sf6sgj04imgfq07sm3vfkiiic0s-vdr-2.2.0.drv' failed

GrahamcOfBorg · 2018-04-22T18:04:35Z

No attempt on x86_64-darwin (full log)

The following builds were skipped because they don't evaluate on x86_64-darwin: vdr

Partial log (click to expand)


a) For `nixos-rebuild` you can set
  { nixpkgs.config.allowUnsupportedSystem = true; }
in configuration.nix to override this.

b) For `nix-env`, `nix-build`, `nix-shell` or any other Nix command you can add
  { allowUnsupportedSystem = true; }
to ~/.config/nixpkgs/config.nix.

GrahamcOfBorg · 2018-04-22T18:05:10Z

Failure on aarch64-linux (full log)

Attempted: vdr

Partial log (click to expand)

        __bos (__s), __fmt, __va_arg_pack ());
                                            ^
ar rv libsi.a util.o si.o section.o descriptor.o
ar: creating libsi.a
a - util.o
a - si.o
a - section.o
a - descriptor.o
builder for '/nix/store/jcpyzqd46df75av5h6ad85891g1xciff-vdr-2.2.0.drv' failed with exit code 2
�[31;1merror:�[0m build of '/nix/store/jcpyzqd46df75av5h6ad85891g1xciff-vdr-2.2.0.drv' failed

ck3d · 2018-04-23T20:47:49Z

x86_64-linux: fixed the gcc-7 compatibility issue.
x86_64-darwin: Is not supported and not part of the root packages vdrBundle.vdr meta information. What else needs to be done, to pass that check?
aarch64-linux: I can only check x86_64-linux. I should restrict that packages to that platform.

used same plugin mechanism as kodi does

ck3d · 2018-12-08T22:14:50Z

updated PR contains:

restrict to linux 32 and 64 bit platforms
updated to vdr 2.4.0 and latest plugins

Mic92 · 2018-12-08T23:23:32Z

nixos/modules/services/hardware/vdr.nix

+        mkdir -p "${cfg.videoDir}"
+        chown vdr:vdr "${cfg.videoDir}"
+      fi
+    '';


It is better to use systemd.tmpfiles.rules instead of activation scripts, which are executed unconditionally on every switch.

videoDir is the folder where all recordings are put in. tmpfiles is a little bit missleading. I put responsibility to admin. Folder has to exist.

Mic92 · 2018-12-08T23:29:18Z

pkgs/top-level/all-packages.nix

+      ++ optional (config.vdr.enableSvdrpDemo or false) svdrpdemo
+      ++ optional (config.vdr.enableText2Skin or false) text2skin
+      ++ optional (config.vdr.enableVnsiServer or false) vnsiserver
+      );


Instead of so many options cannot users just pass them to vdrPlugins { plugins = [];}?

Mic92 · 2018-12-08T23:31:47Z

nixos/modules/services/hardware/vdr.nix

+
+      if ! [ -d "${libDir}" ]; then
+        mkdir -p "${libDir}"
+        cp --dereference -r "${cfg.package}"/share/vdr/conf/* "${libDir}"


This one should be part of the service. Also use touch for a file in libDir afterwards so the command get repeated on failure by checking for the existence of the file instead of the existence of the directory.

It is OK to remove that feature.

Mic92 · 2018-12-08T23:34:38Z

nixos/modules/services/hardware/vdr.nix

+    '';
+
+    security.sudo.configFile = ''
+      vdr ALL=(root) NOPASSWD:${pkgs.utillinux}/bin/rtcwake


This one also needs sudo to be activated I think.

That's true. I think it was a bad design to expect that rtcwake is used during shutdown. Will be removed.

Mic92 · 2018-12-08T23:35:49Z

nixos/modules/services/hardware/vdr.nix

+  ###### implementation
+
+  config = mkIf cfg.enable {
+    environment.systemPackages = [ cfg.package ];


Does this needs to be in PATH?

No not needed. Will be removed.

Mic92 · 2018-12-08T23:36:16Z

nixos/modules/services/hardware/vdr.nix

+      package = mkOption {
+        type = types.package;
+        default = pkgs.vdr-with-plugins;
+        description = "Package to use.";


Add here an example using wrapVdr.

Mic92 · 2018-12-08T23:36:39Z

nixos/modules/services/hardware/vdr.nix

+      };
+
+      extraArguments = mkOption {
+        type = types.str;


Better to make this a list.

right, much better. Will use it with lib.escapeShellArgs

Mic92 · 2018-12-08T23:39:18Z

nixos/modules/services/hardware/vdr.nix

+      if ! [ -d "${libDir}" ]; then
+        mkdir -p "${libDir}"
+        cp --dereference -r "${cfg.package}"/share/vdr/conf/* "${libDir}"
+        chown -R vdr:vdr "${libDir}"


You can also use systemd's StateDirectory option to automatically create the directory and chown it to the user.

OK, but copy of good defaults if folder doesn't exist will the be removed.

Mic92 · 2018-12-08T23:39:53Z

nixos/modules/services/hardware/vdr.nix

+    system.activationScripts.vdr = ''
+      if ! [ -d /var/cache/vdr ]; then
+        mkdir -p /var/cache/vdr
+        chown vdr:vdr /var/cache/vdr


Use systemd's CacheDirectory here.

CacheDirectory matches perfectly.

ck3d · 2018-12-09T21:52:04Z

@Mic92 Thanks a lot for all the comments. I will take a bit of time to incorporate all your good recommendations.

Mic92 · 2018-12-13T14:48:19Z

Please don't mark reviews as resolves unless you have also pushed the changes, where you addressed the review.

Mic92 · 2018-12-21T13:07:15Z

@GrahamcOfBorg eval

Mic92 · 2018-12-21T13:10:43Z

pkgs/applications/video/vdr/default.nix

+    buildInputs = [ gettext vdr ];
+    nativeBuildInputs = [ pkgconfig ];
+    preConfigure = "cd PLUGINS/src/${name}";
+    installFlags = [ "DESTDIR=$(out)" "PREFIX=" ];


Usually this comes with its own set of problems using DESTDIR instead of PREFIX when it uses PREFIX somewhere in the source. What was the offending file that could that ignored PREFIX, when installing?

Mic92 · 2018-12-21T13:10:58Z

pkgs/applications/video/vdr/default.nix

+    name = "vdr-${name}-${version}";
+    inherit (vdr) src;
+    buildInputs = [ gettext vdr ];
+    nativeBuildInputs = [ pkgconfig ];


gettext also is a nativeBuildInput.

Mic92 · 2018-12-21T13:11:40Z

pkgs/applications/video/vdr/default.nix

+
+    postPatch = "substituteInPlace Makefile --replace libsystemd-daemon libsystemd";
+
+    buildInputs = [ fontconfig libjpeg libcap freetype perl ]


If perl is not linked at runtime or used in runtime scripts, it should be also in nativeBuildInputs.

Mic92 · 2018-12-21T13:12:00Z

pkgs/applications/video/vdr/default.nix

+      ++ stdenv.lib.optional enableSystemd "SDNOTIFY=1"
+      ++ stdenv.lib.optional enableBidi "BIDI=1";
+
+    propagatedBuildInputs = [ pkgconfig gettext ];


Should this not be in nativeBuildInputs?

Every vdr plugin need to have the same build tools than vdr itself. If will you propagatedNavtiveBuildInputs. If you want the plugin descriptions more explicit I can change it to no propagated.

The problem is that it also propagate to every other package that uses vdr somehow, which can be surprising at times. You can abstract it way in a variable or so or use vdr.propagatedBuildInputs explicit.

Mic92 · 2018-12-21T13:13:13Z

pkgs/applications/video/vdr/plugins.nix

+      pcre
+      perl # for pod2man and pos2html
+      utillinux
+      groff


perl, utillinux and groff probably belong to nativeBuildInputs.

Mic92 · 2018-12-21T13:13:53Z

pkgs/applications/video/vdr/plugins.nix

+
+    buildInputs = [ vdr graphicsmagick ];
+
+    buildFlags = [ "DESTDIR=$(out)" ];


also here the question as above regarding PREFIX instead of DESTDIR.

Mic92 · 2018-12-21T13:14:37Z

pkgs/applications/video/vdr/plugins.nix

+        -e 's,IMAGELIB = imagemagick,IMAGELIB = graphicsmagic,' \
+        -e 's,^VDRDIR *=.*,VDRDIR = ${vdr.dev}/include/vdr,' \
+        -e 's,^LOCALEDIR *=.*,LOCALEDIR = $(DESTDIR)/share/locale,' \
+        -e 's,^LIBDIR *=.*,LIBDIR = $(DESTDIR)/lib/vdr,'


You can also override variables by using makeFlags instead of sed.

pkgs/applications/video/vdr/wrapper.nix

Mic92 · 2018-12-21T13:19:27Z

pkgs/applications/video/vdr/plugins.nix

+
+    buildInputs = [ vdr boost libconvpp libfritzpp libnetpp liblogpp ];
+
+    installFlags = [ "DESTDIR=$(out)" ];


pkgs/applications/video/vdr/plugins.nix

Mic92 · 2018-12-21T13:22:11Z

nixos/modules/misc/ids.nix

@@ -338,6 +338,7 @@
      minetest = 311;
      rss2email = 312;
      cockroachdb = 313;
+      vdr = 314;


not required, will be allocated on first use:

Suggested change

vdr = 314;

Mic92 · 2018-12-21T13:22:46Z

nixos/modules/misc/ids.nix

@@ -636,6 +637,7 @@
      minetest = 311;
      rss2email = 312;
      cockroachdb = 313;
+      vdr = 314;


same here:

Suggested change

vdr = 314;

Mic92 · 2018-12-21T13:25:41Z

nixos/modules/services/hardware/vdr.nix

+
+      package = mkOption {
+        type = types.package;
+        default = pkgs.vdr;


To fix the aarch64 build we also need a defaultText = "pkgs.vdr".

Co-Authored-By: ck3d <ck3d@gmx.de>

ck3d · 2018-12-22T11:36:51Z

@Mic92 Thanks for all our good comments! Could you please have once more a look to my PR.

Mic92 · 2018-12-22T16:14:05Z

See #52686

vdr: revisited version of #32050

GrahamcOfBorg added 10.rebuild-darwin: 0 10.rebuild-linux: 1-10 labels Nov 25, 2017

matthewbauer requested changes Apr 21, 2018

View reviewed changes

ck3d force-pushed the upstream-vdr branch from 503297a to c47b0a6 Compare April 22, 2018 07:03

GrahamcOfBorg added 6.topic: nixos 8.has: module (update) labels Apr 22, 2018

veprbl added the 2.status: merge conflict label Nov 28, 2018

initial vdr package and nixos module

28e56e2

used same plugin mechanism as kodi does

ck3d force-pushed the upstream-vdr branch from 3c6f064 to 28e56e2 Compare December 8, 2018 22:11

ck3d requested a review from infinisil as a code owner December 8, 2018 22:11

GrahamcOfBorg removed the 2.status: merge conflict label Dec 8, 2018

Mic92 reviewed Dec 8, 2018

View reviewed changes

vdr: implement review comments

002e8da

ck3d added 2 commits December 20, 2018 20:43

vdrPlugins: use callPackages instead of callPackage

e784510

vdr: use libva instead of obsolete libva-full

0907ed1

Mic92 reviewed Dec 21, 2018

View reviewed changes

pkgs/applications/video/vdr/wrapper.nix Outdated Show resolved Hide resolved

Mic92 reviewed Dec 21, 2018

View reviewed changes

pkgs/applications/video/vdr/plugins.nix Outdated Show resolved Hide resolved

Mic92 reviewed Dec 21, 2018

View reviewed changes

Mic92 and others added 6 commits December 21, 2018 18:32

Apply suggestions from code review

856ffb5

Co-Authored-By: ck3d <ck3d@gmx.de>

vdr: sorted native build tools out

1a11aea

vdr: use dynmic uid and gid allocation

dbfecd3

vdr: set defaultText to satisfy build errors on aarch64

75bceb3

vdr: get rid of all sed commands and use buildFlags

9c89ec6

vdr: clarify why PREFIX can not be used

56f12a4

GrahamcOfBorg added 8.has: package (new) 10.rebuild-linux: 11-100 and removed 10.rebuild-linux: 1-10 labels Dec 22, 2018

Mic92 mentioned this pull request Dec 22, 2018

vdr: revisited version of https://github.com/NixOS/nixpkgs/pull/32050 #52686

Merged

Mic92 added a commit that referenced this pull request Dec 23, 2018

Merge pull request #52686 from Mic92/vdr

633bc1d

vdr: revisited version of #32050

ck3d closed this Dec 23, 2018


		postPatch = "substituteInPlace Makefile --replace libsystemd-daemon libsystemd";

		buildInputs = [ fontconfig libjpeg libcap freetype perl ]


		buildInputs = [ vdr graphicsmagick ];

		buildFlags = [ "DESTDIR=$(out)" ];


		buildInputs = [ vdr boost libconvpp libfritzpp libnetpp liblogpp ];

		installFlags = [ "DESTDIR=$(out)" ];

Initial VDR integration #32050

Initial VDR integration #32050

Conversation

ck3d commented Nov 25, 2017

Motivation for this change

Things done

matthewbauer left a comment

Choose a reason for hiding this comment

ck3d commented Apr 22, 2018

matthewbauer commented Apr 22, 2018

GrahamcOfBorg commented Apr 22, 2018

GrahamcOfBorg commented Apr 22, 2018

GrahamcOfBorg commented Apr 22, 2018

ck3d commented Apr 23, 2018

ck3d commented Dec 8, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Mic92 Dec 8, 2018 • edited

Choose a reason for hiding this comment

Mic92 Dec 8, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ck3d commented Dec 9, 2018

Mic92 commented Dec 13, 2018

Mic92 commented Dec 21, 2018

Mic92 Dec 21, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Mic92 Dec 22, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Mic92 Dec 21, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ck3d commented Dec 22, 2018

Mic92 commented Dec 22, 2018

Mic92 Dec 8, 2018 •

edited

Mic92 Dec 8, 2018 •

edited

Mic92 Dec 21, 2018 •

edited

Mic92 Dec 22, 2018 •

edited

Mic92 Dec 21, 2018 •

edited