gildas: init at jul17a #27138

bzizou · 2017-07-05T07:21:08Z

Motivation for this change

Gildas is a radio-astronomy code that is used into my HPC center.
This package has been successfully tested by our users and is now in production since a few weeks into our local channel.

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.

0xABAB · 2017-07-06T07:34:31Z

pkgs/applications/science/astronomy/gildas/default.nix

+{ stdenv, fetchurl, gtk2 , pkgconfig , python27 , gfortran , python27Packages , lesstif , cfitsio , getopt , perl , groff }:
+
+stdenv.mkDerivation rec {
+  version = "feb17d";


Are you sure the version doesn't break version sorting?

I would like to see a version like 20170217_d (for the exact format, grep the repository to see other examples with dates in the version), if this assumption is invalid.

I agree that this can break version sorting, but I'm not responsible for the versioning of Gildas... I'll propose a different version number for the nix package so...

@bzizou In order to make your life (and that of everyone else) easier, you could e-mail the author of Gildas. Alternatively, you could parse the format they are using and compute the version based on their version. This way, if you ever automate the update process, there is no manual work required anymore.

OK, already suggested to my contact in the Gildas team. I think that I'll do something simple for now and see if they changed the thing for the next release

Note that versions in Nix must start with a digit. Otherwise nix-env will consider it part of the package name. So e.g. nix-env -i gildas won't work.

If versions must start with a digit, then we should have an assertion somewhere.

0xABAB · 2017-07-06T07:36:10Z

pkgs/applications/science/astronomy/gildas/default.nix

+    cp admin/wrapper.sh $out/bin/gildas-wrapper.sh
+    chmod 755 $out/bin/gildas-wrapper.sh
+    for i in $out/libexec/bin/* ; do
+      ln -s $out/bin/gildas-wrapper.sh $out/bin/`basename $i`


Please replace with "$(basename "$i")"

0xABAB · 2017-07-06T07:37:05Z

pkgs/applications/science/astronomy/gildas/default.nix

+  meta = {
+    description = "Radioastronomy data analysis software";
+    longDescription = ''
+      GILDAS is a collection of state-of-the-art softwares


Replace softwares with software.

0xABAB · 2017-07-06T07:40:38Z

pkgs/applications/science/astronomy/gildas/format-security.patch

+    sprintf( exec_argv[i++], "%s", app_name);
+     exec_argv[i++] = NULL;
+     if (i >= MAX_ARGC) {
+         gcore_c_message(seve_e, "SIC", "Too much arguments, exiting");


much -> many

Also please output how many arguments were given and how many was the maximum expected.

Well, this comes from the upstream code. I can also patch this, and I4ll suggest the patch to the upstream

If it's upstream code, please suggest it to upstream and don't spend the time to make the change here.

Done: made the suggestion to my private contact in the team (and by the way, submitted the original patch...)

0xABAB

The attribute nativeBuildInputs is not used for pkgconfig and perhaps others, so that's an improvement you could still make.

The most important value however is that you already tested the package for a few weeks before sharing it.

Thanks for your contribution.

0xABAB · 2017-07-09T10:27:49Z

@bzizou The version issue still needs to be resolved before this can be merged.

bzizou · 2017-07-09T21:41:33Z

@0xABAB Sure, I'll amend my commit very soon. I just wanted one of my users to test the new package, as I upgraded gildas by the way...

bzizou · 2017-07-10T11:49:39Z

Fixes:

typos
moved some deps into nativeBuildinputs
version of the package is more nix-compatible
feb17d -> jul17a

Re-tested by a real user on a real hpc cluster

Mic92 · 2017-07-13T13:17:25Z

pkgs/applications/science/astronomy/gildas/wrapper.patch

@@ -0,0 +1,18 @@
+diff --new-file -r -u gildas-src-feb17d.orig/admin/wrapper.sh gildas-src-feb17d/admin/wrapper.sh
+--- gildas-src-feb17d.orig/admin/wrapper.sh	1970-01-01 01:00:00.000000000 +0100
+++ gildas-src-feb17d/admin/wrapper.sh	2017-05-18 21:00:01.660778782 +0200


Will this wrapper part in future versions of the software? Just asking because of the git-like hash in the patch path name.

Mic92 · 2017-07-13T13:19:48Z

pkgs/applications/science/astronomy/gildas/default.nix

+
+  enableParallelBuilding = false;
+
+  nativeBuildInputs = [ pkgconfig groff perl getopt gfortran python27 python27Packages.numpy ];


Is numpy a runtime dependency of this application?

Is numpy a runtime dependency of this application?

Yes, it is.

bzizou · 2017-10-19T09:20:27Z

FYI I'm currently in the process of upgrading to the latest version as the source for this PR is no more available

obadz · 2018-03-18T19:54:19Z

Ok, please reopen as a separate PR when you're ready then. Thanks.

0xABAB reviewed Jul 6, 2017

View reviewed changes

0xABAB approved these changes Jul 6, 2017

View reviewed changes

gildas: init at jul17a

97fb842

bzizou force-pushed the gildas-pr branch from 10c24df to 97fb842 Compare July 10, 2017 11:45

bzizou changed the title ~~gildas: init at feb17d~~ gildas: init at jul17a Jul 10, 2017

Mic92 reviewed Jul 13, 2017

View reviewed changes

obadz closed this Mar 18, 2018

smaret mentioned this pull request Jun 13, 2018

gildas: init at 201806_a #41929

Merged

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

gildas: init at jul17a #27138

gildas: init at jul17a #27138

bzizou commented Jul 5, 2017 •

edited

0xABAB Jul 6, 2017

bzizou Jul 6, 2017

0xABAB Jul 6, 2017

bzizou Jul 6, 2017

edolstra Jul 6, 2017

0xABAB Jul 6, 2017

0xABAB Jul 6, 2017

bzizou Jul 6, 2017

0xABAB Jul 6, 2017

bzizou Jul 6, 2017

0xABAB Jul 6, 2017

bzizou Jul 6, 2017

0xABAB Jul 6, 2017

bzizou Jul 6, 2017

0xABAB left a comment

0xABAB commented Jul 9, 2017

bzizou commented Jul 9, 2017

bzizou commented Jul 10, 2017 •

edited

Mic92 Jul 13, 2017 •

edited

Mic92 Jul 13, 2017

smaret Oct 19, 2017

bzizou commented Oct 19, 2017

obadz commented Mar 18, 2018


		enableParallelBuilding = false;

		nativeBuildInputs = [ pkgconfig groff perl getopt gfortran python27 python27Packages.numpy ];

gildas: init at jul17a #27138

gildas: init at jul17a #27138

Conversation

bzizou commented Jul 5, 2017 • edited

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

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

0xABAB left a comment

Choose a reason for hiding this comment

0xABAB commented Jul 9, 2017

bzizou commented Jul 9, 2017

bzizou commented Jul 10, 2017 • edited

Mic92 Jul 13, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bzizou commented Oct 19, 2017

obadz commented Mar 18, 2018

bzizou commented Jul 5, 2017 •

edited

bzizou commented Jul 10, 2017 •

edited

Mic92 Jul 13, 2017 •

edited