GNU bug report logs - #31952
[PATCH] gnu: git: Update to 2.18.0.

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 31952 in the body.
You can then email your comments to 31952 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Marius Bakke <mbakke <at> fastmail.com>
To: guix-patches <at> gnu.org
Cc: Marius Bakke <mbakke <at> fastmail.com>
Subject: [PATCH] gnu: git: Update to 2.18.0.
Date: Sat, 23 Jun 2018 21:45:07 +0200

* gnu/packages/version-control.scm (git): Update to 2.18.0.
[native-inputs]: Add BASH.
[arguments]: In #:make-flags, define SHELL_PATH and TEST_SHELL_PATH.
Add #:disallowed-references.  Drop /bin/sh substitution from Makefile.  Add
phase to prevent BASH from ending up in PATH.  Delete 'patch-shebangs' phase.
---
 gnu/packages/version-control.scm | 35 ++++++++++++++++++++++++++++----
 1 file changed, 31 insertions(+), 4 deletions(-)

diff --git a/gnu/packages/version-control.scm b/gnu/packages/version-control.scm
index 3980db756..fd6b62da4 100644
--- a/gnu/packages/version-control.scm
+++ b/gnu/packages/version-control.scm
@@ -53,6 +53,7 @@
   #:use-module (gnu packages autotools)
   #:use-module (gnu packages documentation)
   #:use-module (gnu packages base)
+  #:use-module (gnu packages bash)
   #:use-module (gnu packages bison)
   #:use-module (gnu packages boost)
   #:use-module (gnu packages check)
@@ -144,17 +145,18 @@ as well as the classic centralized workflow.")
    (name "git")
    ;; XXX When updating Git, check if the special 'git:src' input to cgit needs
    ;; to be updated as well.
-   (version "2.17.1")
+   (version "2.18.0")
    (source (origin
             (method url-fetch)
             (uri (string-append "mirror://kernel.org/software/scm/git/git-"
                                 version ".tar.xz"))
             (sha256
              (base32
-              "0pm6bdnrrm165k3krnazxcxadifk2gqi30awlbcf9fism1x6w4vr"))))
+              "14hfwfkrci829a9316hnvkglnqqw1p03cw9k56p4fcb078wbwh4b"))))
    (build-system gnu-build-system)
    (native-inputs
     `(("native-perl" ,perl)
+      ("bash-for-tests" ,bash)       ;needed for completion test
       ("gettext" ,gettext-minimal)
       ("git-manpages"
        ,(origin
@@ -164,7 +166,7 @@ as well as the classic centralized workflow.")
                 version ".tar.xz"))
           (sha256
            (base32
-            "0m7grrwsqaihdgcgaicxiy4rlqjpa75n5wl6hi2qhi33xa34gmc3"))))))
+            "15k04s9pcc5wkmlfa8x99nbgczjbx0c91767ciqmjy9kwsavxqws"))))))
    (inputs
     `(("curl" ,curl)
       ("expat" ,expat)
@@ -196,10 +198,23 @@ as well as the classic centralized workflow.")
    (arguments
     `(#:make-flags `("V=1"                        ;more verbose compilation
 
+                     ,(string-append "SHELL_PATH="
+                                     (assoc-ref %build-inputs "bash")
+                                     "/bin/sh")
+
+                     ;; Tests require a bash with completion support.
+                     ,(string-append "TEST_SHELL_PATH="
+                                     (assoc-ref %build-inputs "bash-for-tests")
+                                     "/bin/bash")
+
                      ;; By default 'make install' creates hard links for
                      ;; things in 'libexec/git-core', which leads to huge
                      ;; nars; see <https://bugs.gnu.org/21949>.
                      "NO_INSTALL_HARDLINKS=indeed")
+
+      ;; Make sure the full bash does not end up in the final closure.
+      #:disallowed-references (,bash)
+
       #:test-target "test"
 
       ;; Tests fail randomly when parallel: <https://bugs.gnu.org/29512>.
@@ -212,13 +227,23 @@ as well as the classic centralized workflow.")
                                              "/bin/wish8.6")) ; XXX
 
       #:modules ((srfi srfi-1)
+                 (srfi srfi-26)
                  ,@%gnu-build-system-modules)
       #:phases
       (modify-phases %standard-phases
+        (add-after 'unpack 'modify-PATH
+          (lambda* (#:key inputs #:allow-other-keys)
+            (let ((path (string-split (getenv "PATH") #\:))
+                  (bash-full (assoc-ref inputs "bash-for-tests")))
+              ;; Drop the test bash from PATH so that (which "sh") and
+              ;; similar does the right thing.
+              (setenv "PATH" (string-join
+                              (remove (cut string-prefix? bash-full <>) path)
+                              ":"))
+              #t)))
         (add-after 'configure 'patch-makefiles
           (lambda _
             (substitute* "Makefile"
-              (("/bin/sh") (which "sh"))
               (("/usr/bin/perl") (which "perl"))
               (("/usr/bin/python") (which "python")))
             #t))
@@ -266,6 +291,8 @@ as well as the classic centralized workflow.")
                           "t/t9167-git-svn-cmd-branch-subproject.sh"
                           "t/t9141-git-svn-multiple-branches.sh"))
               #t)))
+        ;; FIXME: This phase picks up the wrong bash when patching shebangs.
+        (delete 'patch-shebangs)
         (add-after 'install 'install-shell-completion
           (lambda* (#:key outputs #:allow-other-keys)
             (let* ((out         (assoc-ref outputs "out"))
-- 
2.18.0

Message #8 received at 31952 <at> debbugs.gnu.org (full text, mbox):

From: ludo <at> gnu.org (Ludovic Courtès)
To: Marius Bakke <mbakke <at> fastmail.com>
Cc: 31952 <at> debbugs.gnu.org
Subject: Re: [bug#31952] [PATCH] gnu: git: Update to 2.18.0.
Date: Sat, 23 Jun 2018 23:59:20 +0200

Hi Marius!

Marius Bakke <mbakke <at> fastmail.com> skribis:

> * gnu/packages/version-control.scm (git): Update to 2.18.0.
> [native-inputs]: Add BASH.
> [arguments]: In #:make-flags, define SHELL_PATH and TEST_SHELL_PATH.
> Add #:disallowed-references.  Drop /bin/sh substitution from Makefile.  Add
> phase to prevent BASH from ending up in PATH.  Delete 'patch-shebangs' phase.

[...]

> +        ;; FIXME: This phase picks up the wrong bash when patching shebangs.
> +        (delete 'patch-shebangs)

Do the installed scripts still have the right shebang in spite of this?

Removing this phase altogether sounds a bit risky.  Another option would
have been to replace it with one that moves the “right” Bash to the
front of PATH and then calls the original ‘patch-shebangs’ phase.

WDYT?

Apart from that it LGTM.

Thank you!

Ludo’.

Message #11 received at 31952 <at> debbugs.gnu.org (full text, mbox):

From: Marius Bakke <mbakke <at> fastmail.com>
To: Ludovic Courtès <ludo <at> gnu.org>
Cc: 31952 <at> debbugs.gnu.org
Subject: Re: [bug#31952] [PATCH] gnu: git: Update to 2.18.0.
Date: Sun, 24 Jun 2018 01:40:16 +0200

[Message part 1 (text/plain, inline)]

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

> Hi Marius!
>
> Marius Bakke <mbakke <at> fastmail.com> skribis:
>
>> * gnu/packages/version-control.scm (git): Update to 2.18.0.
>> [native-inputs]: Add BASH.
>> [arguments]: In #:make-flags, define SHELL_PATH and TEST_SHELL_PATH.
>> Add #:disallowed-references.  Drop /bin/sh substitution from Makefile.  Add
>> phase to prevent BASH from ending up in PATH.  Delete 'patch-shebangs' phase.
>
> [...]
>
>> +        ;; FIXME: This phase picks up the wrong bash when patching shebangs.
>> +        (delete 'patch-shebangs)
>
> Do the installed scripts still have the right shebang in spite of this?

I haven't yet compared the results with and without this phase, but
"normal" usage (including "send-email") works at least.

> Removing this phase altogether sounds a bit risky.  Another option would
> have been to replace it with one that moves the “right” Bash to the
> front of PATH and then calls the original ‘patch-shebangs’ phase.

The problem is that 'patch-shebangs' does not use PATH, but instead
iterates over inputs directly.  It's supposed to prefer 'inputs' to
'native-inputs' (according to a comment), yet in this case it picks the
native "full" bash rather than bash-minimal.

If you read closely, you'll notice that 'bash-for-tests' is not in PATH
at all.  'patch-source-shebangs' and other things that use (which "sh")
works okay due to that.

I suppose we can replace the phase with a fixed version, but I'm not
sure why (@@ (guix build gnu-build-system) patch-shebangs) does the
wrong thing.

Another alternative is to delete the one test (t/t9902-completion.sh)
that requires the full bash until we have a proper fix.

Thoughts?

[signature.asc (application/pgp-signature, inline)]

Message #14 received at 31952 <at> debbugs.gnu.org (full text, mbox):

From: ludo <at> gnu.org (Ludovic Courtès)
To: Marius Bakke <mbakke <at> fastmail.com>
Cc: 31952 <at> debbugs.gnu.org
Subject: Re: [bug#31952] [PATCH] gnu: git: Update to 2.18.0.
Date: Sun, 24 Jun 2018 22:01:20 +0200

Hello,

Marius Bakke <mbakke <at> fastmail.com> skribis:

> ludo <at> gnu.org (Ludovic Courtès) writes:
>
>> Hi Marius!
>>
>> Marius Bakke <mbakke <at> fastmail.com> skribis:
>>
>>> * gnu/packages/version-control.scm (git): Update to 2.18.0.
>>> [native-inputs]: Add BASH.
>>> [arguments]: In #:make-flags, define SHELL_PATH and TEST_SHELL_PATH.
>>> Add #:disallowed-references.  Drop /bin/sh substitution from Makefile.  Add
>>> phase to prevent BASH from ending up in PATH.  Delete 'patch-shebangs' phase.
>>
>> [...]
>>
>>> +        ;; FIXME: This phase picks up the wrong bash when patching shebangs.
>>> +        (delete 'patch-shebangs)
>>
>> Do the installed scripts still have the right shebang in spite of this?
>
> I haven't yet compared the results with and without this phase, but
> "normal" usage (including "send-email") works at least.

OK.  I suppose you can simply grep for “^#!” and make sure there’s
nothing suspicious like /usr/bin/something.

>> Removing this phase altogether sounds a bit risky.  Another option would
>> have been to replace it with one that moves the “right” Bash to the
>> front of PATH and then calls the original ‘patch-shebangs’ phase.
>
> The problem is that 'patch-shebangs' does not use PATH, but instead
> iterates over inputs directly.  It's supposed to prefer 'inputs' to
> 'native-inputs' (according to a comment), yet in this case it picks the
> native "full" bash rather than bash-minimal.

OK.

> If you read closely, you'll notice that 'bash-for-tests' is not in PATH
> at all.  'patch-source-shebangs' and other things that use (which "sh")
> works okay due to that.

It may be that moving “bash-for-tests” to ‘inputs’ actually solves the
problem (and we don’t have to worry about cross-compilation since “make
check” does nothing when cross-compiling) and we can keep the
‘patch-shebangs’ phase.

But yeah, if this patch works, we can go for it.

Thank you!

Ludo’.

Message #19 received at 31952-done <at> debbugs.gnu.org (full text, mbox):

From: Marius Bakke <mbakke <at> fastmail.com>
To: Ludovic Courtès <ludo <at> gnu.org>
Cc: 31952-done <at> debbugs.gnu.org
Subject: Re: [bug#31952] [PATCH] gnu: git: Update to 2.18.0.
Date: Sun, 24 Jun 2018 22:38:17 +0200

[Message part 1 (text/plain, inline)]

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

>>> Removing this phase altogether sounds a bit risky.  Another option would
>>> have been to replace it with one that moves the “right” Bash to the
>>> front of PATH and then calls the original ‘patch-shebangs’ phase.
>>
>> The problem is that 'patch-shebangs' does not use PATH, but instead
>> iterates over inputs directly.  It's supposed to prefer 'inputs' to
>> 'native-inputs' (according to a comment), yet in this case it picks the
>> native "full" bash rather than bash-minimal.
>
> OK.
>
>> If you read closely, you'll notice that 'bash-for-tests' is not in PATH
>> at all.  'patch-source-shebangs' and other things that use (which "sh")
>> works okay due to that.
>
> It may be that moving “bash-for-tests” to ‘inputs’ actually solves the
> problem (and we don’t have to worry about cross-compilation since “make
> check” does nothing when cross-compiling) and we can keep the
> ‘patch-shebangs’ phase.

Moving it to 'inputs' worked, though I'm not sure why!

I pushed the patch with that change, thanks for your feedback :-)

[signature.asc (application/pgp-signature, inline)]

Message #22 received at 31952-done <at> debbugs.gnu.org (full text, mbox):

From: ludo <at> gnu.org (Ludovic Courtès)
To: Marius Bakke <mbakke <at> fastmail.com>
Cc: 31952-done <at> debbugs.gnu.org
Subject: Re: [bug#31952] [PATCH] gnu: git: Update to 2.18.0.
Date: Mon, 25 Jun 2018 10:54:50 +0200

Marius Bakke <mbakke <at> fastmail.com> skribis:

> ludo <at> gnu.org (Ludovic Courtès) writes:
>
>>>> Removing this phase altogether sounds a bit risky.  Another option would
>>>> have been to replace it with one that moves the “right” Bash to the
>>>> front of PATH and then calls the original ‘patch-shebangs’ phase.
>>>
>>> The problem is that 'patch-shebangs' does not use PATH, but instead
>>> iterates over inputs directly.  It's supposed to prefer 'inputs' to
>>> 'native-inputs' (according to a comment), yet in this case it picks the
>>> native "full" bash rather than bash-minimal.
>>
>> OK.
>>
>>> If you read closely, you'll notice that 'bash-for-tests' is not in PATH
>>> at all.  'patch-source-shebangs' and other things that use (which "sh")
>>> works okay due to that.
>>
>> It may be that moving “bash-for-tests” to ‘inputs’ actually solves the
>> problem (and we don’t have to worry about cross-compilation since “make
>> check” does nothing when cross-compiling) and we can keep the
>> ‘patch-shebangs’ phase.
>
> Moving it to 'inputs' worked, though I'm not sure why!

It’s because of the order in which ‘native-inputs’ and ‘inputs’ are
concatenated somewhere (presumably in ‘bag->derivation’.)

Thanks,
Ludo’.

Previous Next

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