global-platform-pro: init at 0.3.10-rc11 #44288

Ekleog · 2018-08-01T04:55:47Z

Motivation for this change

Things done

Tested using sandboxing (nix.useSandbox on NixOS, or option 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/)
Determined the impact on package closure size (by running nix path-info -S before and after)
Fits CONTRIBUTING.md.

GrahamcOfBorg · 2018-08-01T05:07:48Z

No attempt on aarch64-linux (full log)

The following builds were skipped because they don't evaluate on aarch64-linux: global-platform-pro

Partial log (click to expand)


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

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

GrahamcOfBorg · 2018-08-01T05:10:15Z

Success on x86_64-linux (full log)

Attempted: global-platform-pro

Partial log (click to expand)

[INFO] ------------------------------------------------------------------------
installing
post-installation fixup
moving /nix/store/ndlq42l31y84gh3x1kprycz3bm65n485-global-platform-pro-0.3.10-rc11/doc to /nix/store/ndlq42l31y84gh3x1kprycz3bm65n485-global-platform-pro-0.3.10-rc11/share/doc
shrinking RPATHs of ELF executables and libraries in /nix/store/ndlq42l31y84gh3x1kprycz3bm65n485-global-platform-pro-0.3.10-rc11
strip is /nix/store/1hi76hr87bd1y1q1qjk0lv8nmcjip1c8-binutils-2.30/bin/strip
stripping (with command strip and flags -S) in /nix/store/ndlq42l31y84gh3x1kprycz3bm65n485-global-platform-pro-0.3.10-rc11/lib  /nix/store/ndlq42l31y84gh3x1kprycz3bm65n485-global-platform-pro-0.3.10-rc11/bin
patching script interpreter paths in /nix/store/ndlq42l31y84gh3x1kprycz3bm65n485-global-platform-pro-0.3.10-rc11
checking for references to /build in /nix/store/ndlq42l31y84gh3x1kprycz3bm65n485-global-platform-pro-0.3.10-rc11...
/nix/store/ndlq42l31y84gh3x1kprycz3bm65n485-global-platform-pro-0.3.10-rc11

timokau · 2018-08-15T09:25:24Z

Why are you packaging 0.3.10-rc11 instead of 0.3.9?

timokau · 2018-08-15T09:27:31Z

pkgs/development/tools/global-platform-pro/default.nix

+        sha256 = "0rk81x2y7vx1caxm6wa59fjrfxmjn7s8yxaxm764p8m2qxk3m4y2";
+    };
+
+    patches = [ (writeText "${name}-version.patch" ''


What does this do? Is there an upstream report for it? I think it would be better to put the patch into a separate file. Then add a comment describing what it fixes and a link to upstream.

That's not something that can be upstreamed, unfortunately. It's basically bypassing the build system, that attempts to run git describe --tags --always --long --dirty which fails due to fetchFromGitHub not keeping the git repo.

This patch hardcodes the output based on the describeVersion variable, which means it can't really be split into a separate file, or else the describeVersion would also have to be in this separate file, and an updater may easily forget to update it when bumping the version.

I've written a comment to explain it :)

Oh, that makes sense. But while this exact change cannot be upstreamed, we should notify upstream that relying on git describe is not ideal. Maybe they'll change it (why do they actually need that?) or at least provide a nicer way to override it at build time.

Generating a patch based on a template seems a bit fishy to me, but I can't think of a better solution either.

I can't find the gephy package, could you ping the author?

Ugh, it's gephi, not gephy, sorry! I'm not sure on what part of the topic you want me to tag them, though, so I'll let you tag Radvendii where you want :)

timokau · 2018-08-15T09:29:55Z

pkgs/development/tools/global-platform-pro/default.nix

+        inherit src patches;
+        nativeBuildInputs = [ jdk maven ];
+        buildPhase = ''
+            while mvn package -Dmaven.repo.local=$out/.m2 -Dmaven.wagon.rto=5000; [ $? = 1 ]; do


please quote the shell variable

isn't this equivalent to while ! mvn package ...?

is it possible to end up in an endless loop?

what does that actually do? Download shouldn't be possible in the buildPhase

This is part of the code copy/pasted and adapted from the gephy package. Basically copied it and twisted it until it compiled, I don't really understand maven :/

That said for your questions:

Done

I… think it is. Tried building with while ! mvn package … and it appears to build, at least :)

AFAIK it just re-attempts downloading the dependencies every time the download fails. I guess the author of the gephy derivation put it there due to unreliability of the maven repository, and so let it here because it's hard to know whether I'm just having luck downloading all at once or not :)

It can download thanks to it being a fixed-output derivation :) (the outputHash* variables)

I… think it is. Tried building with while ! mvn package … and it appears to build, at least :)

Then I think using that would be a bit nicer :) Although it is not 100% equivalent since ! command will be true at any exit code other than 0, I think that is what is meant anyways.

I don't find any mention of outputHash, what do I miss?

It would probably be valuable to get the author of gephy's expression here.

It's the three lines at https://github.com/NixOS/nixpkgs/pull/44288/files/cccfa8f3953933147757d6790ad025e88de32dc0#diff-3f80555b27d7cb249ab01d104b052e6eR52 :)

For the equivalence, I'd guess maybe mvn package returns a different error code depending on whether it's a download failure or some other kind of failure? cc @Radvendii

And why is it run in a while loop in the first place @Radvendii?

timokau · 2018-08-15T09:32:40Z

pkgs/development/tools/global-platform-pro/default.nix

+            done
+        '';
+        # keep only *.{pom,jar,sha1,nbm} and delete all ephemeral files with lastModified timestamps inside
+        installPhase = ''find $out/.m2 -type f -regex '.+\(\.lastUpdated\|resolver-status\.properties\|_remote\.repositories\)' -delete'';


(damn that is an ugly regex syntax)

Please quote the shell variable and maybe put the command into its own line:

installPhase = '' # command ''

Where is this actually installing anything?

Done, and split the line :) (even though it's $out so it's guaranteed to not contain spaces, I do understand what you mean by wanting to always quote variables :))

As for where this is installing anything, my understanding of this code (from gephy) is that the buildPhase is doing the downloading, and the installPhase is doing the cleanup. This is similar to the usual way of doing the build in the buildPhase and then copying only what's required in the installPhase, so I think it makes more or less sense? :)

even though it's $out so it's guaranteed to not contain spaces, I do understand what you mean by wanting to always quote variables :)

IIRC it is possible to change the store location. So if I'd change it to /nix/s t o r e/, $out would contain spaces. Also it is easier to always quote on principle :)

Oh so buildPhase downloads directly to $out. I guess it might make sense, although I'd just put everything into installPhase since there is no actual building involved.

Done, it looks even more like buildRustPackage this way :)

There probably is room for generalization, but I don't know maven either.

timokau · 2018-08-15T09:34:03Z

pkgs/development/tools/global-platform-pro/default.nix

+    nativeBuildInputs = [ jdk maven makeWrapper ];
+
+    buildPhase = ''
+        mvn package --offline -Dmaven.repo.local=$( \


Please quote the $(...)

timokau · 2018-08-15T09:34:21Z

pkgs/development/tools/global-platform-pro/default.nix

+
+    buildPhase = ''
+        mvn package --offline -Dmaven.repo.local=$( \
+            cp -dpR ${deps}/.m2 ./ && \


I've simplified a bit this part of the code on my way :) should have paid more attention to having simple code when I first packaged it, I guess.

timokau · 2018-08-15T09:35:11Z

pkgs/development/tools/global-platform-pro/default.nix

+    '';
+
+    installPhase = ''
+        mkdir -p $out/lib/java $out/share/java


timokau · 2018-08-15T09:35:39Z

pkgs/development/tools/global-platform-pro/default.nix

+
+    installPhase = ''
+        mkdir -p $out/lib/java $out/share/java
+        cp -R target/apidocs $out/doc


timokau · 2018-08-15T09:35:48Z

pkgs/development/tools/global-platform-pro/default.nix

+    installPhase = ''
+        mkdir -p $out/lib/java $out/share/java
+        cp -R target/apidocs $out/doc
+        cp target/gp.jar $out/share/java


timokau · 2018-08-15T09:36:01Z

pkgs/development/tools/global-platform-pro/default.nix

+        mkdir -p $out/lib/java $out/share/java
+        cp -R target/apidocs $out/doc
+        cp target/gp.jar $out/share/java
+        makeWrapper ${jre_headless}/bin/java $out/bin/gp \


timokau · 2018-08-15T09:36:24Z

pkgs/development/tools/global-platform-pro/default.nix

+        cp -R target/apidocs $out/doc
+        cp target/gp.jar $out/share/java
+        makeWrapper ${jre_headless}/bin/java $out/bin/gp \
+            --add-flags "-jar $out/share/java/gp.jar" \


And one last time 😀

Ekleog · 2018-08-16T15:26:19Z

Why are you packaging 0.3.10-rc11 instead of 0.3.9?

That's a good question. The honest answer is, I didn't pay attention. However, giving it a bit more thorough look, it seems that 0.3.9 is 2 years old. Across Debian, rpmfind, Gentoo and Archlinux, the only equivalent package appears to be globalplatformpro-git from AUR, that is 0.3.10-rc8.[somegitrev].

I don't have strong opinions on rolling back to 0.3.9, though :) it looks like it's a package a bit like tinc: the pre-release is stable, and the officially-stable version is ancient -- but I don't have many data points to prove this.

[review comments]

I've replied to some of them, should have fixed everything in my last commit! Also just noticed I used 4-spaces indentation on this file, for my defense it wasn't my usual computer.

I've pushed two commits for ease of review, should all be squashed before merge :)

Ekleog · 2018-08-16T15:26:35Z

Oh, and forgot to mention: thank you for your exhaustive review! :)

timokau · 2018-08-16T15:32:22Z

On 18-08-16 08:27, Léo Gaspard wrote: > Why are you packaging 0.3.10-rc11 instead of 0.3.9? That's a good question. The honest answer is, I didn't pay attention. However, giving it a bit more thorough look, it seems that 0.3.9 is 2 years old. Across Debian, rpmfind, Gentoo and Archlinux, the only equivalent package appears to be globalplatformpro-git from AUR, that is 0.3.10-rc8.[somegitrev]. I don't have strong opinions on rolling back to 0.3.9, though :) it looks like it's a package a bit like tinc: the pre-release is stable, and the officially-stable version is ancient -- but I don't have many data points to prove this.

In that case I think packaging the rc is okay, especially because its a release candidate and not a beta. But if you want to do that, please open an upstream issue asking for a new release and link to that in a comment by the `version` attribute.

> [review comments] I've replied to some of them, should have fixed everything in my last commit! Also just noticed I used 4-spaces indentation on this file, for my defense it wasn't my usual computer.

Alright I'll re-review. I would love if nix used some sensible indentation but of course the best indentation rule to follow is always the one of the project :)

I've pushed two commits for ease of review, should all be squashed before merge :)

Thanks!

Oh, and forgot to mention: thank you for your exhaustive review! :)

Thank you for understanding. I know that exhaustive review can be exhausting, but I think we should strive for a high quality standard :)

GrahamcOfBorg · 2018-08-16T15:40:11Z

No attempt on aarch64-linux (full log)

The following builds were skipped because they don't evaluate on aarch64-linux: global-platform-pro

Partial log (click to expand)


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

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

GrahamcOfBorg · 2018-08-16T15:41:02Z

Success on x86_64-linux (full log)

Attempted: global-platform-pro

Partial log (click to expand)

[INFO] Finished at: 2018-08-16T15:41:00Z
[INFO] ------------------------------------------------------------------------
installing
post-installation fixup
moving /nix/store/wz4g13jqcwrncqi2lz0aqpyka2iqccyp-global-platform-pro-0.3.10-rc11/doc to /nix/store/wz4g13jqcwrncqi2lz0aqpyka2iqccyp-global-platform-pro-0.3.10-rc11/share/doc
shrinking RPATHs of ELF executables and libraries in /nix/store/wz4g13jqcwrncqi2lz0aqpyka2iqccyp-global-platform-pro-0.3.10-rc11
strip is /nix/store/gpc2wld1s0c6qzx9326cwn1wcx29xzsj-binutils-2.30/bin/strip
stripping (with command strip and flags -S) in /nix/store/wz4g13jqcwrncqi2lz0aqpyka2iqccyp-global-platform-pro-0.3.10-rc11/lib  /nix/store/wz4g13jqcwrncqi2lz0aqpyka2iqccyp-global-platform-pro-0.3.10-rc11/bin
patching script interpreter paths in /nix/store/wz4g13jqcwrncqi2lz0aqpyka2iqccyp-global-platform-pro-0.3.10-rc11
checking for references to /build in /nix/store/wz4g13jqcwrncqi2lz0aqpyka2iqccyp-global-platform-pro-0.3.10-rc11...

timokau

The changes look good to me 👍

The two remaining things I'd like to see are

upstream report about the use of git describe
upstream request for a new version
input from the gephy author

Ekleog · 2018-08-16T16:34:21Z

I think your first two points are fixed as per the latest push, let's wait for @Radvendii 's opinion about the while ! mvn package question, then :)

timokau · 2018-08-16T16:37:23Z

Perfect, thank you! If @Radvendii doesn't respond until lets say monday, ping me and I'll merge.

GrahamcOfBorg · 2018-08-16T16:44:58Z

No attempt on aarch64-linux (full log)

The following builds were skipped because they don't evaluate on aarch64-linux: global-platform-pro

Partial log (click to expand)


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

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

GrahamcOfBorg · 2018-08-16T16:45:46Z

Success on x86_64-linux (full log)

Attempted: global-platform-pro

Partial log (click to expand)

[INFO] Finished at: 2018-08-16T16:45:45Z
[INFO] ------------------------------------------------------------------------
installing
post-installation fixup
moving /nix/store/wz4g13jqcwrncqi2lz0aqpyka2iqccyp-global-platform-pro-0.3.10-rc11/doc to /nix/store/wz4g13jqcwrncqi2lz0aqpyka2iqccyp-global-platform-pro-0.3.10-rc11/share/doc
shrinking RPATHs of ELF executables and libraries in /nix/store/wz4g13jqcwrncqi2lz0aqpyka2iqccyp-global-platform-pro-0.3.10-rc11
strip is /nix/store/gpc2wld1s0c6qzx9326cwn1wcx29xzsj-binutils-2.30/bin/strip
stripping (with command strip and flags -S) in /nix/store/wz4g13jqcwrncqi2lz0aqpyka2iqccyp-global-platform-pro-0.3.10-rc11/lib  /nix/store/wz4g13jqcwrncqi2lz0aqpyka2iqccyp-global-platform-pro-0.3.10-rc11/bin
patching script interpreter paths in /nix/store/wz4g13jqcwrncqi2lz0aqpyka2iqccyp-global-platform-pro-0.3.10-rc11
checking for references to /build in /nix/store/wz4g13jqcwrncqi2lz0aqpyka2iqccyp-global-platform-pro-0.3.10-rc11...

Ekleog · 2018-08-20T15:33:07Z

@timokau Ping for merge

Ekleog · 2018-08-21T00:53:36Z

Thank you! :)

global-platform-pro: init at 0.3.10-rc11

cccfa8f

GrahamcOfBorg added 8.has: package (new) 10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10 labels Aug 1, 2018

timokau requested changes Aug 15, 2018

View reviewed changes

Ekleog added 2 commits August 17, 2018 00:19

fixup: points mentioned by timokau

a52ad25

fixup: reindent

39cd3f3

timokau reviewed Aug 16, 2018

View reviewed changes

Ekleog added 3 commits August 17, 2018 01:13

fixup: use the correct gephy package name

61e0b8d

fixup: merge buildPhase and installPhase

fd14b4d

fixup: add links to upstream issues

6195eee

timokau approved these changes Aug 16, 2018

View reviewed changes

timokau merged commit f8f6ab3 into NixOS:master Aug 20, 2018

global-platform-pro: init at 0.3.10-rc11 #44288

global-platform-pro: init at 0.3.10-rc11 #44288

Conversation

Ekleog commented Aug 1, 2018 • edited

Motivation for this change

Things done

GrahamcOfBorg commented Aug 1, 2018

GrahamcOfBorg commented Aug 1, 2018

timokau commented Aug 15, 2018

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

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

Ekleog commented Aug 16, 2018

Ekleog commented Aug 16, 2018

timokau commented Aug 16, 2018 via email

GrahamcOfBorg commented Aug 16, 2018

GrahamcOfBorg commented Aug 16, 2018

timokau left a comment

Choose a reason for hiding this comment

Ekleog commented Aug 16, 2018

timokau commented Aug 16, 2018

GrahamcOfBorg commented Aug 16, 2018

GrahamcOfBorg commented Aug 16, 2018

Ekleog commented Aug 20, 2018

Ekleog commented Aug 21, 2018

Ekleog commented Aug 1, 2018 •

edited