[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

[bug#49456] [PATCH] gnu: add environment-modules

From: Ludovic Courtès
Subject: [bug#49456] [PATCH] gnu: add environment-modules
Date: Tue, 20 Jul 2021 22:34:55 +0200
User-agent: Gnus/5.13 (Gnus v5.13) Emacs/27.2 (gnu/linux)


Nice!  Overall LGTM, modulo cosmetic issues described below.

Ivan Gankevich <> 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/"
> +                (("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/

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.  :-)



reply via email to

[Prev in Thread] Current Thread [Next in Thread]