scyther: init at 1.1.3 #27006

infinisil · 2017-07-01T04:37:33Z

This PR adds the scyther application, a tool for automatic verification of security protocols. On GitHub

This is the first time I wrote a non-trivial amount of nix code, I tried to structure it well :)

I split the package up into 4 files:

sources.nix contains different versions of the package. The official latest release is 1.1.3, there is also a Compromise-0.9.2 version which has some different performance characteristics which may not everybody want. Then I also included the latest commit from master, as the project hasn't seen a new release in a while and there were some bug fixes.
cli.nix contains the 32bit package for building the C part, which generates a single executable, usable by itself.
gui.nix contains the python GUI frontend, which uses the CLI at runtime
default.nix contains the actual package which builds a single environment. Using the includeGUI (default true) option, one can choose to not to have the GUI, while the version option lets you select one of the three above mentioned versions.

This split into multiple derivations is made to reduce build time and output path size.

Motivation for this change

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.

MostAwesomeDude

I have no serious objections as-is, but there's a few nits. Thank you for your hard work.

MostAwesomeDude · 2017-07-01T04:41:10Z

pkgs/applications/science/programming/scyther/default.nix

@@ -0,0 +1,30 @@
+{ lib, callPackage, callPackage_i686, buildEnv
+, includeGUI ? true


I don't know this package at all; how useful is it to its audience without the GUI? I'm wondering whether this should default to false.

The package is advertised as a GUI application first, it provides visualizations the cli can't do easily.

Thinking about it again, it might be better to default to false, the current NixOS target audience would probably prefer CLI tools (me included). Also the CLI tool doesn't have the mentioned runtime problems with certain protocol

Why bother joining the two tools in a single output?

MostAwesomeDude · 2017-07-01T04:43:11Z

pkgs/applications/science/programming/scyther/default.nix

+    paths = [ cli ] ++ lib.optional includeGUI gui;
+    pathsToLink = ["/bin"];
+
+    postBuild = if includeGUI then ''


I did a double-take here. I think I understand what's happening; if the GUI's included, then link the GUI protocols in; otherwise, copy the (bare-bones?) protocols from the source package. But I'm not sure why. Can we get a comment explaining this trickery?

Ah yes, I'm copying the whole ./gui directory from the source to $out, because that's where all the GUI is. I believe there is some way for the GUI to show these protocols, that's why they're there. But the CLI wouldn't include them, but because the protocols are so useful, I decided to have them in there anyways. Could be made a seperate output though, doesn't technically need to be there.

And if I always copied them from source they would be duplicated in the case of includeGUI = true

MostAwesomeDude · 2017-07-01T04:43:48Z

pkgs/applications/science/programming/scyther/sources.nix

+    let
+      filtered = builtins.filter (s: s.name == "${version}") versions;
+    in if filtered != [] then fetchSource (builtins.head filtered) else
+      throw "Scyther version ${version} is not available, choose one of ${showVersions} instead.";


UX is key :)

infinisil · 2017-07-01T04:58:34Z

Running it on Ubuntu doesn't work (the build succeeds though). I'll try to fix that and add some simple checks

infinisil · 2017-07-01T06:08:27Z

Now doesn't have any runtime problems (the problem I mentioned before was on both Ubuntu and NixOS btw). I added a simple execution of the program as a test. I couldn't get the default installCheck phase to work (maybe because of this?), so I just put in a custom testPhase which gets run at the very end.

This package is done as far as I can tell. There are some issues when running certain protocols, but I'm 99.9% sure this is a bug in the package and has nothing to do with Nix.

0xABAB · 2017-07-01T09:37:30Z

pkgs/applications/science/programming/scyther/cli.nix

+    # For some reason it doesn't use the C89 standard which would allow __inline__
+    for file in *.*
+    do
+      substituteInPlace $file --replace "__inline__ " ""


Please do an upstream PR for this, such that we don't need to patch it ourselves. The rest of your PR looks good enough.

The issue about this version.h file comes from the fact that the python script for its generation relies on being in a git repo (so it can check the tag/rev). This could be solved by doing a full checkout, but I don't think it's worth the build time dependency of python, considering the script is dead simple.

The issue with these __inline__ attributes I have no idea how to solve, cmake is supposed to use the C89 standard but it doesn't for some reason. If somebody could show me how it's done that would be awesome.

infinisil · 2017-07-01T14:36:04Z

Ahh I just figured it out, I was able to remove the __inline__ patch, I fixed it by adding the -std=gnu89 flag explicitly, because it was only recently added (not present in versions other than master).

FRidh · 2017-07-02T09:38:52Z

Here's a PR I opened when @infinisil was trying to package this https://github.com/NixOS/nixpkgs/pull/26785/files. Note that I do not use this package.

FRidh · 2017-07-02T09:39:59Z

pkgs/applications/science/programming/scyther/cli.nix

@@ -0,0 +1,26 @@
+{ stdenv, lib, cmake, glibc, makeWrapper, fetchFromGitHub, flex, bison, source }:
+stdenv.mkDerivation { 
+  name = "scyther-cli-${source.rev}";


a source revision is not a valid version for in Nixpkgs

FRidh · 2017-07-02T09:40:46Z

pkgs/applications/science/programming/scyther/cli.nix

+    flex
+    bison
+  ];
+}


meta is missing

Thanks for the review!

Since only the drv in default.nix is intended to be used I only put meta there, it would be the same for all (internal) drvs.

FRidh · 2017-07-02T09:41:24Z

pkgs/applications/science/programming/scyther/default.nix

@@ -0,0 +1,30 @@
+{ lib, callPackage, callPackage_i686, buildEnv
+, includeGUI ? true


Why bother joining the two tools in a single output?

FRidh · 2017-07-02T09:42:22Z

pkgs/applications/science/programming/scyther/gui.nix

+
+  format = "other";
+
+  phases = [ "unpackPhase" "patchPhase" "installPhase" "fixupPhase" "testPhase" ];


don't set phases, bad practice

FRidh · 2017-07-02T09:42:58Z

pkgs/applications/science/programming/scyther/gui.nix

+
+  phases = [ "unpackPhase" "patchPhase" "installPhase" "fixupPhase" "testPhase" ];
+
+  patchPhase = ''


postPatch, try not to override the phases

I'll fix that

And my reply to your comment about joining the outputs (can't reply to that for some reason): it separates stuff nicely I think, since the source needs to be shared by both parts and the gui depends on the cli with this setup one can only install the cli if desired. I'll think about it once more but I believe it's the cleanest like this. One other solution would be to just use 2 top-level packaged as you did before

FRidh · 2017-07-02T09:43:40Z

pkgs/applications/science/programming/scyther/gui.nix

+  '';
+
+  # This would call wrapPythonPrograms which only wraps ones in $out/bin
+  dontWrapPythonPrograms = true;


if you don't want the wrappers you may as well just use stdenv.mkDerivation.

Good call, I thought about this when writing it, I'll try this

FRidh · 2017-07-02T09:48:16Z

pkgs/applications/science/programming/scyther/sources.nix

@@ -0,0 +1,33 @@
+{ lib, fetchFromGitHub }:
+let
+  versions = map (s: { name = s.rev; } // s) [


We typically provide only a single version. If users want another version, they could override and pass their own src in.

Heh, yes that is a very valid point, I thought it would be neat if it could be chosen between two releases which very much differ (very comparable to firefox and firefox-esr).

How about this: Getting rid of the master version (this can also fix the rev as version in your other comment) and making it possible to override source like all other packages?

0xABAB · 2017-07-02T17:12:31Z

pkgs/applications/science/programming/scyther/cli.nix

+  src = source;
+  preConfigure = ''
+    cd src
+    # Since we're not in a git dir, the normal command this project uses to create this file wouldn't work


This works until the moment it doesn't. Please add some kind of assertion checking that upstream didn't change their method of producing this string in a couple of versions.

0xABAB · 2017-07-02T17:18:08Z

pkgs/applications/science/programming/scyther/cli.nix

+  preConfigure = ''
+    cd src
+    # Since we're not in a git dir, the normal command this project uses to create this file wouldn't work
+    printf "#define TAGVERSION \"${source.rev}\"\n" >> version.h


The Python script writes to the file, you append to it. Your use of printf is potentially wrong depending on the value of ${source.rev}, so please use a format string.

infinisil · 2017-07-05T00:33:56Z

@FRidh @0xABAB @MostAwesomeDude I redid the whole package, putting everything into a single derivation, abandoning the 'providing different versions by default' thing. While it was a cool idea, it's not the right thing to do.

I fixed every mentioned flaw of previous review comments, except what @0xABAB said regarding the generation of the version.h file which might not work forever. Since this package is in a pretty inactive phase I doubt it will ever get so much attention to get this changed (and even if it does, I'll fix it then).

One big drawback of how its done now is that since everything (both cli and gui part) gets built using callPackage_i686, it uses 32bit everything for python, which isn't needed at all. If somebody knows how to fix this, let me know, I'll think about it some more too.

0xABAB · 2017-07-10T08:50:45Z

pkgs/applications/science/programming/scyther/default.nix

+
+  patchPhase = ''
+    # Since we're not in a git dir, the normal command this project uses to create this file wouldn't work
+    printf "%s\n" "#define TAGVERSION \"${version}\"" >> src/version.h


You are still appending (>>) instead of writing (>) to src/version.h.

0xABAB · 2017-07-10T08:52:24Z

pkgs/applications/science/programming/scyther/default.nix

+
+
+  configurePhase = ''
+    pushd src


(cd src && cmakeConfigurePhase) (including the parentheses does the same, right? (And is shorter and more portable.)

0xABAB · 2017-07-10T08:55:50Z

pkgs/applications/science/programming/scyther/default.nix

+  ];
+
+  dontUseCmakeBuildDir = true;
+  cmakeFlags = [ "-DCMAKE_C_FLAGS=\"-std=gnu89\"" ];


"-DCMAKE_C_FLAGS=-std=gnu89" does the same.

joachifm · 2017-07-15T20:16:06Z

pkgs/applications/science/programming/scyther/default.nix

+, includeProtocols ? true
+}:
+let
+  version = "v1.1.3";


This version string is invalid. See builtins.parseDrvName. A version string must begin with a digit.

joachifm · 2017-07-15T20:16:25Z

pkgs/applications/science/programming/scyther/default.nix

+stdenv.mkDerivation {
+  name = "scyther-${version}";
+  src = fetchFromGitHub {
+    rev = "v1.1.3";


Once the version has been fixed, you can do rev = v${version}.

Thanks, didn't notice that, fixed now

infinisil · 2017-07-15T21:25:51Z

Pinging @FRidh, the things you mentioned have all been corrected in the totally new revision of this package.

infinisil · 2017-07-15T21:38:32Z

Can somebody tell my why travis complains about wrong platform on Darwin even though I explicitly set platforms to linux only? https://travis-ci.org/NixOS/nixpkgs/jobs/254011342#L1544

grahamc · 2017-07-16T20:59:44Z

pkgs/applications/science/programming/scyther/default.nix

+    (cd src && cmakeConfigurePhase)
+  '';
+
+  propagatedBuildInputs = [] ++ lib.optional includeGUI [


This can omit the [] ++

grahamc · 2017-07-16T21:22:25Z

LGTM if @FRidh doesn't review and/or merge soon, let me know.

FRidh · 2017-07-17T08:10:31Z

One big drawback of how its done now is that since everything (both cli and gui part) gets built using callPackage_i686, it uses 32bit everything for python, which isn't needed at all. If somebody knows how to fix this, let me know, I'll think about it some more too.

That's why its better to build it in two separate derivations. The one, that just builds that little binary, and the other that puts it all together in one derivation.

FRidh · 2017-07-17T08:14:11Z

Can somebody tell my why travis complains about wrong platform on Darwin even though I explicitly set platforms to linux only? https://travis-ci.org/NixOS/nixpkgs/jobs/254011342#L1544

You set platforms correct, but callPackage_i686 assumes an i686 Linux system. See line 30 in all-packages.nix

callPackage_i686 = pkgsi686Linux.callPackage;

You can ignore this error because its expected behavior.

infinisil · 2017-07-23T00:07:09Z

@FRidh I'll make another revision to separate the two parts

infinisil · 2017-07-23T01:23:14Z

@FRidh I separated the package into two versions so it doesn't require 32-bit versions of everything. One thing I changed is the inclusion of protocols. If includeGUI && includeProtocols, then they are included twice, but I don't think it's worth symlinking it (it's only 1MB).

Since some things changed, I'd love the other reviewers to have another look (especially the buildEnv part)

infinisil · 2017-08-01T04:16:25Z

@FRidh ping

MostAwesomeDude reviewed Jul 1, 2017

View reviewed changes

0xABAB reviewed Jul 1, 2017

View reviewed changes

FRidh requested changes Jul 2, 2017

View reviewed changes

0xABAB reviewed Jul 2, 2017

View reviewed changes

pSub added the 8.has: package (new) label Jul 3, 2017

0xABAB reviewed Jul 10, 2017

View reviewed changes

joachifm reviewed Jul 15, 2017

View reviewed changes

FRidh self-assigned this Jul 16, 2017

grahamc reviewed Jul 16, 2017

View reviewed changes

scyther: init at 1.1.3

75fde41

infinisil force-pushed the scyther branch from 3141937 to 75fde41 Compare July 16, 2017 21:40

scyther: Separated into two derivations

313b8b7

FRidh merged commit d1af3b3 into NixOS:master Aug 1, 2017

		@@ -0,0 +1,30 @@
		{ lib, callPackage, callPackage_i686, buildEnv
		, includeGUI ? true


		format = "other";

		phases = [ "unpackPhase" "patchPhase" "installPhase" "fixupPhase" "testPhase" ];


		phases = [ "unpackPhase" "patchPhase" "installPhase" "fixupPhase" "testPhase" ];

		patchPhase = ''

scyther: init at 1.1.3 #27006

scyther: init at 1.1.3 #27006

Conversation

infinisil commented Jul 1, 2017 • edited

Motivation for this change

Things done

MostAwesomeDude left a 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

infinisil commented Jul 1, 2017

infinisil commented Jul 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

infinisil commented Jul 1, 2017 • edited

FRidh commented Jul 2, 2017

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

infinisil commented Jul 5, 2017 • 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

infinisil commented Jul 15, 2017

infinisil commented Jul 15, 2017

Choose a reason for hiding this comment

grahamc commented Jul 16, 2017

FRidh commented Jul 17, 2017

FRidh commented Jul 17, 2017

infinisil commented Jul 23, 2017

infinisil commented Jul 23, 2017

infinisil commented Aug 1, 2017

infinisil commented Jul 1, 2017 •

edited

infinisil commented Jul 1, 2017 •

edited

infinisil commented Jul 5, 2017 •

edited