GNU bug report logs - #48224
[PATCH 0/2] Avoid Bash wrapper in 'guix' package

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 48224 in the body.
You can then email your comments to 48224 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: Ludovic Courtès <ludo <at> gnu.org>
To: guix-patches <at> gnu.org
Cc: Ludovic Courtès <ludo <at> gnu.org>
Subject: [PATCH 0/2] Avoid Bash wrapper in 'guix' package
Date: Tue,  4 May 2021 15:25:42 +0200

Hello!

The first patch here addresses a locale warning coming from Bash
that I noticed while using 1.3.0rc1 on Debian.  Concretely, without
this patch, users will see the Bash locale warning every time
‘guix substitute’ starts (so at least once per session).

The second patch is stylistic: it avoids missing phases, which I
find more readable.

Tested with a native x86_64-linux build and with
‘--target=aarch64-linux-gnu’ from x86_64-linux.

I’d like to have these in ‘version-1.3.0’.

Thoughts?

Ludo’.

Ludovic Courtès (2):
  gnu: guix: Avoid Bash wrapper.
  gnu: guix: Phases refer to #:system and #:target.

 gnu/packages/package-management.scm | 74 +++++++++++++++--------------
 1 file changed, 38 insertions(+), 36 deletions(-)

-- 
2.31.1

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

From: Ludovic Courtès <ludo <at> gnu.org>
To: 48224 <at> debbugs.gnu.org
Cc: Ludovic Courtès <ludo <at> gnu.org>
Subject: [PATCH 1/2] gnu: guix: Avoid Bash wrapper.
Date: Tue,  4 May 2021 15:39:57 +0200

The Bash wrapper created by 'wrap-program' creates an extra
indirection and may annoyingly emit locale warnings:

  /gnu/store/…-bash-minimal-5.0.16/bin/bash: warning: setlocale: LC_ALL: cannot change locale (wtf)

This warning would typically show up when running Guix, as produced by
'guix pack guix', on a foreign distro, annihilating efforts made in
1d4ab335b22a93e01c2eb1eb3e93fc6534157040 and
8a973abc6f7eebfcd8a904bfbb99cb9f86f66ef0.

* gnu/packages/package-management.scm (guix)[arguments]: In
'wrap-program' phase, remove 'string-join' call for PATH and GOPATH.
Replace 'wrap-program' call with a 'substitute*' form.  Remove (when
target ...) form.
[inputs]: Remove "bash-minimal" added in commit
38b9af7c92344a17b6680ebd2aeea14171f84a1c and no longer needed.
---
 gnu/packages/package-management.scm | 56 ++++++++++++++++-------------
 1 file changed, 31 insertions(+), 25 deletions(-)

diff --git a/gnu/packages/package-management.scm b/gnu/packages/package-management.scm
index 827166c938..1a637f9ec8 100644
--- a/gnu/packages/package-management.scm
+++ b/gnu/packages/package-management.scm
@@ -326,31 +326,38 @@ $(prefix)/etc/openrc\n")))
                                  (open-pipe* OPEN_READ
                                              (string-append guile "/bin/guile")
                                              "-c" "(display (effective-version))")))
-                               (path   (string-join
-                                        (map (cut string-append <>
-                                                  "/share/guile/site/"
-                                                  effective)
-                                             (delete #f deps*))
-                                        ":"))
-                               (gopath (string-join
-                                        (map (cut string-append <>
-                                                  "/lib/guile/" effective
-                                                  "/site-ccache")
-                                             (delete #f deps*))
-                                        ":"))
+                               (path   (map (cut string-append <>
+                                                 "/share/guile/site/"
+                                                 effective)
+                                            (delete #f deps*)))
+                               (gopath (map (cut string-append <>
+                                                 "/lib/guile/" effective
+                                                 "/site-ccache")
+                                            (delete #f deps*)))
                                (locpath (string-append locales "/lib/locale")))
 
-                          (wrap-program (string-append out "/bin/guix")
-                            `("GUILE_LOAD_PATH" ":" prefix (,path))
-                            `("GUILE_LOAD_COMPILED_PATH" ":" prefix (,gopath))
-                            `("GUIX_LOCPATH" ":" suffix (,locpath)))
-
-                          (when target
-                            ;; XXX Touching wrap-program rebuilds world
-                            (let ((bash (assoc-ref inputs "bash")))
-                              (substitute* (string-append out "/bin/guix")
-                                (("^#!.*/bash") (string-append "#! " bash "/bin/bash")))))
-                          #t)))
+                          ;; Modify 'guix' directly instead of using
+                          ;; 'wrap-command'.  This avoids the indirection
+                          ;; through Bash, which in turn avoids getting Bash's
+                          ;; own locale warnings.
+                          (substitute* (string-append out "/bin/guix")
+                            (("!#")
+                             (string-append
+                              "!#\n\n"
+                              (object->string
+                               `(set! %load-path (append ',path %load-path)))
+                              "\n"
+                              (object->string
+                               `(set! %load-compiled-path
+                                  (append ',gopath %load-compiled-path)))
+                              "\n"
+                              (object->string
+                               `(let ((path (getenv "GUIX_LOCPATH")))
+                                  (setenv "GUIX_LOCPATH"
+                                          (if path
+                                              (string-append path ":" ,locpath)
+                                              ,locpath))))
+                              "\n\n"))))))
 
                     ;; The 'guix' executable has 'OUT/libexec/guix/guile' as
                     ;; its shebang; that should remain unchanged, thus remove
@@ -405,8 +412,7 @@ $(prefix)/etc/openrc\n")))
                `(("boot-guile/i686" ,(bootstrap-guile-origin "i686-linux")))
                '())
          ,@(if (%current-target-system)
-               `(("bash" ,bash-minimal)
-                 ("xz" ,xz))
+               `(("xz" ,xz))
                '())
 
          ;; Tests also rely on these bootstrap executables.
-- 
2.31.1

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

From: Ludovic Courtès <ludo <at> gnu.org>
To: 48224 <at> debbugs.gnu.org
Cc: Ludovic Courtès <ludo <at> gnu.org>
Subject: [PATCH 2/2] gnu: guix: Phases refer to #:system and #:target.
Date: Tue,  4 May 2021 15:39:58 +0200

* gnu/packages/package-management.scm (guix)[arguments]: In
'copy-bootstrap-guile' and 'wrap-program' phases, refer to #:system
and #:target instead of unquoting (%current-system)
and (%current-target-system).
---
 gnu/packages/package-management.scm | 18 +++++++-----------
 1 file changed, 7 insertions(+), 11 deletions(-)

diff --git a/gnu/packages/package-management.scm b/gnu/packages/package-management.scm
index 1a637f9ec8..5c6937adb8 100644
--- a/gnu/packages/package-management.scm
+++ b/gnu/packages/package-management.scm
@@ -261,11 +261,9 @@ $(prefix)/etc/openrc\n")))
                           (intern (assoc-ref inputs "boot-guile") #f)
 
                           ;; On x86_64 some tests need the i686 Guile.
-                          ,@(if (and (not (%current-target-system))
-                                     (string=? (%current-system)
-                                               "x86_64-linux"))
-                                '((intern (assoc-ref inputs "boot-guile/i686") #f))
-                                '())
+                          (when (and (not target)
+                                     (string=? system "x86_64-linux"))
+                            (intern (assoc-ref inputs "boot-guile/i686") #f))
 
                           ;; Copy the bootstrap executables.
                           (for-each (lambda (input)
@@ -299,9 +297,9 @@ $(prefix)/etc/openrc\n")))
                         ;; Make sure the 'guix' command finds GnuTLS,
                         ;; Guile-JSON, and Guile-Git automatically.
                         (let* ((out    (assoc-ref outputs "out"))
-                               (guile  ,@(if (%current-target-system)
-                                             '((assoc-ref native-inputs "guile"))
-                                             '((assoc-ref inputs "guile"))))
+                               (guile  (if target
+                                           (assoc-ref native-inputs "guile")
+                                           (assoc-ref inputs "guile")))
                                (avahi  (assoc-ref inputs "guile-avahi"))
                                (gcrypt (assoc-ref inputs "guile-gcrypt"))
                                (guile-lib   (assoc-ref inputs "guile-lib"))
@@ -318,9 +316,7 @@ $(prefix)/etc/openrc\n")))
                                (locales (assoc-ref inputs "glibc-utf8-locales"))
                                (deps   (list gcrypt json sqlite gnutls git
                                              bs ssh zlib lzlib zstd guile-lib))
-                               (deps*  ,@(if (%current-target-system)
-                                             '(deps)
-                                             '((cons avahi deps))))
+                               (deps*  (if target deps (cons avahi deps)))
                                (effective
                                 (read-line
                                  (open-pipe* OPEN_READ
-- 
2.31.1

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

From: Maxime Devos <maximedevos <at> telenet.be>
To: Ludovic Courtès <ludo <at> gnu.org>, 48224 <at> debbugs.gnu.org
Subject: Re: [bug#48224] [PATCH 2/2] gnu: guix: Phases refer to #:system and
 #:target.
Date: Tue, 04 May 2021 21:21:24 +0200

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

> The second patch is stylistic: it avoids missing phases, which I
> find more readable.

I don't see any missing phases. (I read this as ‘The second patch is
stylistic: it avoids making the existence of a phase dependent on
%current-target-system.’)

> -                               (guile  ,@(if (%current-target-system)
> -                                             '((assoc-ref native-inputs "guile"))
> -                                             '((assoc-ref inputs "guile"))))
> +                               (guile  (if target
> +                                           (assoc-ref native-inputs "guile")
> +                                           (assoc-ref inputs "guile")))

Something I tend to do is
  (assoc-ref (or native-inputs inputs) "guile")

Do you have any particular preference?

>                                 (deps   (list gcrypt json sqlite gnutls git
>                                               bs ssh zlib lzlib zstd guile-lib))
> -                               (deps*  ,@(if (%current-target-system)
> -                                             '(deps)
> -                                             '((cons avahi deps))))
> +                               (deps*  (if target deps (cons avahi deps)))

Why not simply
  ;; avahi is #f (not in 'inputs') when cross-compiling.
  ;; Remove it.
  (deps* (delete #f avahi))
?  Then, when guile-avahi becomes cross-compilable at some point, we only
need to adjust 'propagated-inputs' and not anything else.

Also, was this code (deps* ,@(if (%current-target-system) '(deps) ...)) needed in
the first place?  guile2.2-guix inherits its phases from guix, and guile2.2-guix does
not have a guile-zlib or guile-lzlib input.

Greetings,
Maxime.

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

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

From: Ludovic Courtès <ludo <at> gnu.org>
To: Maxime Devos <maximedevos <at> telenet.be>
Cc: 48224 <at> debbugs.gnu.org, Vagrant Cascadian <vagrant <at> reproducible-builds.org>
Subject: Re: bug#48224: [PATCH 0/2] Avoid Bash wrapper in 'guix' package
Date: Wed, 05 May 2021 01:05:03 +0200

Hi,

Pushed to ‘master’ with followup commits to address the ‘guile2.2-guix’
issues you mentioned:

  dd3e4fe6e7 gnu: guile2.2-guix: Add missing dependencies.
  2238bd8ddc gnu: guile-lzlib: Add Guile 2.2 variant.
  2a9af22540 gnu: guile-zlib: Add Guile 2.2 variant.
  c3f20a6678 gnu: guix: Phases refer to #:system, #:target, and #:native-inputs.
  e42bfd236e gnu: guix: Avoid Bash wrapper.
  55f7cd701c gnu: guix: Add run-time dependency on Guile-Lib.

I’ll cherry-pick to ‘version-1.3.0’.

‘guile2.2-guix’ fails tests/swh.scm though, because (guix swh) relies on
#:verify-certificate?, which Guile 2.2 didn’t have.  I wonder how
Vagrant addressed that for Debian?

Thanks,
Ludo’.

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

From: Vagrant Cascadian <vagrant <at> reproducible-builds.org>
To: Ludovic Courtès <ludo <at> gnu.org>, Maxime Devos
 <maximedevos <at> telenet.be>
Cc: 48224 <at> debbugs.gnu.org
Subject: Re: bug#48224: [PATCH 0/2] Avoid Bash wrapper in 'guix' package
Date: Tue, 04 May 2021 16:26:13 -0700

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

On 2021-05-05, Ludovic Courtès wrote:
> Pushed to ‘master’ with followup commits to address the ‘guile2.2-guix’
> issues you mentioned:
>
>   dd3e4fe6e7 gnu: guile2.2-guix: Add missing dependencies.
>   2238bd8ddc gnu: guile-lzlib: Add Guile 2.2 variant.
>   2a9af22540 gnu: guile-zlib: Add Guile 2.2 variant.
>   c3f20a6678 gnu: guix: Phases refer to #:system, #:target, and #:native-inputs.
>   e42bfd236e gnu: guix: Avoid Bash wrapper.
>   55f7cd701c gnu: guix: Add run-time dependency on Guile-Lib.
>
> I’ll cherry-pick to ‘version-1.3.0’.
>
> ‘guile2.2-guix’ fails tests/swh.scm though, because (guix swh) relies on
> #:verify-certificate?, which Guile 2.2 didn’t have.  I wonder how
> Vagrant addressed that for Debian?

Heavy handedly sprinkling (test-skip 1):

  https://salsa.debian.org/debian/guix/-/blob/debian/devel/debian/patches/0029-tests-swh.scm-Disable-tests-the-fail-with-guile-2.2.patch
  https://salsa.debian.org/debian/guix/-/blob/debian/devel/debian/patches/0032-Disable-origin-visit-no-snapshots-test.patch

It would be nice to be able to automatically skip tests using known
guile 3 features when building with guile 2.2; maybe that will require
flagging each test individually or maybe there is a clever way to do
that dynamically?


live well,
  vagrant

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

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

From: Ludovic Courtès <ludo <at> gnu.org>
To: Vagrant Cascadian <vagrant <at> reproducible-builds.org>
Cc: 48224 <at> debbugs.gnu.org, Maxime Devos <maximedevos <at> telenet.be>
Subject: Re: bug#48224: [PATCH 0/2] Avoid Bash wrapper in 'guix' package
Date: Sun, 09 May 2021 23:56:11 +0200

Hi,

Vagrant Cascadian <vagrant <at> reproducible-builds.org> skribis:

>> ‘guile2.2-guix’ fails tests/swh.scm though, because (guix swh) relies on
>> #:verify-certificate?, which Guile 2.2 didn’t have.  I wonder how
>> Vagrant addressed that for Debian?
>
> Heavy handedly sprinkling (test-skip 1):
>
>   https://salsa.debian.org/debian/guix/-/blob/debian/devel/debian/patches/0029-tests-swh.scm-Disable-tests-the-fail-with-guile-2.2.patch
>   https://salsa.debian.org/debian/guix/-/blob/debian/devel/debian/patches/0032-Disable-origin-visit-no-snapshots-test.patch

This patch addresses the problem:

  https://git.savannah.gnu.org/cgit/guix.git/commit/?h=version-1.3.0&id=626e61959089b9c7d1faa5dd6b15f8ed94f551fb

(It’s in 1.3.0rc2.)

HTH!

Ludo’.

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

From: Vagrant Cascadian <vagrant <at> reproducible-builds.org>
To: Ludovic Courtès <ludo <at> gnu.org>
Cc: 48224 <at> debbugs.gnu.org, Maxime Devos <maximedevos <at> telenet.be>
Subject: Re: bug#48224: [PATCH 0/2] Avoid Bash wrapper in 'guix' package
Date: Wed, 12 May 2021 20:45:47 -0700

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

On 2021-05-09, Ludovic Courtès wrote:
> Vagrant Cascadian <vagrant <at> reproducible-builds.org> skribis:
>>> ‘guile2.2-guix’ fails tests/swh.scm though, because (guix swh) relies on
>>> #:verify-certificate?, which Guile 2.2 didn’t have.  I wonder how
>>> Vagrant addressed that for Debian?
>>
>> Heavy handedly sprinkling (test-skip 1):
>>
>>   https://salsa.debian.org/debian/guix/-/blob/debian/devel/debian/patches/0029-tests-swh.scm-Disable-tests-the-fail-with-guile-2.2.patch
>>   https://salsa.debian.org/debian/guix/-/blob/debian/devel/debian/patches/0032-Disable-origin-visit-no-snapshots-test.patch
>
> This patch addresses the problem:
>
>   https://git.savannah.gnu.org/cgit/guix.git/commit/?h=version-1.3.0&id=626e61959089b9c7d1faa5dd6b15f8ed94f551fb
>
> (It’s in 1.3.0rc2.)

Just tested 1.3.0 with the tests enabled again, and they passed! Thanks!


live well,
  vagrant

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

Previous Next

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