bug#65924: [PATCH core-updates 3/3] gnu: git-minimal: Add coreutils and sed to PATH.

[Maxim Cournoyer]

Hi Ludovic,

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

> Maxim Cournoyer <maxim.cournoyer@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