[bug#39414] [PATCH core-updates 0/2] Clarify search path handling in commencement.scm

[bug#39414] [PATCH core-updates 0/2] Clarify search path handling in commencement.scm

[Ludovic Courtès]

Hello Guix!

The patch below for current ‘core-updates’ is an attempt to clarify
search path handling in commencement.scm by:

  1. Having ‘native-search-paths’ fields only for compilers—e.g.,
     “C_INCLUDE_PATH” belongs to the compiler, not to libc.

  2. Avoiding phases that manually fiddle with search path
     environment variables: normally, this is handled automatically
     by the ‘set-paths’ phase based on the declared search paths,
     so manual fiddling should be a last resort and it should be
     well commented so we remember why it’s there.

This is an attempt to reduce complexity and keep things declarative
as much as possible.  I’ve tested it on top of
46312064de8ae0cca531fddbc4b5ec8421e5d866 and “guix build coreutils”
succeeds.

There’s another pattern that I found hard to follow that has to do
with the reuse of build phases.

For example, there’s a build phase named ‘setenv’ (perhaps we should
find a more descriptive name :-)) in the various GCCs that is reused
or replaced; when looking at a specific package, it’s difficult to
see which phases it really runs because this particular phase is
inherited and modified on several layers.  If I can make time for it,
I’ll see if I can come up with a proposal to clarify this, but at any
rate, it’s probably something to keep in mind for future changes.

Thoughts?  (I’m particularly interested in your feedback, janneke!)

Ludo’.

Ludovic Courtès (2):
  gnu: commencement: Avoid hard-coded GCC version numbers.
  gnu: commencement: Rationalize search path handling.

 gnu/packages/commencement.scm | 201 ++++++++--------------------------
 1 file changed, 48 insertions(+), 153 deletions(-)

-- 
2.25.0

[bug#39414] [PATCH 1/2] gnu: commencement: Avoid hard-coded GCC version numbers.

[Ludovic Courtès]

* gnu/packages/commencement.scm (gcc-mesboot1, gcc-mesboot): Use
'package-version' instead of hard-coding the version number.
---
 gnu/packages/commencement.scm | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/gnu/packages/commencement.scm b/gnu/packages/commencement.scm
index 851bb02163..f011891725 100644
--- a/gnu/packages/commencement.scm
+++ b/gnu/packages/commencement.scm
@@ -938,7 +938,7 @@ ac_cv_c_float_format='IEEE (little-endian)'
   (package
     (inherit gcc-mesboot0)
     (name "gcc-mesboot1")
-    (version "4.7.4")
+    (version (package-version gcc-4.7))
     (source (bootstrap-origin
              (origin (inherit (package-source gcc-4.7))
                      (patches (search-patches "gcc-boot-4.7.4.patch")))))
@@ -1265,7 +1265,7 @@ exec " gcc "/bin/" program
   (package
     (inherit gcc-mesboot1)
     (name "gcc-mesboot")
-    (version "4.9.4")
+    (version (package-version gcc-4.9))
     (source (bootstrap-origin (package-source gcc-4.9)))
     (native-inputs `(("binutils" ,binutils-mesboot)
                      ("gcc-wrapper" ,gcc-mesboot1-wrapper)
-- 
2.25.0

[bug#39414] [PATCH 2/2] gnu: commencement: Rationalize search path handling.

[Ludovic Courtès]

This commit ensures that only packages that correspond to compilers have
a search path set.  It also reduces manual handling of search path
environment variables.

* gnu/packages/commencement.scm (tcc-boot0)[native-search-paths]: Remove
copy/pasted comment.
(gcc-core-mesboot)[arguments]: In 'install2' phase, do not copy
TCC/include to OUT/include.
[native-search-paths]: Remove leading slash in "lib/gcc-lib/…" directory name.
(mesboot-headers)[native-search-paths]: Remove.
(glibc-mesboot0)[native-search-paths]: Remove.
(gcc-mesboot0)[native-inputs]: Reorder so that we have libc, then
kernel-headers, then gcc.
[arguments]: Rewrite 'setenv' phase to only set CONFIG_SHELL and create
'config.cache'.
(gcc-mesboot1)[native-inputs]: Reorder similarly.
[arguments]: In 'setenv' phase, only set CONFIG_SHELL, C_INCLUDE_PATH,
and CPLUS_INCLUDE_PATH.
(glibc-headers-mesboot)[arguments]: In 'setenv' phase, replace
references to '%build-inputs' by references to 'inputs'; simplify
setting of CONFIG_SHELL and SHELL; simplify patching of /bin/pwd in
the "configure" file; leave C_INCLUDE_PATH and LIBRARY_PATH unset.
(glibc-mesboot)[native-search-paths]: Remove.
(gcc-mesboot)[native-inputs]: Reorder.
[arguments]: Remove clause for #:phases that would change the 'setenv'
phase.
---
 gnu/packages/commencement.scm | 197 ++++++++--------------------------
 1 file changed, 46 insertions(+), 151 deletions(-)

diff --git a/gnu/packages/commencement.scm b/gnu/packages/commencement.scm
index f011891725..adc4447454 100644
--- a/gnu/packages/commencement.scm
+++ b/gnu/packages/commencement.scm
@@ -1,5 +1,5 @@
 ;;; GNU Guix --- Functional package management for GNU
-;;; Copyright © 2012, 2013, 2014, 2015, 2016, 2017, 2018, 2019 Ludovic Courtès <ludo@gnu.org>
+;;; Copyright © 2012, 2013, 2014, 2015, 2016, 2017, 2018, 2019, 2020 Ludovic Courtès <ludo@gnu.org>
 ;;; Copyright © 2014 Andreas Enge <andreas@enge.fr>
 ;;; Copyright © 2012 Nikita Karetnikov <nikita@karetnikov.org>
 ;;; Copyright © 2014, 2015, 2017 Mark H Weaver <mhw@netris.org>
@@ -148,11 +148,6 @@
            (lambda _
              (invoke "sh" "install.sh"))))))
     (native-search-paths
-     ;; Use the language-specific variables rather than 'CPATH' because they
-     ;; are equivalent to '-isystem' whereas 'CPATH' is equivalent to '-I'.
-     ;; The intent is to allow headers that are in the search path to be
-     ;; treated as "system headers" (headers exempt from warnings) just like
-     ;; the typical /usr/include headers on an FHS system.
      (list (search-path-specification
             (variable "C_INCLUDE_PATH")
             (files '("share/mes/include")))
@@ -266,11 +261,6 @@
              (lambda _
                (invoke "sh" "install.sh"))))))
       (native-search-paths
-       ;; Use the language-specific variables rather than 'CPATH' because they
-       ;; are equivalent to '-isystem' whereas 'CPATH' is equivalent to '-I'.
-       ;; The intent is to allow headers that are in the search path to be
-       ;; treated as "system headers" (headers exempt from warnings) just like
-       ;; the typical /usr/include headers on an FHS system.
        (list (search-path-specification
               (variable "C_INCLUDE_PATH")
               (files '("include")))
@@ -627,18 +617,14 @@ ac_cv_c_float_format='IEEE (little-endian)'
                        (string-append tcc-lib "/libc+gnu.o")
                        (string-append tcc-lib "/libtcc1.o"))
                (invoke "ls" "-ltrF" gcc-dir)
-               (copy-recursively (string-append tcc "/include")
-                                 (string-append out "/include"))
                #t))))))
     (native-search-paths
-     ;; Use the language-specific variables rather than 'CPATH' because they
-     ;; are equivalent to '-isystem' whereas 'CPATH' is equivalent to '-I'.
-     ;; The intent is to allow headers that are in the search path to be
-     ;; treated as "system headers" (headers exempt from warnings) just like
-     ;; the typical /usr/include headers on an FHS system.
      (list (search-path-specification
             (variable "C_INCLUDE_PATH")
-            (files '("include" "/lib/gcc-lib/i686-unknown-linux-gnu/2.95.3/include")))
+            (files '("include"
+
+                     ;; Needed to get things like GCC's <stddef.h>.
+                     "lib/gcc-lib/i686-unknown-linux-gnu/2.95.3/include")))
            (search-path-specification
             (variable "LIBRARY_PATH")
             (files '("lib")))))))
@@ -669,16 +655,7 @@ ac_cv_c_float_format='IEEE (little-endian)'
                (mkdir-p include)
                (copy-recursively "include" out)
                (copy-recursively headers out)
-               #t))))))
-    (native-search-paths
-     ;; Use the language-specific variables rather than 'CPATH' because they
-     ;; are equivalent to '-isystem' whereas 'CPATH' is equivalent to '-I'.
-     ;; The intent is to allow headers that are in the search path to be
-     ;; treated as "system headers" (headers exempt from warnings) just like
-     ;; the typical /usr/include headers on an FHS system.
-     (list (search-path-specification
-            (variable "C_INCLUDE_PATH")
-            (files '("include")))))))
+               #t))))))))
 
 (define glibc-mesboot0
   ;; GNU C Library 2.2.5 is the most recent glibc that we managed to build
@@ -760,67 +737,38 @@ ac_cv_c_float_format='IEEE (little-endian)'
            (lambda* (#:key configure-flags #:allow-other-keys)
              (format (current-error-port)
                      "running ./configure ~a\n" (string-join configure-flags))
-             (apply invoke "./configure" configure-flags))))))
-    (native-search-paths
-     ;; Use the language-specific variables rather than 'CPATH' because they
-     ;; are equivalent to '-isystem' whereas 'CPATH' is equivalent to '-I'.
-     ;; The intent is to allow headers that are in the search path to be
-     ;; treated as "system headers" (headers exempt from warnings) just like
-     ;; the typical /usr/include headers on an FHS system.
-     (list (search-path-specification
-            (variable "C_INCLUDE_PATH")
-            (files '("include")))
-           (search-path-specification
-            (variable "CPLUS_INCLUDE_PATH")
-            (files '("include")))
-           (search-path-specification
-            (variable "LIBRARY_PATH")
-            (files '("lib")))))))
+             (apply invoke "./configure" configure-flags))))))))
 
 (define gcc-mesboot0
   (package
     (inherit gcc-core-mesboot)
     (name "gcc-mesboot0")
     (native-inputs `(("binutils" ,binutils-mesboot0)
-                     ("gcc" ,gcc-core-mesboot)
+
+                     ;; GCC-CORE-MESBOOT must appear last because it also
+                     ;; provides libc headers from Mes that we want to shadow
+                     ;; with those of GLIBC-MESBOOT0.
                      ("libc" ,glibc-mesboot0)
+                     ("kernel-headers" ,%bootstrap-linux-libre-headers)
+                     ("gcc" ,gcc-core-mesboot)
 
                      ("bash" ,%bootstrap-coreutils&co)
                      ("coreutils" ,%bootstrap-coreutils&co)
                      ("diffutils" ,diffutils-mesboot)
-                     ("kernel-headers" ,%bootstrap-linux-libre-headers)
                      ("make" ,make-mesboot0)))
     (arguments
      (substitute-keyword-arguments (package-arguments gcc-core-mesboot)
        ((#:phases phases)
         `(modify-phases ,phases
            (replace 'setenv
-             (lambda* (#:key outputs #:allow-other-keys)
-               (let ((out (assoc-ref outputs "out"))
-                     (bash (assoc-ref %build-inputs "bash"))
-                     (gcc (assoc-ref %build-inputs "gcc"))
-                     (glibc (assoc-ref %build-inputs "libc"))
-                     (kernel-headers (assoc-ref %build-inputs "kernel-headers")))
-                 (setenv "CONFIG_SHELL" (string-append bash "/bin/sh"))
-                 (format (current-error-port) "C_INCLUDE_PATH=~a\n" (getenv "C_INCLUDE_PATH"))
-                 (setenv "C_INCLUDE_PATH" (string-append
-                                           gcc "/lib/gcc-lib/i686-unknown-linux-gnu/2.95.3/include"
-                                           ":" kernel-headers "/include"
-                                           ":" glibc "/include"))
-                 (format (current-error-port) "C_INCLUDE_PATH=~a\n" (getenv "C_INCLUDE_PATH"))
-                 (format (current-error-port) "LIBRARY_PATH=~a\n" (getenv "LIBRARY_PATH"))
-                 ;; FIXME: add glibc dirs to paths manually
-                 (setenv "LIBRARY_PATH" (string-join
-                                         (list (string-append glibc "/lib")
-                                               (getenv "LIBRARY_PATH"))
-                                         ":"))
-                 (format (current-error-port) "LIBRARY_PATH=~a\n" (getenv "LIBRARY_PATH"))
-                 (with-output-to-file "config.cache"
-                   (lambda _
-                     (display "
+             (lambda _
+               (setenv "CONFIG_SHELL" (which "sh"))
+               (with-output-to-file "config.cache"
+                 (lambda _
+                   (display "
 ac_cv_c_float_format='IEEE (little-endian)'
 ")))
-                 #t)))
+               #t))
            (replace 'install2
              (lambda* (#:key outputs #:allow-other-keys)
                (let* ((out (assoc-ref outputs "out"))
@@ -946,13 +894,14 @@ ac_cv_c_float_format='IEEE (little-endian)'
               ("mpfr-source" ,(package-source mpfr-boot))
               ("mpc-source" ,(package-source mpc-boot))))
     (native-inputs `(("binutils" ,binutils-mesboot)
-                     ("gcc" ,gcc-mesboot0)
+
                      ("libc" ,glibc-mesboot0)
+                     ("kernel-headers" ,%bootstrap-linux-libre-headers)
+                     ("gcc" ,gcc-mesboot0)
 
                      ("bash" ,%bootstrap-coreutils&co)
                      ("coreutils" ,%bootstrap-coreutils&co)
                      ("diffutils" ,diffutils-mesboot)
-                     ("kernel-headers" ,%bootstrap-linux-libre-headers)
                      ("make" ,make-mesboot)))
     (arguments
      (substitute-keyword-arguments (package-arguments gcc-core-mesboot)
@@ -994,24 +943,18 @@ ac_cv_c_float_format='IEEE (little-endian)'
                  #t)))
            (delete 'remove-info)
            (replace 'setenv
-             (lambda* (#:key outputs #:allow-other-keys)
-               (let* ((out (assoc-ref outputs "out"))
-                      (binutils (assoc-ref %build-inputs "binutils"))
-                      (bash (assoc-ref %build-inputs "bash"))
-                      (gcc (assoc-ref %build-inputs "gcc"))
-                      (glibc (assoc-ref %build-inputs "libc"))
-                      (kernel-headers (assoc-ref %build-inputs "kernel-headers")))
-                 (setenv "CONFIG_SHELL" (string-append bash "/bin/sh"))
-                 (setenv "C_INCLUDE_PATH" (string-append
-                                           gcc "/lib/gcc-lib/i686-unknown-linux-gnu/2.95.3/include"
-                                           ":" kernel-headers "/include"
-                                           ":" glibc "/include"
-                                           ":" (getcwd) "/mpfr/src"))
-                 (setenv "LIBRARY_PATH" (string-append glibc "/lib"
-                                                       ":" gcc "/lib"))
-                 (format (current-error-port) "C_INCLUDE_PATH=~a\n" (getenv "C_INCLUDE_PATH"))
-                 (format (current-error-port) "LIBRARY_PATH=~a\n" (getenv "LIBRARY_PATH"))
-                 #t)))
+             (lambda _
+               (setenv "CONFIG_SHELL" (which "sh"))
+
+               ;; Allow MPFR headers to be found.
+               (setenv "C_INCLUDE_PATH"
+                       (string-append (getcwd) "/mpfr/src:"
+                                      (getenv "C_INCLUDE_PATH")))
+
+               ;; Set the C++ search path so that C headers can be found as
+               ;; libstdc++ is being compiled.
+               (setenv "CPLUS_INCLUDE_PATH" (getenv "C_INCLUDE_PATH"))
+               #t))
            (delete 'install2)))
        ((#:configure-flags configure-flags)
         `(let ((out (assoc-ref %outputs "out"))
@@ -1158,22 +1101,18 @@ exec " gcc "/bin/" program
        ((#:phases phases)
         `(modify-phases ,phases
            (replace 'setenv
-             (lambda* (#:key outputs #:allow-other-keys)
-               (let* ((out (assoc-ref outputs "out"))
-                      (headers (assoc-ref %build-inputs "headers"))
-                      (bash (assoc-ref %build-inputs "bash"))
-                      (coreutils (assoc-ref %build-inputs "coreutils"))
-                      (libc (assoc-ref %build-inputs "libc"))
-                      (gcc (assoc-ref %build-inputs "gcc"))
+             (lambda* (#:key inputs #:allow-other-keys)
+               (let* ((headers  (assoc-ref inputs "headers"))
+                      (libc     (assoc-ref inputs "libc"))
+                      (gcc      (assoc-ref inputs "gcc"))
                       (cppflags (string-append
                                  " -I " (getcwd) "/nptl/sysdeps/pthread/bits"
                                  " -D BOOTSTRAP_GLIBC=1"))
                       (cflags (string-append " -L " (getcwd)
                                              " -L " libc "/lib")))
                  (setenv "libc_cv_friendly_stddef" "yes")
-                 (setenv "CONFIG_SHELL" (string-append bash "/bin/sh"))
-                 (setenv "SHELL" (getenv "CONFIG_SHELL"))
-                 (format (current-error-port) "CONFIG_SHELL=~s\n" (getenv "CONFIG_SHELL"))
+                 (setenv "CONFIG_SHELL" (which "sh"))
+                 (setenv "SHELL" (which "sh"))
 
                  (setenv "CPP" (string-append gcc "/bin/gcc -E " cppflags))
                  (setenv "CC" (string-append gcc "/bin/gcc " cppflags cflags))
@@ -1181,10 +1120,7 @@ exec " gcc "/bin/" program
                  ;; avoid -fstack-protector
                  (setenv "libc_cv_ssp" "false")
                  (substitute* "configure"
-                   (("/bin/pwd") (string-append coreutils "/bin/pwd")))
-                 (setenv "C_INCLUDE_PATH" (string-append libc "/include"
-                                                         headers "/include"))
-                 (setenv "LIBRARY_PATH" (string-append libc "/lib"))
+                   (("/bin/pwd") "pwd"))
                  #t)))
            (replace 'install
              (lambda* (#:key outputs make-flags #:allow-other-keys)
@@ -1244,22 +1180,7 @@ exec " gcc "/bin/" program
                           (install-flags (cons "install" make-flags)))
                      (apply invoke "make" install-flags)
                      (copy-recursively kernel-headers out)
-                     #t))))))))
-    (native-search-paths ;; FIXME: move to glibc-mesboot0
-     ;; Use the language-specific variables rather than 'CPATH' because they
-     ;; are equivalent to '-isystem' whereas 'CPATH' is equivalent to '-I'.
-     ;; The intent is to allow headers that are in the search path to be
-     ;; treated as "system headers" (headers exempt from warnings) just like
-     ;; the typical /usr/include headers on an FHS system.
-     (list (search-path-specification
-            (variable "C_INCLUDE_PATH")
-            (files '("include")))
-           (search-path-specification
-            (variable "CPLUS_INCLUDE_PATH")
-            (files '("include")))
-           (search-path-specification
-            (variable "LIBRARY_PATH")
-            (files '("lib")))))))
+                     #t))))))))))
 
 (define gcc-mesboot
   (package
@@ -1268,14 +1189,15 @@ exec " gcc "/bin/" program
     (version (package-version gcc-4.9))
     (source (bootstrap-origin (package-source gcc-4.9)))
     (native-inputs `(("binutils" ,binutils-mesboot)
+
+                     ("libc" ,glibc-mesboot)
+                     ("kernel-headers" ,%bootstrap-linux-libre-headers)
                      ("gcc-wrapper" ,gcc-mesboot1-wrapper)
                      ("gcc" ,gcc-mesboot1)
-                     ("libc" ,glibc-mesboot)
 
                      ("bash" ,%bootstrap-coreutils&co)
                      ("coreutils" ,%bootstrap-coreutils&co)
                      ("diffutils" ,diffutils-mesboot)
-                     ("kernel-headers" ,%bootstrap-linux-libre-headers)
                      ("make" ,make-mesboot)))
     (arguments
      `(#:validate-runpath? #f
@@ -1318,34 +1240,7 @@ exec " gcc "/bin/" program
                      "--disable-libstdcxx-pch"
 
                      ;; for libcpp ...
-                     "--disable-build-with-cxx")))
-           ((#:phases phases)
-            `(modify-phases ,phases
-               (replace 'setenv
-                 (lambda* (#:key outputs #:allow-other-keys)
-                   (let* ((out (assoc-ref outputs "out"))
-                          (binutils (assoc-ref %build-inputs "binutils"))
-                          (bash (assoc-ref %build-inputs "bash"))
-                          (gcc (assoc-ref %build-inputs "gcc"))
-                          (glibc (assoc-ref %build-inputs "libc"))
-                          (kernel-headers (assoc-ref %build-inputs "kernel-headers")))
-                     (setenv "CONFIG_SHELL" (string-append bash "/bin/sh"))
-                     (setenv "C_INCLUDE_PATH" (string-append
-                                               gcc "/lib/gcc-lib/i686-unknown-linux-gnu/4.7.4/include"
-                                               ":" kernel-headers "/include"
-                                               ":" glibc "/include"
-                                               ":" (getcwd) "/mpfr/src"))
-                     (setenv "CPLUS_INCLUDE_PATH" (string-append
-                                                   gcc "/lib/gcc-lib/i686-unknown-linux-gnu/4.7.4/include"
-                                                   ":" kernel-headers "/include"
-                                                   ":" glibc "/include"
-                                                   ":" (getcwd) "/mpfr/src"))
-                     (setenv "LIBRARY_PATH" (string-append glibc "/lib"
-                                                           ":" gcc "/lib"))
-                     (format (current-error-port) "C_INCLUDE_PATH=~a\n" (getenv "C_INCLUDE_PATH"))
-                     (format (current-error-port) "CPLUS_INCLUDE_PATH=~a\n" (getenv "CPLUS_INCLUDE_PATH"))
-                     (format (current-error-port) "LIBRARY_PATH=~a\n" (getenv "LIBRARY_PATH"))
-                     #t))))))))))
+                     "--disable-build-with-cxx"))))))))
 
 (define gcc-mesboot-wrapper
   ;; We need this so gcc-mesboot can be used to create shared binaries that
-- 
2.25.0

[bug#39414] [PATCH 2/2] gnu: commencement: Rationalize search path handling.

[Jan Nieuwenhuizen]

Ludovic Courtès writes:

> This commit ensures that only packages that correspond to compilers have
> a search path set.  It also reduces manual handling of search path
> environment variables.

Beautiful, thank you!

>      (native-inputs `(("binutils" ,binutils-mesboot0)
> -                     ("gcc" ,gcc-core-mesboot)
> +
> +                     ;; GCC-CORE-MESBOOT must appear last because it also
> +                     ;; provides libc headers from Mes that we want to shadow
> +                     ;; with those of GLIBC-MESBOOT0.
>                       ("libc" ,glibc-mesboot0)
> +                     ("kernel-headers" ,%bootstrap-linux-libre-headers)
> +                     ("gcc" ,gcc-core-mesboot)
>  
>                       ("bash" ,%bootstrap-coreutils&co)
>                       ("coreutils" ,%bootstrap-coreutils&co)
>                       ("diffutils" ,diffutils-mesboot)
> -                     ("kernel-headers" ,%bootstrap-linux-libre-headers)
>                       ("make" ,make-mesboot0)))

Here it gets real interesting; it seems above you actually fix (or work
around?) a bug that allowed you to...

>      (arguments
>       (substitute-keyword-arguments (package-arguments gcc-core-mesboot)
>         ((#:phases phases)
>          `(modify-phases ,phases
>             (replace 'setenv
> -             (lambda* (#:key outputs #:allow-other-keys)
> -               (let ((out (assoc-ref outputs "out"))
> -                     (bash (assoc-ref %build-inputs "bash"))
> -                     (gcc (assoc-ref %build-inputs "gcc"))
> -                     (glibc (assoc-ref %build-inputs "libc"))
> -                     (kernel-headers (assoc-ref %build-inputs "kernel-headers")))
> -                 (setenv "CONFIG_SHELL" (string-append bash "/bin/sh"))
> -                 (format (current-error-port) "C_INCLUDE_PATH=~a\n" (getenv "C_INCLUDE_PATH"))
> -                 (setenv "C_INCLUDE_PATH" (string-append
> -                                           gcc "/lib/gcc-lib/i686-unknown-linux-gnu/2.95.3/include"
> -                                           ":" kernel-headers "/include"
> -                                           ":" glibc "/include"))
> -                 (format (current-error-port) "C_INCLUDE_PATH=~a\n" (getenv "C_INCLUDE_PATH"))
> -                 (format (current-error-port) "LIBRARY_PATH=~a\n" (getenv "LIBRARY_PATH"))
> -                 ;; FIXME: add glibc dirs to paths manually
> -                 (setenv "LIBRARY_PATH" (string-join
> -                                         (list (string-append glibc "/lib")
> -                                               (getenv "LIBRARY_PATH"))
> -                                         ":"))
> -                 (format (current-error-port) "LIBRARY_PATH=~a\n" (getenv "LIBRARY_PATH"))
> -                 (with-output-to-file "config.cache"
> -                   (lambda _
> -                     (display "
> +             (lambda _
> +               (setenv "CONFIG_SHELL" (which "sh"))
> +               (with-output-to-file "config.cache"
> +                 (lambda _
> +                   (display "
>  ac_cv_c_float_format='IEEE (little-endian)'
>  ")))
> -                 #t)))
> +               #t))

remove this monster.  Although I am especially happy with it, I am
unsure exactly how it works.

> -             (lambda* (#:key outputs #:allow-other-keys)
> -               (let* ((out (assoc-ref outputs "out"))
> -                      (binutils (assoc-ref %build-inputs "binutils"))
> -                      (bash (assoc-ref %build-inputs "bash"))
> -                      (gcc (assoc-ref %build-inputs "gcc"))
> -                      (glibc (assoc-ref %build-inputs "libc"))
> -                      (kernel-headers (assoc-ref %build-inputs "kernel-headers")))
> -                 (setenv "CONFIG_SHELL" (string-append bash "/bin/sh"))
> -                 (setenv "C_INCLUDE_PATH" (string-append
> -                                           gcc "/lib/gcc-lib/i686-unknown-linux-gnu/2.95.3/include"
> -                                           ":" kernel-headers "/include"
> -                                           ":" glibc "/include"
> -                                           ":" (getcwd) "/mpfr/src"))
> -                 (setenv "LIBRARY_PATH" (string-append glibc "/lib"
> -                                                       ":" gcc "/lib"))
> -                 (format (current-error-port) "C_INCLUDE_PATH=~a\n" (getenv "C_INCLUDE_PATH"))
> -                 (format (current-error-port) "LIBRARY_PATH=~a\n" (getenv "LIBRARY_PATH"))
> -                 #t)))
> +             (lambda _
> +               (setenv "CONFIG_SHELL" (which "sh"))
> +
> +               ;; Allow MPFR headers to be found.
> +               (setenv "C_INCLUDE_PATH"
> +                       (string-append (getcwd) "/mpfr/src:"
> +                                      (getenv "C_INCLUDE_PATH")))
> +
> +               ;; Set the C++ search path so that C headers can be found as
> +               ;; libstdc++ is being compiled.
> +               (setenv "CPLUS_INCLUDE_PATH" (getenv "C_INCLUDE_PATH"))
> +               #t))

Similar here,

> -                     "--disable-build-with-cxx")))
> -           ((#:phases phases)
> -            `(modify-phases ,phases
> -               (replace 'setenv
> -                 (lambda* (#:key outputs #:allow-other-keys)
> -                   (let* ((out (assoc-ref outputs "out"))
> -                          (binutils (assoc-ref %build-inputs "binutils"))
> -                          (bash (assoc-ref %build-inputs "bash"))
> -                          (gcc (assoc-ref %build-inputs "gcc"))
> -                          (glibc (assoc-ref %build-inputs "libc"))
> -                          (kernel-headers (assoc-ref %build-inputs "kernel-headers")))
> -                     (setenv "CONFIG_SHELL" (string-append bash "/bin/sh"))
> -                     (setenv "C_INCLUDE_PATH" (string-append
> -                                               gcc "/lib/gcc-lib/i686-unknown-linux-gnu/4.7.4/include"
> -                                               ":" kernel-headers "/include"
> -                                               ":" glibc "/include"
> -                                               ":" (getcwd) "/mpfr/src"))
> -                     (setenv "CPLUS_INCLUDE_PATH" (string-append
> -                                                   gcc "/lib/gcc-lib/i686-unknown-linux-gnu/4.7.4/include"
> -                                                   ":" kernel-headers "/include"
> -                                                   ":" glibc "/include"
> -                                                   ":" (getcwd) "/mpfr/src"))
> -                     (setenv "LIBRARY_PATH" (string-append glibc "/lib"
> -                                                           ":" gcc "/lib"))
> -                     (format (current-error-port) "C_INCLUDE_PATH=~a\n" (getenv "C_INCLUDE_PATH"))
> -                     (format (current-error-port) "CPLUS_INCLUDE_PATH=~a\n" (getenv "CPLUS_INCLUDE_PATH"))
> -                     (format (current-error-port) "LIBRARY_PATH=~a\n" (getenv "LIBRARY_PATH"))
> -                     #t))))))))))
> +                     "--disable-build-with-cxx"))))))))

and same here.

I tried your commit and it works for me.  Rebasing `wip-bootstrap' onto
it was some work as you might imagine.  The good news is that I finally
got it to "work" again (`wip-bootstrap' @ gitlab); the bad news is
that I have kept (or put back, actually) one of these 'setenv phases in
`gcc-mesboot0'.  Well, that's for later worry; so this will need another
look when merging the scheme-only "wip-bootsrap".

For this patch: LVGTM (looks /very/ good to me!).

Thank you!
janneke

-- 
Jan Nieuwenhuizen <janneke@gnu.org> | GNU LilyPond http://lilypond.org
Freelance IT http://JoyofSource.com | Avatar® http://AvatarAcademy.com

[bug#39414] [PATCH 2/2] gnu: commencement: Rationalize search path handling.

[Ludovic Courtès]

Hello! :-)

Jan Nieuwenhuizen <janneke@gnu.org> skribis:

>>      (native-inputs `(("binutils" ,binutils-mesboot0)
>> -                     ("gcc" ,gcc-core-mesboot)
>> +
>> +                     ;; GCC-CORE-MESBOOT must appear last because it also
>> +                     ;; provides libc headers from Mes that we want to shadow
>> +                     ;; with those of GLIBC-MESBOOT0.
>>                       ("libc" ,glibc-mesboot0)
>> +                     ("kernel-headers" ,%bootstrap-linux-libre-headers)
>> +                     ("gcc" ,gcc-core-mesboot)
>>  
>>                       ("bash" ,%bootstrap-coreutils&co)
>>                       ("coreutils" ,%bootstrap-coreutils&co)
>>                       ("diffutils" ,diffutils-mesboot)
>> -                     ("kernel-headers" ,%bootstrap-linux-libre-headers)
>>                       ("make" ,make-mesboot0)))
>
> Here it gets real interesting; it seems above you actually fix (or work
> around?) a bug that allowed you to...

[...]

>> -                 (setenv "C_INCLUDE_PATH" (string-append
>> -                                           gcc "/lib/gcc-lib/i686-unknown-linux-gnu/2.95.3/include"
>> -                                           ":" kernel-headers "/include"
>> -                                           ":" glibc "/include"))
>> -                 (format (current-error-port) "C_INCLUDE_PATH=~a\n" (getenv "C_INCLUDE_PATH"))
>> -                 (format (current-error-port) "LIBRARY_PATH=~a\n" (getenv "LIBRARY_PATH"))
>> -                 ;; FIXME: add glibc dirs to paths manually
>> -                 (setenv "LIBRARY_PATH" (string-join
>> -                                         (list (string-append glibc "/lib")
>> -                                               (getenv "LIBRARY_PATH"))
>> -                                         ":"))
>> -                 (format (current-error-port) "LIBRARY_PATH=~a\n" (getenv "LIBRARY_PATH"))
>> -                 (with-output-to-file "config.cache"
>> -                   (lambda _
>> -                     (display "
>> +             (lambda _
>> +               (setenv "CONFIG_SHELL" (which "sh"))
>> +               (with-output-to-file "config.cache"
>> +                 (lambda _
>> +                   (display "
>>  ac_cv_c_float_format='IEEE (little-endian)'
>>  ")))
>> -                 #t)))
>> +               #t))
>
> remove this monster.  Although I am especially happy with it, I am
> unsure exactly how it works.

I analyzed what the monster does: it constructs *PATH exactly like the
search path machinery would do.  The big difference I noticed was that
it excluded ‘gcc-core-mesboot’ from the search path.  I understood this
was because ‘gcc-core-mesboot’ contained libc headers (from Mes) and we
didn’t want them to shadow glibc headers.

So I first moved ‘gcc-core-mesboot’ down in the ordering, but then found
out that we could just remove the include/ directory from
‘gcc-core-mesboot’ altogether, which I did.

(I’ll fix the comment that says that ‘gcc-core-mesboot’ provides libc
headers from Mes because that’s no longer the case.)

>> -                 (setenv "C_INCLUDE_PATH" (string-append
>> -                                           gcc "/lib/gcc-lib/i686-unknown-linux-gnu/2.95.3/include"
>> -                                           ":" kernel-headers "/include"
>> -                                           ":" glibc "/include"
>> -                                           ":" (getcwd) "/mpfr/src"))
>> -                 (setenv "LIBRARY_PATH" (string-append glibc "/lib"
>> -                                                       ":" gcc "/lib"))
>> -                 (format (current-error-port) "C_INCLUDE_PATH=~a\n" (getenv "C_INCLUDE_PATH"))
>> -                 (format (current-error-port) "LIBRARY_PATH=~a\n" (getenv "LIBRARY_PATH"))
>> -                 #t)))
>> +             (lambda _
>> +               (setenv "CONFIG_SHELL" (which "sh"))
>> +
>> +               ;; Allow MPFR headers to be found.
>> +               (setenv "C_INCLUDE_PATH"
>> +                       (string-append (getcwd) "/mpfr/src:"
>> +                                      (getenv "C_INCLUDE_PATH")))
>> +
>> +               ;; Set the C++ search path so that C headers can be found as
>> +               ;; libstdc++ is being compiled.
>> +               (setenv "CPLUS_INCLUDE_PATH" (getenv "C_INCLUDE_PATH"))
>> +               #t))
>
> Similar here,

Here I found that the only difference was adding “mpfr/src” to
‘C_INCLUDE_PATH’.

>> -                     (setenv "C_INCLUDE_PATH" (string-append
>> -                                               gcc "/lib/gcc-lib/i686-unknown-linux-gnu/4.7.4/include"
>> -                                               ":" kernel-headers "/include"
>> -                                               ":" glibc "/include"
>> -                                               ":" (getcwd) "/mpfr/src"))
>> -                     (setenv "CPLUS_INCLUDE_PATH" (string-append
>> -                                                   gcc "/lib/gcc-lib/i686-unknown-linux-gnu/4.7.4/include"
>> -                                                   ":" kernel-headers "/include"
>> -                                                   ":" glibc "/include"
>> -                                                   ":" (getcwd) "/mpfr/src"))
>> -                     (setenv "LIBRARY_PATH" (string-append glibc "/lib"
>> -                                                           ":" gcc "/lib"))
>> -                     (format (current-error-port) "C_INCLUDE_PATH=~a\n" (getenv "C_INCLUDE_PATH"))
>> -                     (format (current-error-port) "CPLUS_INCLUDE_PATH=~a\n" (getenv "CPLUS_INCLUDE_PATH"))
>> -                     (format (current-error-port) "LIBRARY_PATH=~a\n" (getenv "LIBRARY_PATH"))
>> -                     #t))))))))))
>> +                     "--disable-build-with-cxx"))))))))
>
> and same here.

Same story.

> I tried your commit and it works for me.  Rebasing `wip-bootstrap' onto
> it was some work as you might imagine.  The good news is that I finally
> got it to "work" again (`wip-bootstrap' @ gitlab); the bad news is
> that I have kept (or put back, actually) one of these 'setenv phases in
> `gcc-mesboot0'.  Well, that's for later worry; so this will need another
> look when merging the scheme-only "wip-bootsrap".

Yes, we’ll have to see what can be done to not put these phases back in.

My approach was to start by looking at what the ‘set-paths’ phase prints
and see what was missing or incorrect in the values it computes.

Thanks for testing & reviewing!  I’ll push shortly.

Ludo’.

bug#39414: [PATCH 2/2] gnu: commencement: Rationalize search path handling.

[Ludovic Courtès]

Pushed as 558b0bbe291b2f2cd38b7f4eadc827e2ed102c54!

Ludo'.

[bug#39414] [PATCH 1/2] gnu: commencement: Avoid hard-coded GCC version numbers.

[Jan Nieuwenhuizen]

Ludovic Courtès writes:

> * gnu/packages/commencement.scm (gcc-mesboot1, gcc-mesboot): Use
> 'package-version' instead of hard-coding the version number.

Makes sense, LGTM.
janneke

-- 
Jan Nieuwenhuizen <janneke@gnu.org> | GNU LilyPond http://lilypond.org
Freelance IT http://JoyofSource.com | Avatar® http://AvatarAcademy.com

[bug#39414] [PATCH core-updates 0/2] Clarify search path handling in commencement.scm

[Jan Nieuwenhuizen]

Ludovic Courtès writes:

Hello Ludo',

> The patch below for current ‘core-updates’ is an attempt to clarify
> search path handling in commencement.scm by:

(Weird, this initial mail got sorted into a new debbugs-submit folder,
only found it later)

>   1. Having ‘native-search-paths’ fields only for compilers—e.g.,
>      “C_INCLUDE_PATH” belongs to the compiler, not to libc.
>
>   2. Avoiding phases that manually fiddle with search path
>      environment variables: normally, this is handled automatically
>      by the ‘set-paths’ phase based on the declared search paths,
>      so manual fiddling should be a last resort and it should be
>      well commented so we remember why it’s there.
>
> This is an attempt to reduce complexity and keep things declarative
> as much as possible.  I’ve tested it on top of
> 46312064de8ae0cca531fddbc4b5ec8421e5d866 and “guix build coreutils”
> succeeds.

Very nice, thank you.  I was happy to get it to build and did not
imagine all this fiddling could actually be workarounds that could
(should!)  all be removed.  Not only will this help readability and
maintenance, it will make porting this to other distributions (NixOS) a
lot easier too.

> There’s another pattern that I found hard to follow that has to do
> with the reuse of build phases.
>
> For example, there’s a build phase named ‘setenv’ (perhaps we should
> find a more descriptive name :-)) in the various GCCs that is reused
> or replaced; when looking at a specific package, it’s difficult to
> see which phases it really runs because this particular phase is
> inherited and modified on several layers.  If I can make time for it,
> I’ll see if I can come up with a proposal to clarify this, but at any
> rate, it’s probably something to keep in mind for future changes.
>
> Thoughts?  (I’m particularly interested in your feedback, janneke!)

Yes, I agree.  A first step could be to use better names and possibly
split it up into serveral stages: set-configure-shells, set-cc-paths?

Doing this will probably only need overriding the set-cc-paths.  I'm not
sure how to make the inherit+replace issue more obvious but it has
been biting me and annoying me too.

Maybe when we get into this replace trickery it is better to not reuse
parent's stages at all

>      (arguments
>       (substitute-keyword-arguments (package-arguments gcc-core-mesboot)

but fully rewrite (arguments ...)?  I haven't looked into the
consequences.  In any case, with patch it has gotten a lot better
already, thank you!

janneke

-- 
Jan Nieuwenhuizen <janneke@gnu.org> | GNU LilyPond http://lilypond.org
Freelance IT http://JoyofSource.com | Avatar® http://AvatarAcademy.com

[bug#39414] [PATCH core-updates 0/2] Clarify search path handling in commencement.scm

[Ludovic Courtès]

Jan Nieuwenhuizen <janneke@gnu.org> skribis:

>>   1. Having ‘native-search-paths’ fields only for compilers—e.g.,
>>      “C_INCLUDE_PATH” belongs to the compiler, not to libc.
>>
>>   2. Avoiding phases that manually fiddle with search path
>>      environment variables: normally, this is handled automatically
>>      by the ‘set-paths’ phase based on the declared search paths,
>>      so manual fiddling should be a last resort and it should be
>>      well commented so we remember why it’s there.
>>
>> This is an attempt to reduce complexity and keep things declarative
>> as much as possible.  I’ve tested it on top of
>> 46312064de8ae0cca531fddbc4b5ec8421e5d866 and “guix build coreutils”
>> succeeds.
>
> Very nice, thank you.  I was happy to get it to build and did not
> imagine all this fiddling could actually be workarounds that could
> (should!)  all be removed.  Not only will this help readability and
> maintenance, it will make porting this to other distributions (NixOS) a
> lot easier too.

Yes, I hope so!

>> There’s another pattern that I found hard to follow that has to do
>> with the reuse of build phases.
>>
>> For example, there’s a build phase named ‘setenv’ (perhaps we should
>> find a more descriptive name :-)) in the various GCCs that is reused
>> or replaced; when looking at a specific package, it’s difficult to
>> see which phases it really runs because this particular phase is
>> inherited and modified on several layers.  If I can make time for it,
>> I’ll see if I can come up with a proposal to clarify this, but at any
>> rate, it’s probably something to keep in mind for future changes.
>>
>> Thoughts?  (I’m particularly interested in your feedback, janneke!)
>
> Yes, I agree.  A first step could be to use better names and possibly
> split it up into serveral stages: set-configure-shells, set-cc-paths?
>
> Doing this will probably only need overriding the set-cc-paths.  I'm not
> sure how to make the inherit+replace issue more obvious but it has
> been biting me and annoying me too.

Yes, I’m not sure exactly how to do it.

> Maybe when we get into this replace trickery it is better to not reuse
> parent's stages at all
>
>>      (arguments
>>       (substitute-keyword-arguments (package-arguments gcc-core-mesboot)
>
> but fully rewrite (arguments ...)?

Yes, either that or always inherit from the same package—e.g., always
inherit from ‘gcc’ instead of inheriting from ‘gcc-mesboot1’, which
inherits from ‘gcc-mesboot0’, and so on.  That way one doesn’t have to
walk the inheritance chain to understand what the phases are.

Another option is to also make the interesting phases generic enough
that we don’t have to modify them in package variants.

We’ll see!

Ludo’.