| Message ID | 20210707085932.20751-1-i.gankevich@spbu.ru |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [bug#49456] gnu: add environment-modules | expand |
| Context | Check | Description |
|---|---|---|
| cbaines/comparison | success | View comparision |
| cbaines/git branch | success | View Git branch |
| cbaines/applying patch | success | View Laminar job |
| cbaines/issue | success | View issue |
Hello, Nice! Overall LGTM, modulo cosmetic issues described below. Ivan Gankevich <i.gankevich@spbu.ru> skribis: > +++ b/gnu/packages/parallel.scm How about ‘package-management.scm’ instead? You probably need to add a copyright line for you too. > +(define-public environment-modules > + (package > + (name "environment-modules") Should the package name be “modules”, since that’s the name that upstream seems to be using? > + (add-before 'configure 'patch-scripts-for-python-3 > + (lambda _ > + ;; patch the script for python-3 Nitpick: please capitalize sentences and end with a period. > + (substitute* "script/createmodule.py.in" > + (("pathkeys.sort\\(\\)") "pathkeys = sorted(pathkeys)") > + (("print\\(\"\\\\t\"\\*") "print(\"\\t\"*int") > + (("@PYTHON@") (which "python3"))) > + #t)) You can omit the trailing #t (here and elsewhere). > + (add-after 'configure 'patch-/bin/sh-in-tests > + (lambda _ > + (for-each > + (lambda (file) > + (substitute* file > + (("/bin/sh") (which "bash")) > + ;; For some reason "kvm" group cannot be resolved for > + ;; "nixbld" user. We remove "-n" switch here to not > + ;; resolve the groups at all. > + (("exec id -G -n -z") "exec id -G -z") > + (("exec id -G -n") "exec id -G") Is this change made for tests? In the build environment, the build user is potentially in the “kvm” group if it exists, but indeed, /etc/group lacks “kvm” (see nix/libstore/build.cc:1777). Should a post-check phase reinstate ‘-n’? > + )) Consider moving the parents on the previous line, as ‘guix lint’ suggests. :-) > + (synopsis "Shell environment variables and aliases management") > + (description "A tool that simplify shell initialization and lets users > +easily modify their environment during the session with modulefiles.") Please write full sentences for the description. Could you send an updated patch? Bonus points if you can provide a commit log that follows our conventions. :-) https://guix.gnu.org/manual/en/html_node/Submitting-Patches.html Thanks! Ludo’.
>> +++ b/gnu/packages/parallel.scm > >How about ‘package-management.scm’ instead? > >You probably need to add a copyright line for you too. Moved to ‘package-management.scm’, added copyright line. >> +(define-public environment-modules >> + (package >> + (name "environment-modules") > >Should the package name be “modules”, since that’s the name that >upstream seems to be using? Renamed to “modules”. >> + (add-after 'configure 'patch-/bin/sh-in-tests >> + (lambda _ >> + (for-each >> + (lambda (file) >> + (substitute* file >> + (("/bin/sh") (which "bash")) >> + ;; For some reason "kvm" group cannot be resolved for >> + ;; "nixbld" user. We remove "-n" switch here to not >> + ;; resolve the groups at all. >> + (("exec id -G -n -z") "exec id -G -z") >> + (("exec id -G -n") "exec id -G") > >Is this change made for tests? In the build environment, the build user >is potentially in the “kvm” group if it exists, but indeed, /etc/group >lacks “kvm” (see nix/libstore/build.cc:1777). > >Should a post-check phase reinstate ‘-n’? This change is needed for tests only, main programme uses different configuration. I have updated to the version 4.8.0 and unfortunately these changes no longer work (developers replaced calls to “id” with Tcl extensions). Now I disabled tests that use group information. Can we add all supplementary groups to /etc/groups? Not adding them to /etc/group makes some shell commands return an error (“groups”, “id -G -n”). > >> + (synopsis "Shell environment variables and aliases management") >> + (description "A tool that simplify shell initialization and lets users >> +easily modify their environment during the session with modulefiles.") > >Please write full sentences for the description. Changed description. >Could you send an updated patch? > >Bonus points if you can provide a commit log that follows our >conventions. :-) I’ve sent an updated patch in a separate email. Thank you for the corrections!
diff --git a/gnu/packages/parallel.scm b/gnu/packages/parallel.scm index 42826f49d6..f07ce02d33 100644 --- a/gnu/packages/parallel.scm +++ b/gnu/packages/parallel.scm @@ -41,8 +41,10 @@ #:use-module (gnu packages admin) #:use-module (gnu packages autotools) #:use-module (gnu packages base) + #:use-module (gnu packages dejagnu) #:use-module (gnu packages flex) #:use-module (gnu packages freeipmi) + #:use-module (gnu packages less) #:use-module (gnu packages linux) #:use-module (gnu packages mpi) #:use-module (gnu packages perl) @@ -378,3 +380,65 @@ and output captured in the notebook. Whatever arguments are accepted by a SLURM command line executable are also accepted by the corresponding magic command---e.g., @code{%salloc}, @code{%sbatch}, etc.") (license license:bsd-3)))) + +(define-public environment-modules + (package + (name "environment-modules") + (version "4.7.1") + (source + (origin + (method url-fetch) + (uri (string-append "mirror://sourceforge/modules/Modules/modules-" + version "/modules-" version ".tar.bz2")) + (sha256 (base32 "07r03vqskjxyjy5m2b6p2px42djnd2z1k4b5j9dxqv8prin01ax6")))) + (build-system gnu-build-system) + (arguments + `(#:configure-flags + (list (string-append "--with-bin-search-path=" + (assoc-ref %build-inputs "tcl") "/bin" ":" + (assoc-ref %build-inputs "procps") "/bin" ":" + (assoc-ref %build-inputs "less") "/bin" ":" + (assoc-ref %build-inputs "coreutils") "/bin") + (string-append "--with-tcl=" (assoc-ref %build-inputs "tcl") "/lib") + "--disable-compat-version") + #:test-target "test" + #:phases + (modify-phases %standard-phases + (add-before 'configure 'patch-scripts-for-python-3 + (lambda _ + ;; patch the script for python-3 + (substitute* "script/createmodule.py.in" + (("pathkeys.sort\\(\\)") "pathkeys = sorted(pathkeys)") + (("print\\(\"\\\\t\"\\*") "print(\"\\t\"*int") + (("@PYTHON@") (which "python3"))) + #t)) + (add-after 'configure 'patch-/bin/sh-in-tests + (lambda _ + (for-each + (lambda (file) + (substitute* file + (("/bin/sh") (which "bash")) + ;; For some reason "kvm" group cannot be resolved for + ;; "nixbld" user. We remove "-n" switch here to not + ;; resolve the groups at all. + (("exec id -G -n -z") "exec id -G -z") + (("exec id -G -n") "exec id -G") + )) + '("testsuite/modules.00-init/005-init_ts.exp" + "testsuite/install.00-init/005-init_ts.exp")) + #t))))) + (native-inputs + `(("dejagnu" ,dejagnu) + ("autoconf" ,autoconf) + ("which" ,which))) + (inputs + `(("tcl" ,tcl) + ("less" ,less) + ("procps" ,procps) + ("coreutils" ,coreutils) + ("python" ,python-3))) + (home-page "http://modules.sourceforge.net/") + (synopsis "Shell environment variables and aliases management") + (description "A tool that simplify shell initialization and lets users +easily modify their environment during the session with modulefiles.") + (license license:gpl2+)))