ring-daemon: init at 2017-07-11 #27311

Radvendii · 2017-07-11T20:30:12Z

Motivation for this change

RING DAEMON!!! 😁 (I have been trying to get this working for a while)

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
- Linux
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.

Radvendii · 2017-07-11T20:31:00Z

There are a couple of things that I haven't gotten working yet (video, for example). Should I mark that somewhere?

Radvendii · 2017-07-11T20:39:43Z

Also, I'm not sure if restbed should be a different package, but I'm using a super specific version of it and somehow I doubt it will be useful to anyone else

Radvendii · 2017-07-11T20:40:13Z

Should dringctrl.py be a separate package? It's a little script that you can use to control dring without using the dbus interface directly.

joachifm · 2017-07-11T20:45:19Z

The patches do not look nixpkgs specific. Please do not check them into the repository. If you are the author, consider submitting the patches upstream, otherwise provide an external source.

Radvendii · 2017-07-11T20:48:54Z

@joachifm Oh good call. They are actually contained in the src of ring-daemon. Can I include them somehow like ${src}/path/to/path?

joachifm · 2017-07-11T20:54:22Z

Sure. You need to rearrange things slightly, but in principle you should be able to do that quite simply.

joachifm · 2017-07-11T20:55:33Z

I'm lazy so I'd simply float the src to the outer scope to make accessing it more convenient.

joachifm

Added a few notes for your consideration.

joachifm · 2017-07-11T21:04:21Z

pkgs/applications/networking/instant-messengers/ring-daemon/default.nix

+    mkdir $out/bin
+    ln -s $out/lib/ring/dring $out/bin/dring
+    cp -R ./tools/dringctrl/ $out/
+    echo -e "#!${bash}/bin/bash\n${python}/bin/python $out/dringctrl/dringctrl.py \"\$@\"" > $out/bin/dringctrl


What about patching the python interpreter path directly into the script instead of this indirection? Wrappers are best avoided if at all possible.

and cping dringctrl.py into $out/bin?

The standard build sequence involves scanning outputs for shebangs that can be patched, so if you're lucky you could simply copy the python script to $out/bin and it'd Just Work.

It wasn't automatically patching the shebang, and the python file needs the other files in dringctrl to run, so I copied the whole thing into $out and then symlinked it into $out/bin/. I also had to manually change the shebang as patchShebangs wasn't doing anything.

joachifm · 2017-07-11T21:05:33Z

pkgs/applications/networking/instant-messengers/ring-daemon/default.nix

+    speexdsp
+  ];
+
+  preConfigure = ''


Have you tried the autoreconfHook? It removes some of the boilerplate (adding autotools to the build environment, running autogen).

Keeping the working version for now makes sense.

I had just taken out a package that was necessary. autoreconfHook works

joachifm · 2017-07-11T21:06:22Z

pkgs/applications/networking/instant-messengers/ring-daemon/default.nix

+  };
+
+  buildInputs = [
+    which


Tools that run only on the build host can be added to nativeBuildInputs (only really matters for cross compilation).

Does this go for all of them, or just which? Or do I have to test each one?

A rule of thumb is that tools that are used only as part of the build process (hence only runs on the build machine) can/should be added to the native inputs. Consider the case where build and host machines differ (incompatible). In that case, a which built for the host would fail to run on the build machine, so if which was intended to be used during the build, you'd need to mark it as native. If which is intended to run on the host (suppose the program you're building shells out to which at runtime) you'd add it to buildInputs.

So if a tool is needed in both contexts (building and running), should it be put in both nativeBuildInputs and buildInputs?

Done... I think. Not entirely sure what goes where, but I made educated guesses

joachifm · 2017-07-11T21:06:51Z

pkgs/applications/networking/instant-messengers/ring-daemon/default.nix

@@ -0,0 +1,133 @@
+{ stdenv
+, lib


lib is typically accessed via stdenv.lib.

joachifm · 2017-07-11T21:09:19Z

pkgs/applications/networking/instant-messengers/ring-daemon/default.nix

@@ -0,0 +1,133 @@
+{ stdenv
+, lib
+, callPackage


callPackage is primarily intended to be used at the toplevel, not within package expressions, where explicit import is typically used instead.

Mic92 · 2017-07-12T19:45:01Z

pkgs/applications/networking/instant-messengers/ring-daemon/default.nix

+
+  patchdir = "${src}/contrib/src";
+
+  restbed = import ./restbed.nix {


import inside a package prevents the use ring-daemon.override {}. It might be a better idea to make restbed a dedicated package in all-packages.

For clarification: it prevents someone from overriding restbed in ring-daemon.override{}. It also prevents restbed.overide{} since restbed is used without callPackage.

Yeah. I'm using a really specific version of restbed though, so I figured it wasn't worth making into a package. And then someone on IRC agreed with me. I'm happy to make a separate package though if that's the consensus.

It's also an old version of restbed

joachifm

Final nits, then I have nothing further on this.

joachifm · 2017-07-13T16:23:08Z

pkgs/applications/networking/instant-messengers/ring-daemon/restbed.nix

+
+stdenv.mkDerivation rec {
+  name = "restbed-${version}";
+  version = "34187502642144ab9f749ab40f5cdbd8cb17a54a";


I guess this is strictly an internal component, but in general using revisions for version strings is not recommended. It is important that new > old so that name based package management will be able to pick up on updated versions. You can use the date of the revision in year-month-day format instead.

joachifm · 2017-07-13T16:23:28Z

pkgs/applications/networking/instant-messengers/ring-daemon/default.nix

+  '';
+
+  meta = with stdenv.lib; {
+    description = "a Voice-over-IP software phone";


Please capitalize description

Radvendii · 2017-07-13T17:09:36Z

Is there a reason the checks are failing? It builds on my computer with nix-build -A ring-daemon

joachifm · 2017-07-13T17:25:33Z

Travis can't complete if there are large rebuilds going on. In general, if the failure doesn't look related to your change, ignore it.

But please consider enabling sandboxing when testing locally, it helps identify implicit dependencies.

Radvendii · 2017-07-13T17:33:27Z

How do I do that? nix-build --option build-use-sandbox true -A ring-daemon?

joachifm · 2017-07-13T17:39:09Z

If you use the nix daemon, you must enable it in nix.conf.

Radvendii · 2017-07-13T18:33:01Z

I don't, so it looks like using the command line option works fine (from what I read in the nix manual). Alright, I tested using that and it still works fine.

joachifm · 2017-07-13T20:52:46Z

So you're on nixos but don't use the daemon? Hm, okay.

Radvendii · 2017-07-13T21:08:37Z

Not that I'm aware of at least? It said in the manual that the daemon is for multi-user systems where people don't have wheel or something. I might be wrong about all this, I'm not very familiar with the inner workings of nixos

joachifm · 2017-07-13T21:13:05Z

Unless you went out of your way to disable the daemon, you're likely using it. See e.g, systemctl status nix-daemon.

Radvendii · 2017-07-13T21:28:09Z

Oh I see now. The command line option seemed to work fine, but now I added the option to my configuration.nix and built again. Everything seems fine.

joachifm · 2017-07-14T14:18:42Z

Thank you

joachifm reviewed Jul 11, 2017

View reviewed changes

Mic92 reviewed Jul 12, 2017

View reviewed changes

joachifm reviewed Jul 13, 2017

View reviewed changes

ring-daemon: init at 2017-07-11

484df87

joachifm merged commit e4229bb into NixOS:master Jul 14, 2017


		patchdir = "${src}/contrib/src";

		restbed = import ./restbed.nix {

ring-daemon: init at 2017-07-11 #27311

ring-daemon: init at 2017-07-11 #27311

Conversation

Radvendii commented Jul 11, 2017 • edited Loading

Motivation for this change

Things done

Radvendii commented Jul 11, 2017

Radvendii commented Jul 11, 2017

Radvendii commented Jul 11, 2017

joachifm commented Jul 11, 2017

Radvendii commented Jul 11, 2017

joachifm commented Jul 11, 2017

joachifm commented Jul 11, 2017

joachifm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Radvendii Jul 11, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Radvendii Jul 12, 2017 • edited Loading

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

joachifm Jul 11, 2017 • edited Loading

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

Mic92 Jul 12, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joachifm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Radvendii commented Jul 13, 2017

joachifm commented Jul 13, 2017

Radvendii commented Jul 13, 2017

joachifm commented Jul 13, 2017

Radvendii commented Jul 13, 2017 • edited Loading

joachifm commented Jul 13, 2017

Radvendii commented Jul 13, 2017

joachifm commented Jul 13, 2017

Radvendii commented Jul 13, 2017

joachifm commented Jul 14, 2017

Radvendii commented Jul 11, 2017 •

edited

Loading

Radvendii Jul 11, 2017 •

edited

Loading

Radvendii Jul 12, 2017 •

edited

Loading

joachifm Jul 11, 2017 •

edited

Loading

Mic92 Jul 12, 2017 •

edited

Loading

Radvendii commented Jul 13, 2017 •

edited

Loading