Re: [PATCH v5 01/10] gnu: cross: Use CROSS_*_INCLUDE_PATH for system headers.

[Jan Nieuwenhuizen]

Andy Wingo writes:

> Since it seems you have one more round, a couple of nits :)

Thanks!

>> +   static const char *const lang_env_vars[] =
>> +-    { "C_INCLUDE_PATH", "CPLUS_INCLUDE_PATH",
>> +-      "OBJC_INCLUDE_PATH", "OBJCPLUS_INCLUDE_PATH" };
>> ++    { "CROSS_C_INCLUDE_PATH", "CROSS_CPLUS_INCLUDE_PATH",
>> ++      "CROSS_OBJC_INCLUDE_PATH", "CROSS_OBJCPLUS_INCLUDE_PATH" };
>> +   cpp_options *cpp_opts = cpp_get_options (pfile);
>> +   size_t idx = (cpp_opts->objc ? 2: 0);
>> + 
>
> I think this needs a comment somewhere -- you mean to completely replace
> C_INCLUDE_PATH et al with CROSS_C_INCLUDE_PATH et al?  If you could add
> a short rationale somewhere, either in a comment or in the patch
> summary, future hackers would really appreciate it :-)  I get a bit lost
> in these patches-to-patches.

Yes.  This is analogous to what the current patch does with CPATH, only
for C_*INCLUDE_PATH.  CROSS_CPATH, CROSS_C*_INCLUDE_PATH is to be used
for cross compilers, native compilers are unchanged, of course.  There
were two urls at the top of the patch, I changed that to

    Subject: [PATCH] Search path environment variables for cross-compilers.

    See the discussion at

        <http://gcc.gnu.org/ml/gcc/2013-02/msg00124.html>

    which lead to the previous iteration of this patch, introducing CROSS_CPATH,
    CROSS_LIBRARY_PATH and advocated the use of CPATH/CROSS_CPATH.

    As a concequence of the bug report

        http://bugs.gnu.org/22186

    usage of C_INCLUDE_PATH was re-introduced.  As noted in he discussion at

        <https://lists.gnu.org/archive/html/guix-devel/2016-04/msg00533.html>

    this introduces native headers in the search path and it was decided to keep
    using C_INCLUDE_PATH for system headers and introduce CROSS_C_INCLUDE_PATH 
et
    al., next to CROSS_CPATH to support cross compiling.

>> ---- 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="));
>> @@ -34,10 +62,11 @@ at <http://gcc.gnu.org/ml/gcc/2013-02/msg00124.html>.
>>   
>>     while ((f = file_pop ()) != NULL)
>>       {
>> -
>
> No change?

No; only a regenerated patch.  The only change is, as discussed in

    <https://lists.gnu.org/archive/html/guix-devel/2016-04/msg00533.html>

is adding CROSS_ variants of the C*_INCLUDE_PATH variables.

>> +-- 
>> +2.1.4
>> +
>> -- 
>> 2.7.3
>
> Likewise?  In any case you don't need two footers AFAIU.

The first footer is of the gcc-patch, the second is the one in the guix
patch.  To avoid confusion, I have stripped the footer away from the
gcc-patch.

>> diff --git a/gnu/packages/cross-base.scm b/gnu/packages/cross-base.scm
>> index c5bf66f..5d2d0fe 100644
>> --- a/gnu/packages/cross-base.scm
>> +++ b/gnu/packages/cross-base.scm
>> @@ -122,20 +128,34 @@ may be either a libc package or #f.)"
>>                                 "--disable-libquadmath"
>>                                 "--disable-decimal-float" ;would need libc
>>                                 "--disable-libcilkrts"
>> -                               )))
>> +                                ))
>> +
>> +                        ,@(if (cross-newlib? target)
>> +                              '("--with-newlib"
>> +                                "--without-threads"
>> +                                "--without-headers")
>> +                              '()))
>>  
>>                   ,(if libc
>>                        flags
>>                        `(remove (cut string-match "--enable-languages.*" <>)
>>                                 ,flags))))
>>         ((#:make-flags flags)
>> -        (if libc
>> +         (cond
>> +          ((mingw-target? target)
>
> One or two lines of rationale would be appreciated here :)

I have added, lik so
                       ;; For a newlib (non-glibc) target
                       ,@(if (cross-newlib? target)
                             '("--with-newlib"

the only thing I could explain about this snippet, and hardly needs any
explanation: the newlib flag.  I could not think of a good explanation
for the other changes, except maybe that my first attempt did not work
and I just pun in all flags that I new to work from the GUB cross build.

Removed all other changes above.

> Had to stop reviewing due to time.  Looking v good tho!

Thanks a lot again for your help.  Good questions!

Rebuilding, rebuilding etc; a v6 series will follow later.

Greetings,
Jan

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