cplex: init at 12.8 #44029

bfortz · 2018-07-23T20:37:20Z

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.

infinisil · 2018-07-24T21:54:22Z

pkgs/applications/science/math/cplex/default.nix

+  nativeBuildInputs = [ coreutils makeWrapper ];
+  buildInputs = [ openjdk gtk2 xorg.libXtst glibcLocales ];
+
+  phases = "installPhase";


Don't assign phases directly, instead assign the parts of the script the role they have: The first sed is patchPhase, the sh buildPhase, put the ln's in installPhase and the patchelf/wrapProgram in fixupPhase.

infinisil · 2018-07-24T21:58:14Z

pkgs/applications/science/math/cplex/default.nix

+    for pgm in $out/opl/bin/x86-64_linux/oplrun $out/opl/bin/x86-64_linux/oplrunjava $out/opl/oplide/oplide;
+    do
+      patchelf --set-interpreter "$interpreter" $pgm;
+      wrapProgram $pgm --prefix LD_LIBRARY_PATH : $out/opl/bin/x86-64_linux:${stdenv.lib.makeLibraryPath [ stdenv.cc.cc gtk2 xorg.libXtst ]} --set LOCALE_ARCHIVE ${glibcLocales}/lib/locale/locale-archive;


You can use \ to break lines and put a libraryPath = stdenv.lib.makeLibraryPath [ ... ]; definition in a let in to keep the line length short:

fixupPhase = let libraryPath = ... in '' ... wrapProgram $pgm \ --prefix LD_LIBRARY_PATH : $out/opl/bin/x86-64_linux:${libraryPath} \ --set LOCALE_ARCHIVE ${glibcLocales}/lib/locale/locale-archive;

infinisil · 2018-07-24T21:58:36Z

pkgs/applications/science/math/cplex/default.nix

+    do
+      if grep ELF $pgm > /dev/null;
+      then
+        patchelf --set-interpreter "$interpreter" $pgm;


I'm pretty sure patchelf already checks whether it's an ELF binary or not. You could also then merge the patchelf loops.

infinisil · 2018-07-24T22:01:29Z

pkgs/applications/science/math/cplex/default.nix

+  installer = "cplex_studio${version}.linux-x86-64.bin";
+
+  src = requireFile rec {
+    name = "${installer}";


You can just assign name = "cplex_studio${version}.linux-x86-64.bin"; directly here. And there's no need to use the $installer variable down below I'd think

bfortz · 2018-08-03T12:20:15Z

@infinisil thanks for the suggestions, indeed the code is much better now. I also changed the source definition, copying from what is done in the renoise build, because the dowloaded archive is likely to be personalized and sha256 check would fail. Note that patchelf fails on non-ELF files.

infinisil · 2018-08-03T14:43:22Z

pkgs/applications/science/math/cplex/default.nix

+  nativeBuildInputs = [ coreutils makeWrapper ];
+  buildInputs = [ openjdk gtk2 xorg.libXtst glibcLocales ];
+
+  phases = "patchPhase buildPhase installPhase fixupPhase";


I meant to remove this line completely, these phases are set like this automatically

How can I disable unpackPhase? I tried to define an empty one, but it did not work (and it complains it does not know how to unpack the cplex bin archive if I do not remove the phase completely).

Maybe unpackPhase = "cp $src ." will work

cp $src $name

infinisil · 2018-08-03T14:43:46Z

pkgs/applications/science/math/cplex/default.nix

+    else
+      releasePath;
+
+  nativeBuildInputs = [ coreutils makeWrapper ];


Pretty sure coreutils are present by default, no need to add them here

infinisil · 2018-08-03T14:44:46Z

pkgs/applications/science/math/cplex/default.nix

+      throw ''
+        This nix expression requires that the cplex installer is already
+        downloaded to your machine. Get it from IBM and override the
+        releasePath attribute to point to the location of the file. 


Can you add the link to where one can download it in this error message?

infinisil · 2018-08-03T14:46:08Z

pkgs/applications/science/math/cplex/default.nix

+  phases = "patchPhase buildPhase installPhase fixupPhase";
+
+  patchPhase = ''
+    sed -e 's|/usr/bin/tr"|tr"         |' $src > $name


Use sed -i to modify the file inplace, no need to > $name

But $src is the original file, I don't want to modify that one. (The other option would be to first copy the file in the unpack phase...)

With the cp in the unpackPhase, sed -i does the job. (BTW, I did not use the builtin substituteInPlace because it is way too slow on such a large file).

infinisil · 2018-08-03T14:48:32Z

pkgs/applications/science/math/cplex/default.nix

+    for pgm in $out/opl/bin/x86-64_linux/oplrun $out/opl/bin/x86-64_linux/oplrunjava $out/opl/oplide/oplide $out/cplex/bin/x86-64_linux/cplex $out/cpoptimizer/bin/x86-64_linux/cpoptimizer;
+    do
+      ln -s $pgm $out/bin;
+    done


You can make this a single command and use some newlines:

ln -s $out/opl/bin/x86-64_linux/oplrun \ $out/opl/bin/x86-64_linux/oplrunjava \ ... \ $out/bin

infinisil · 2018-08-03T14:52:42Z

pkgs/applications/science/math/cplex/default.nix

@@ -0,0 +1,75 @@
+{ stdenv, coreutils, requireFile, patchelf, makeWrapper, openjdk, gtk2, xorg, glibcLocales, releasePath ? null }:


Can remove patchelf, coreutils and requireFile from here

infinisil

Sorry for the long review process, I hope you don't mind! I asked around, and apparently some people are against including derivations that need such special ways of fetching sources: #13625. I think it's alright for this one though :)

infinisil · 2018-08-04T14:23:26Z

pkgs/applications/science/math/cplex/default.nix

@@ -0,0 +1,81 @@
+{ stdenv, makeWrapper, openjdk, gtk2, xorg, glibcLocales, releasePath ? null }:


Alternate suggestion I think I'd prefer: Use the config attribute here instead, and use config.cplex.releasePath to get the path. Then users using nix-env will have it easy to make it work, it just needs a { cplex.releasePath = ...; } in their ~/.config/nixpkgs/config.nix. This could then also be documented in the error message: "Set cplex.releasePath = /path/to/download; in your ~/.config/nixpkgs/config.nix for nix-* commands, or config.cplex.releasePath = /path/to/download; in your configuration.nix for NixOS ..."

No need to apologize, I appreciate the feedback as I am quite a newbie here, and is good to have some mentoring. I like the config idea, it should also be ported to renoise to imho (I inspired myself from the renoise build for this one).

Here it is. I hope I did it the right way...

infinisil · 2018-08-13T17:56:16Z

Thanks! The only thing left to do is squash all commits into one

cplex: init at 12.8

a129033

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

Removed bundled java from bin directory

d7f9e30

infinisil requested changes Jul 24, 2018

View reviewed changes

bfortz added 3 commits July 31, 2018 11:50

Merge remote-tracking branch 'upstream/master' into cplex

3bae632

Merge remote-tracking branch 'upstream/master' into cplex

97473f9

Refactored

1caca87

GrahamcOfBorg added 10.rebuild-linux: 0 and removed 10.rebuild-linux: 1-10 labels Aug 3, 2018

infinisil requested changes Aug 3, 2018

View reviewed changes

More refactoring

ba2d7e8

infinisil requested changes Aug 4, 2018

View reviewed changes

bfortz added 2 commits August 4, 2018 17:46

Added config option for releasePath

f702057

Minor cleanup

7aa1e37

infinisil approved these changes Aug 13, 2018

View reviewed changes

matthewbauer merged commit 22c15ab into NixOS:master Aug 16, 2018

bfortz deleted the cplex branch August 16, 2018 06:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cplex: init at 12.8 #44029

cplex: init at 12.8 #44029

bfortz commented Jul 23, 2018

infinisil Jul 24, 2018

infinisil Jul 24, 2018

infinisil Jul 24, 2018

infinisil Jul 24, 2018

bfortz commented Aug 3, 2018

infinisil Aug 3, 2018

bfortz Aug 3, 2018

infinisil Aug 3, 2018

bfortz Aug 4, 2018

infinisil Aug 3, 2018

infinisil Aug 3, 2018

bfortz Aug 4, 2018

infinisil Aug 3, 2018

bfortz Aug 3, 2018

bfortz Aug 4, 2018

infinisil Aug 3, 2018

bfortz Aug 4, 2018

infinisil Aug 3, 2018

bfortz Aug 4, 2018

infinisil left a comment

infinisil Aug 4, 2018

bfortz Aug 4, 2018

bfortz Aug 4, 2018

infinisil commented Aug 13, 2018

		@@ -0,0 +1,75 @@
		{ stdenv, coreutils, requireFile, patchelf, makeWrapper, openjdk, gtk2, xorg, glibcLocales, releasePath ? null }:

		@@ -0,0 +1,81 @@
		{ stdenv, makeWrapper, openjdk, gtk2, xorg, glibcLocales, releasePath ? null }:

cplex: init at 12.8 #44029

cplex: init at 12.8 #44029

Conversation

bfortz commented Jul 23, 2018

Motivation for this change

Things done

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

bfortz commented Aug 3, 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

infinisil 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

infinisil commented Aug 13, 2018