bug#58861: [PATCH] shell: Fix '--emulate-fhs' sometimes not including 'glibc-for-fhs'.

[John Kehayias via Bug reports for GNU Guix <bug-guix@gnu.org>]

[-- Attachment #1: Type: text/plain, Size: 2339 bytes --]

Hi Ludo’,

On Wed, Nov 02, 2022 at 04:50 PM, Ludovic Courtès wrote:

> Hi John,
>
> John Kehayias <john.kehayias@protonmail.com> skribis:
>
>> After commit
>> <https://git.savannah.gnu.org/cgit/guix.git/commit/?id=c07b55eb94f8cfa9d0f56cfd97a16f2f7d842652>
>> I noticed a changed in behavior of guix shell with the emulate-fhs option for a
>> container. I tracked it down to the wrong glibc package appearing in the container, i.e.
>> the standard Guix version rather than glibc-for-fhs (which reads a global ld cache).
>>
>> The cause I believe is related to <https://issues.guix.gnu.org/58859>, namely that
>> package input order for a profile can matter. But it is slightly different here since
>> the glibc-for-fhs package is added internally.
>>
>> We can see this demonstrated by comparing the FHS container with a -D input so that a
>> glibc package is implicitly included (here from the gnu-build-system):
>>
>> ❯ guix shell -CFD hello coreutils
>> john@narya ~/Files/UPenn/canvasgrading [env]$ ls /lib/ld* -la
>> lrwxrwxrwx 1 65534 overflow 69 Jan 1 1970 /lib/ld-2.33.so ->
>> /gnu/store/5h2w4qi9hk1qzzgi1w83220ydslinr4s-glibc-2.33/lib/ld-2.33.so
>> lrwxrwxrwx 1 65534 overflow 79 Jan 1 1970 /lib/ld-linux-x86-64.so.2 ->
>> /gnu/store/5h2w4qi9hk1qzzgi1w83220ydslinr4s-glibc-2.33/lib/ld-linux-x86-64.so.2
>
> How about fixing it by moving the (alist-cons 'expression …) thing right
> before the ‘options-with-caching’ call in ‘parse-args’?
>
> That way it would no longer be sensitive to the position of ‘-F’ on the
> command line.
>

Good idea, that worked! I didn't think right away of an easier way of doing this, so I added another let binding to easily check for '--emulate-fhs' in the parsed arguments.

> Could you give it a try and add a test?
>

I added a test that explicitly includes 'glibc' in the 'guix shell' invocation and checked the link to '/lib/libc.so' was from 'glibc-for-fhs'. Again, not sure if there is a better way here, but the test does pass now and fails without the change you proposed. I also checked against the examples I gave originally and looked good there too.

Patch attached. I included an explanation (and link) of this bug and the fix in the commit message.

Thanks and let me know if there is anything to improve here!

John

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-shell-Fix-emulate-fhs-sometimes-not-including-glibc-.patch --]
[-- Type: text/x-patch; name=0001-shell-Fix-emulate-fhs-sometimes-not-including-glibc-.patch, Size: 4870 bytes --]

From 72be4a15a10916ae8d51dfb2998d6179bc57be59 Mon Sep 17 00:00:00 2001
From: John Kehayias <john.kehayias@protonmail.com>
Date: Thu, 3 Nov 2022 14:25:09 -0400
Subject: [PATCH] shell: Fix '--emulate-fhs' sometimes not including
 'glibc-for-fhs'.

Fixes <https://issues.guix.gnu.org/58861>.

Previously the order of the options giving to 'guix shell' could mean that the
'glibc-for-fhs' package included with the '--emulate-fhs' option would not
appear in the container.  For example, using the development option with a
package using the 'gnu-build-system', e.g. 'guix shell -CFD hello', would
include the regular 'glibc' package.  The option ordered mattered: 'guix shell
-CD hello -F' would include the expected 'glibc-for-fhs'.  We fix this by
having 'glibc-for-fhs' added to the package list just before calling
'options-with-caching' so the option order given by the user does not matter.

* guix/scripts/shell.scm (%options): Move the '--emulate-fhs' (expression
. ...) component from here...
(parse-args): ... to here.
* tests/guix-environment-container.sh: Add a test to check that
'glibc-for-fhs' is in the container even when 'glibc' is included in the 'guix
shell' package list.
---
 guix/scripts/shell.scm              | 26 ++++++++++++++------------
 tests/guix-environment-container.sh | 10 ++++++++++
 2 files changed, 24 insertions(+), 12 deletions(-)

diff --git a/guix/scripts/shell.scm b/guix/scripts/shell.scm
index a2836629ad..f334bd57ae 100644
--- a/guix/scripts/shell.scm
+++ b/guix/scripts/shell.scm
@@ -143,16 +143,7 @@ (define %options
 
               (option '(#\F "emulate-fhs") #f #f
                       (lambda (opt name arg result)
-                        (let ((result
-                               ;; For an FHS-container, add the (hidden)
-                               ;; package glibc-for-fhs which uses the global
-                               ;; cache at /etc/ld.so.cache.
-                               (alist-cons
-                                'expression
-                                '(ad-hoc-package
-                                  "(@@ (gnu packages base) glibc-for-fhs)")
-                                result)))
-                         (alist-cons 'emulate-fhs? #t result)))))
+                        (alist-cons 'emulate-fhs? #t result))))
         (filter-map (lambda (opt)
                       (and (not (any (lambda (name)
                                        (member name to-remove))
@@ -173,8 +164,19 @@ (define (parse-args args)
   ;; The '--' token is used to separate the command to run from the rest of
   ;; the operands.
   (let ((args command (break (cut string=? "--" <>) args)))
-    (let ((opts (parse-command-line args %options (list %default-options)
-                                    #:argument-handler handle-argument)))
+    (let* ((args-parsed (parse-command-line args %options (list %default-options)
+                                            #:argument-handler handle-argument))
+           ;; For an FHS-container, add the (hidden) package glibc-for-fhs
+           ;; which uses the global cache at /etc/ld.so.cache.  We handle
+           ;; adding this package here to ensure it will always appear in the
+           ;; container as it is the first package in OPTS.
+           (opts (if (assoc-ref args-parsed 'emulate-fhs?)
+                     (alist-cons
+                      'expression
+                      '(ad-hoc-package
+                        "(@@ (gnu packages base) glibc-for-fhs)")
+                      args-parsed)
+                     args-parsed)))
       (options-with-caching
        (auto-detect-manifest
         (match command
diff --git a/tests/guix-environment-container.sh b/tests/guix-environment-container.sh
index f233c3fcc0..fb2c19b193 100644
--- a/tests/guix-environment-container.sh
+++ b/tests/guix-environment-container.sh
@@ -1,5 +1,6 @@
 # GNU Guix --- Functional package management for GNU
 # Copyright © 2015 David Thompson <davet@gnu.org>
+# Copyright © 2022 John Kehayias <john.kehayias@protonmail.com>
 #
 # This file is part of GNU Guix.
 #
@@ -231,3 +232,12 @@ guix shell -C --emulate-fhs --bootstrap guile-bootstrap \
 # Test that the ld cache was generated and can be successfully read.
 guix shell -CF --bootstrap guile-bootstrap \
      -- guile -c '(execlp "ldconfig" "ldconfig" "-p")'
+
+# Test that the package glibc-for-fhs is in the container even if there is the
+# regular glibc package from another source.  See
+# <https://issues.guix.gnu.org/58861>.
+guix shell -CF --bootstrap guile-bootstrap glibc \
+     -- guile -c '(exit (if (string-contains (readlink "/lib/libc.so")
+                            "glibc-for-fhs")
+                           0
+                           1))'
-- 
2.38.0