GNU bug report logs - #65924
git searches coreutils and util-linux commands in PATH

Previous Next

Full log

View this message in rfc822 format

From: Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
To: Ludovic Courtès <ludo <at> gnu.org>
Cc: 65924 <at> debbugs.gnu.org, Simon Tournier <zimon.toutoune <at> gmail.com>
Subject: bug#65924: [PATCH core-updates 3/3] gnu: git-minimal: Add coreutils and sed to PATH.
Date: Sun, 15 Oct 2023 16:03:51 -0400

Hi Ludovic,

Ludovic Courtès <ludo <at> gnu.org> writes:

> Maxim Cournoyer <maxim.cournoyer <at> gmail.com> skribis:
>
>> Fixes <https://issues.guix.gnu.org/65924>.
>>
>> * gnu/packages/version-control.scm (git-minimal)
>> [arguments] <imported-modules>: New field.
>> <modules>: Augment with (ice-9 match), (ice-9 textual-ports) and (guix
>> search-paths).
>> <phases>: Add patch-commands phase.
>> [inputs]: Add coreutils-minimal and sed.
>
> [...]
>
>> +      #:imported-modules `(,@%gnu-build-system-modules
>> +                           ,@(source-module-closure '((guix search-paths))))
>
> I think we should avoid the dependency on (guix search-paths) here, to
> avoid situation such as that described in
> <https://issues.guix.gnu.org/66525>.

OK, I can see how that could be annoying, especially since (guix
search-paths) will see frequent new search paths additions.

I think the best course to avoid a repeat may be to document that only
modules part of the (guix build ...) prefix should be used on the build
side, with the list of exception modules, if there are any; what do you
think?

>> +          (add-after 'unpack 'patch-commands
>> +            (lambda* (#:key inputs #:allow-other-keys)
>> +              (define (prepend-string-to-file text file)
>> +                "Prepend TEXT to FILE."
>
> Nitpick: no need to add a docstring to internal defines because it’s
> optimized out and inaccessible (you can use a comment instead).

I think I did so here in case it's ever moved to (guix build utils) or
the likes; it seems a useful procedure to have around.  Thanks for the
bit of knowledge!

>> +                (let ((content (call-with-input-file file
>> +                                 (cut get-string-all <>))))
>> +                  (call-with-output-file file
>> +                    (lambda (port)
>> +                      (display text port)
>> +                      (display content port)))))
>> +
>> +              (define PATH-variable-definition
>> +                (let ((value
>> +                       (match (evaluate-search-paths
>> +                               (list $PATH)
>> +                               (list #$(this-package-input "coreutils-minimal")
>> +                                     #$(this-package-input "sed")))
>> +                         (((spec . value))
>> +                          value))))
>> +                  (string-append
>> +                   (search-path-definition $PATH value
>> +                                           #:kind 'prefix) "\n\n")))
>> +
>> +              ;; Ensure that coreutils (for basename) and sed are on PATH
>> +              ;; for any script that sources the 'git-sh-setup.sh' file.
>> +              (prepend-string-to-file PATH-variable-definition
>> +                                      "git-sh-setup.sh")
>
> How about something along these lines instead:
>
>   ;; Instead PATH definition at the top of the file.
>   (substitute* "git-sh-setup.sh"
>     (("^unset CDPATH" all)
>      (string-append "PATH=" (dirname (search-input-file inputs "bin/basename"))
>                     ":$PATH\nexport PATH\n" all)))

I'd like to preserve prepending the shell expression to the beginning of
the file, as substitute* doesn't error when it doesn't match anything,
which could lead to silent breakage in the future (if that 'unset
CDPATH' line is ever moved/deleted).  The rest looks good, except we'd
have to add sed to the PATH as well.

I can send a new patch to that effect.

-- 
Thanks,
Maxim

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.