Re: [PATCH v4 1/9] gnu: cross: Use CROSS_*_INCLUDE_PATH for system headers.

[Andy Wingo <wingo@igalia.com>]

Hi!

Just one reply to the summary then review of the patches.

On Mon 25 Apr 2016 13:28, Jan Nieuwenhuizen <janneke@gnu.org> writes:

> Yes... Why do you not prefer the and-let*, how would you write it?

Well, you had two versions there, and the second one that just used let*
looked fine to me.  Dunno.  I find that the structure imposed by
and-let* tends to contort things in a way I don't want -- sometimes it's
only some of the bindings that I want to test for truthiness.  I find
myself fighting and-let* more than enjoying it.  Anyway.  It is a very
minor point and I don't think it matters here :)

> From f2842ce5e35c0d984fc18912088bb81f9bac38f5 Mon Sep 17 00:00:00 2001
> From: Jan Nieuwenhuizen <janneke@gnu.org>
> Date: Sun, 17 Apr 2016 18:20:05 +0200
> Subject: [PATCH 1/9] gnu: cross: Use CROSS_*_INCLUDE_PATH for system headers.
>
> * gnu/packages/patches/gcc-cross-environment-variables.patch: Also use CROSS_
>   variants: CROSS_C_INCLUDE_PATH, CROSS_CPLUS_INCLUDE_PATH,
>   CROSS_OBJC_INCLUDE_PATH, CROSS_OBJCPLUS_INCLUDE_PATH to be used for system
>   libraries, see
>   https://lists.gnu.org/archive/html/guix-devel/2016-04/msg00620.html.
> * gnu/packages/cross-base.scm (cross-gcc, cross-gcc-arguments, cross-libc):
>   Use CROSS_*_INCLUDE_PATH (WAS: CPATH).
> ---
>  gnu/packages/cross-base.scm                        | 70 +++++++++++-------
>  .../patches/gcc-cross-environment-variables.patch  | 86 +++++++++++++++-------
>  2 files changed, 104 insertions(+), 52 deletions(-)
>
> diff --git a/gnu/packages/cross-base.scm b/gnu/packages/cross-base.scm
> index 8bd599c..2e1bcf8 100644
> --- a/gnu/packages/cross-base.scm
> +++ b/gnu/packages/cross-base.scm
> @@ -168,34 +170,38 @@ may be either a libc package or #f.)"
>                  (lambda* (#:key inputs #:allow-other-keys)
>                    ;; Add the cross Linux headers to CROSS_CPATH, and remove them
>                    ;; from CPATH.
> -                  (let ((libc  (assoc-ref inputs "libc"))
> +                  (let ((libc (assoc-ref inputs libc))
>                          (linux (assoc-ref inputs "xlinux-headers")))

FYI while I usually don't vertically line up subexpressions, it would
appear to be the guix style ;)  So this edit undoes some valid code.  I
don't think it matters though.

> diff --git a/gnu/packages/patches/gcc-cross-environment-variables.patch b/gnu/packages/patches/gcc-cross-environment-variables.patch
> index 0bd0be5..a2b94cb 100644
> --- a/gnu/packages/patches/gcc-cross-environment-variables.patch
> +++ b/gnu/packages/patches/gcc-cross-environment-variables.patch
> @@ -1,9 +1,48 @@

OK so this is literally a patch in a patch and it gets complicated to
review :)  But cool, I had a question about one piece:

> +diff --git a/gcc/gcc.c b/gcc/gcc.c
> +index adbf0c4..70448c6 100644
> +--- a/gcc/gcc.c
> ++++ b/gcc/gcc.c
> +@@ -3853,7 +3853,7 @@ process_command (unsigned int decoded_options_count,
> +     }
> + 
> +   temp = getenv (LIBRARY_PATH_ENV);
> +-  if (temp && *cross_compile == '0')
> ++  if (temp)
> +     {
> +       const char *startp, *endp;
> +       char *nstore = (char *) alloca (strlen (temp) + 3);

Why this change?

> ---- gcc-4.7.2/gcc/system.h	2012-02-17 00:16:28.000000000 +0100
> -+++ gcc-4.7.2/gcc/system.h	2013-02-12 10:22:17.000000000 +0100
> -@@ -1023,4 +1023,6 @@ helper_const_non_const_cast (const char
> - #define DEBUG_VARIABLE
> - #endif
> +diff --git a/gcc/system.h b/gcc/system.h
> +index 42bc509..af3b9ad 100644
> +--- a/gcc/system.h
> ++++ b/gcc/system.h
> +@@ -1063,4 +1063,6 @@ helper_const_non_const_cast (const char *p)
> + /* Get definitions of HOST_WIDE_INT and HOST_WIDEST_INT.  */
> + #include "hwint.h"
>   
>  +#define LIBRARY_PATH_ENV "CROSS_LIBRARY_PATH"
>  +
>   #endif /* ! GCC_SYSTEM_H */
> -

I wasn't quite able to understand this bit.

> ---- gcc-4.7.2/gcc/tlink.c	2012-02-11 09:50:23.000000000 +0100
> -+++ gcc-4.7.2/gcc/tlink.c	2013-05-23 22:06:19.000000000 +0200
> -@@ -461,7 +461,7 @@ recompile_files (void)
> +diff --git a/gcc/tlink.c b/gcc/tlink.c
> +index bc358b8..ad6242f 100644
> +--- a/gcc/tlink.c
> ++++ b/gcc/tlink.c
> +@@ -458,7 +458,7 @@ recompile_files (void)
>     file *f;
>   
>     putenv (xstrdup ("COMPILER_PATH="));
>  -  putenv (xstrdup ("LIBRARY_PATH="));
> -+  putenv (xstrdup (LIBRARY_PATH_ENV "="));
> ++  putenv (xstrdup ("LIBRARY_PATH_ENV="));
>   
>     while ((f = file_pop ()) != NULL)
>       {

Surely this is incorrect?

> ---- gcc-4.7.3/gcc/gcc.c	2013-03-08 08:25:09.000000000 +0100
> -+++ gcc-4.7.3/gcc/gcc.c	2013-05-24 08:58:16.000000000 +0200
> -@@ -3726,7 +3726,7 @@ process_command (unsigned int decoded_op
> -     }
> - 
> -   temp = getenv (LIBRARY_PATH_ENV);
> --  if (temp && *cross_compile == '0')
> -+  if (temp)
> -     {
> -       const char *startp, *endp;
> -       char *nstore = (char *) alloca (strlen (temp) + 3);
> -- 
> 2.7.3

Similar comment as above.

The Scheme parts LGTM; would you mind giving details about the GCC
patch?

Cheers,

Andy