unofficial mirror of guix-patches@gnu.org 
 help / color / mirror / code / Atom feed
* [bug#39414] [PATCH core-updates 0/2] Clarify search path handling in commencement.scm
@ 2020-02-04 12:53 Ludovic Courtès
  2020-02-04 13:00 ` [bug#39414] [PATCH 1/2] gnu: commencement: Avoid hard-coded GCC version numbers Ludovic Courtès
  2020-02-06  6:39 ` [bug#39414] [PATCH core-updates 0/2] Clarify search path handling in commencement.scm Jan Nieuwenhuizen
  0 siblings, 2 replies; 9+ messages in thread
From: Ludovic Courtès @ 2020-02-04 12:53 UTC (permalink / raw)
  To: 39414; +Cc: Ludovic Courtès, janneke

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [bug#39414] [PATCH 1/2] gnu: commencement: Avoid hard-coded GCC version numbers.
  2020-02-04 12:53 [bug#39414] [PATCH core-updates 0/2] Clarify search path handling in commencement.scm Ludovic Courtès
@ 2020-02-04 13:00 ` Ludovic Courtès
  2020-02-04 13:00   ` [bug#39414] [PATCH 2/2] gnu: commencement: Rationalize search path handling Ludovic Courtès
  2020-02-06  6:16   ` [bug#39414] [PATCH 1/2] gnu: commencement: Avoid hard-coded GCC version numbers Jan Nieuwenhuizen
  2020-02-06  6:39 ` [bug#39414] [PATCH core-updates 0/2] Clarify search path handling in commencement.scm Jan Nieuwenhuizen
  1 sibling, 2 replies; 9+ messages in thread
From: Ludovic Courtès @ 2020-02-04 13:00 UTC (permalink / raw)
  To: 39414; +Cc: Ludovic Courtès, janneke

* 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

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [bug#39414] [PATCH 2/2] gnu: commencement: Rationalize search path handling.
  2020-02-04 13:00 ` [bug#39414] [PATCH 1/2] gnu: commencement: Avoid hard-coded GCC version numbers Ludovic Courtès
@ 2020-02-04 13:00   ` Ludovic Courtès
  2020-02-06  6:25     ` Jan Nieuwenhuizen
  2020-02-06  6:16   ` [bug#39414] [PATCH 1/2] gnu: commencement: Avoid hard-coded GCC version numbers Jan Nieuwenhuizen
  1 sibling, 1 reply; 9+ messages in thread
From: Ludovic Courtès @ 2020-02-04 13:00 UTC (permalink / raw)
  To: 39414; +Cc: Ludovic Courtès, janneke

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

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [bug#39414] [PATCH 1/2] gnu: commencement: Avoid hard-coded GCC version numbers.
  2020-02-04 13:00 ` [bug#39414] [PATCH 1/2] gnu: commencement: Avoid hard-coded GCC version numbers Ludovic Courtès
  2020-02-04 13:00   ` [bug#39414] [PATCH 2/2] gnu: commencement: Rationalize search path handling Ludovic Courtès
@ 2020-02-06  6:16   ` Jan Nieuwenhuizen
  1 sibling, 0 replies; 9+ messages in thread
From: Jan Nieuwenhuizen @ 2020-02-06  6:16 UTC (permalink / raw)
  To: Ludovic Courtès; +Cc: 39414

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [bug#39414] [PATCH 2/2] gnu: commencement: Rationalize search path handling.
  2020-02-04 13:00   ` [bug#39414] [PATCH 2/2] gnu: commencement: Rationalize search path handling Ludovic Courtès
@ 2020-02-06  6:25     ` Jan Nieuwenhuizen
  2020-02-06 14:22       ` Ludovic Courtès
  0 siblings, 1 reply; 9+ messages in thread
From: Jan Nieuwenhuizen @ 2020-02-06  6:25 UTC (permalink / raw)
  To: Ludovic Courtès; +Cc: 39414

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [bug#39414] [PATCH core-updates 0/2] Clarify search path handling in commencement.scm
  2020-02-04 12:53 [bug#39414] [PATCH core-updates 0/2] Clarify search path handling in commencement.scm Ludovic Courtès
  2020-02-04 13:00 ` [bug#39414] [PATCH 1/2] gnu: commencement: Avoid hard-coded GCC version numbers Ludovic Courtès
@ 2020-02-06  6:39 ` Jan Nieuwenhuizen
  2020-02-06 14:25   ` Ludovic Courtès
  1 sibling, 1 reply; 9+ messages in thread
From: Jan Nieuwenhuizen @ 2020-02-06  6:39 UTC (permalink / raw)
  To: Ludovic Courtès; +Cc: 39414

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [bug#39414] [PATCH 2/2] gnu: commencement: Rationalize search path handling.
  2020-02-06  6:25     ` Jan Nieuwenhuizen
@ 2020-02-06 14:22       ` Ludovic Courtès
  2020-02-06 17:48         ` bug#39414: " Ludovic Courtès
  0 siblings, 1 reply; 9+ messages in thread
From: Ludovic Courtès @ 2020-02-06 14:22 UTC (permalink / raw)
  To: Jan Nieuwenhuizen; +Cc: 39414

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’.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [bug#39414] [PATCH core-updates 0/2] Clarify search path handling in commencement.scm
  2020-02-06  6:39 ` [bug#39414] [PATCH core-updates 0/2] Clarify search path handling in commencement.scm Jan Nieuwenhuizen
@ 2020-02-06 14:25   ` Ludovic Courtès
  0 siblings, 0 replies; 9+ messages in thread
From: Ludovic Courtès @ 2020-02-06 14:25 UTC (permalink / raw)
  To: Jan Nieuwenhuizen; +Cc: 39414

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’.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* bug#39414: [PATCH 2/2] gnu: commencement: Rationalize search path handling.
  2020-02-06 14:22       ` Ludovic Courtès
@ 2020-02-06 17:48         ` Ludovic Courtès
  0 siblings, 0 replies; 9+ messages in thread
From: Ludovic Courtès @ 2020-02-06 17:48 UTC (permalink / raw)
  To: Jan Nieuwenhuizen; +Cc: 39414-done

Pushed as 558b0bbe291b2f2cd38b7f4eadc827e2ed102c54!

Ludo'.

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2020-02-06 17:49 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-02-04 12:53 [bug#39414] [PATCH core-updates 0/2] Clarify search path handling in commencement.scm Ludovic Courtès
2020-02-04 13:00 ` [bug#39414] [PATCH 1/2] gnu: commencement: Avoid hard-coded GCC version numbers Ludovic Courtès
2020-02-04 13:00   ` [bug#39414] [PATCH 2/2] gnu: commencement: Rationalize search path handling Ludovic Courtès
2020-02-06  6:25     ` Jan Nieuwenhuizen
2020-02-06 14:22       ` Ludovic Courtès
2020-02-06 17:48         ` bug#39414: " Ludovic Courtès
2020-02-06  6:16   ` [bug#39414] [PATCH 1/2] gnu: commencement: Avoid hard-coded GCC version numbers Jan Nieuwenhuizen
2020-02-06  6:39 ` [bug#39414] [PATCH core-updates 0/2] Clarify search path handling in commencement.scm Jan Nieuwenhuizen
2020-02-06 14:25   ` Ludovic Courtès

Code repositories for project(s) associated with this public inbox

	https://git.savannah.gnu.org/cgit/guix.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).