Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

NetLogo: init 6.0.4 #49376

Merged
merged 1 commit into from Nov 22, 2018
Merged

NetLogo: init 6.0.4 #49376

merged 1 commit into from Nov 22, 2018

Conversation

dpaetzel
Copy link
Contributor

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.

@dpaetzel dpaetzel force-pushed the package-netlogo branch 3 times, most recently from a52e40e to 218480d Compare October 29, 2018 08:45
@dpaetzel dpaetzel force-pushed the package-netlogo branch 2 times, most recently from 8c80a3d to c7c4f66 Compare October 29, 2018 12:19
@dpaetzel dpaetzel force-pushed the package-netlogo branch 2 times, most recently from 5b073d4 to e12767c Compare November 12, 2018 07:20
worldofpeace
worldofpeace reviewed Nov 12, 2018
View reviewed changes
pkgs/applications/science/misc/netlogo/default.nix Outdated

installPhase = ''
mkdir -pv $out/share/netlogo
cp -rv app readme.md $out/share/netlogo
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this readme.md will be useful in the derivation.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be honest, I'm not sure whether that README has to be included because it documents which libraries are used within the application?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is some useful documentation, so maybe it can go to $out/share/doc.

@worldofpeace
Copy link
Contributor

I noticed that they aren't distributing a desktop file, so usually it's best to generate one with makeDesktopItem and install it at the installPhase.

I went ahead and wrote a patch if you would like to include it:

diff --git a/pkgs/applications/science/misc/netlogo/default.nix b/pkgs/applications/science/misc/netlogo/default.nix
index 241fc6ed17c..40000f6e97e 100644
--- a/pkgs/applications/science/misc/netlogo/default.nix
+++ b/pkgs/applications/science/misc/netlogo/default.nix
@@ -1,4 +1,17 @@
-{ jre, stdenv, fetchurl, makeWrapper }:
+{ jre, stdenv, fetchurl, makeWrapper, makeDesktopItem }:
+
+let
+
+  netlogo = makeDesktopItem rec {
+    name = "netlogo";
+    exec = name;
+    icon = name;
+    comment = "A multi-agent programmable modeling environment";
+    desktopName = "NetLogo";
+    categories = "Science;";
+  };
+
+in
 
 stdenv.mkDerivation rec {
   name = "NetLogo-${version}";
@@ -9,16 +22,26 @@ stdenv.mkDerivation rec {
     sha256 = "0dcd9df4dfb218826a74f9df42163fa588908a1dfe58864106936f8dfb76acec";
   };
 
+  src1 = fetchurl {
+    name = "netlogo.png";
+    url = "https://netlogoweb.org/assets/images/desktopicon.png";
+    sha256 = "1i43lhr31lzva8d2r0dxpcgr58x496gb5vmb0h2da137ayvifar8";
+  };
+
   nativeBuildInputs = [ makeWrapper ];
 
   installPhase = ''
-    mkdir -pv $out/share/netlogo
+    mkdir -pv $out/share/netlogo $out/share/icons/hicolor/256x256/apps $out/share/applications
     cp -rv app readme.md $out/share/netlogo
 
     # launcher with `cd` is required b/c otherwise the model library isn't usable
     makeWrapper "${jre}/bin/java" "$out/bin/netlogo" \
       --run "cd $out/share/netlogo/app" \
       --add-flags "-jar netlogo-${version}.jar"
+
+    cp $src1 $out/share/icons/hicolor/256x256/apps/netlogo.png
+    cp ${netlogo}/share/applications/* $out/share/applications
+    
   '';
 
   meta = with stdenv.lib; {

The addition of src1 is just downloading a usable icon from their website.

@worldofpeace
Copy link
Contributor

@dpaetzel This looks good to merge, just need to test it one more time.

One last nit-pick would be that name = "NetLogo-${version}"; doesn't match the derivation naming guidelines, where everything should just be lowercase. So netlogo.

@dpaetzel
Copy link
Contributor Author

One last nit-pick would be that name = "NetLogo-${version}"; doesn't match the derivation naming guidelines, where everything should just be lowercase. So netlogo.

Oh, you're right. Does that mean that the commit message should also be “netlogo: …”?

@worldofpeace
Copy link
Contributor

Yep 👍

worldofpeace
worldofpeace approved these changes Nov 22, 2018
View reviewed changes
Copy link
Contributor

@worldofpeace worldofpeace left a comment •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dpaetzel Sorry for the wait and the fuss. This looks good.

@worldofpeace worldofpeace merged commit 2e70b40 into NixOS:master Nov 22, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@symphorien symphorien symphorien left review comments

@worldofpeace worldofpeace worldofpeace approved these changes

Assignees
No one assigned
Labels
8.has: package (new) 10.rebuild-darwin: 0 10.rebuild-linux: 1-10
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@dpaetzel @worldofpeace @symphorien @GrahamcOfBorg