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

[Jan Nieuwenhuizen]

Andy Wingo writes:

> 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 :)

Okay.

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

There's something wrong here that I keep correcting, I'll have a look.
I think it should be "libc" ... I have wondered about a patch where I
change libc to "libc" -- how did that ever work?  Apparently, I broke it
here.

> 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:

What I did was import gcc-4.9.3 into git, applied the original
gcc-cross-environment-variables.patch from Guix, made some changes and
used git to produce a new gcc-cross-environment-variables.patch.

I'm not too happy with this, maybe I should look into making the guix
patch-in-patch nicer?  Anyway, here inline is the actual diff between
the original gcc-cross-environment-variables.patch and the new one:

diff --git a/gcc/incpath.c b/gcc/incpath.c
index 3a34998..ba12249 100644
--- a/gcc/incpath.c
+++ b/gcc/incpath.c
@@ -461,8 +461,8 @@ register_include_chains (cpp_reader *pfile, const char 
*sysroot,
                         int stdinc, int cxx_stdinc, int verbose)
 {
   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);
 
diff --git a/gcc/tlink.c b/gcc/tlink.c
index 65d4a84..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_ENV "="));
+  putenv (xstrdup ("LIBRARY_PATH_ENV="));
 
   while ((f = file_pop ()) != NULL)
     {

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

You spotted the putenv problem, quite visible above; I'm not sure why
this still works for me, I'll fix (i.e., revert) this.

As to your other two questions about if (temp): those are actually not
changes wrt the current gcc-cross-environment-variables.patch.

>> +-  if (temp && *cross_compile == '0')
>> ++  if (temp)
>> +     {
>> +       const char *startp, *endp;
>> +       char *nstore = (char *) alloca (strlen (temp) + 3);
>
> Why this change?

Not a change per se, AIUI our cross patch makes COMPILER_PATH now
valid, as we populate it from CROSS_*.

>>  +#define LIBRARY_PATH_ENV "CROSS_LIBRARY_PATH"
>>  +
>>   #endif /* ! GCC_SYSTEM_H */
>> -
>
> I wasn't quite able to understand this bit.

>>  -  putenv (xstrdup ("LIBRARY_PATH="));
>> -+  putenv (xstrdup (LIBRARY_PATH_ENV "="));
>> ++  putenv (xstrdup ("LIBRARY_PATH_ENV="));
>>   
>>     while ((f = file_pop ()) != NULL)
>>       {
>
> Surely this is incorrect?

Oops yes, this is wrong.  Thanks

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

Not a change per se, AIUI our cross patch makes LIBRARY_PATH[_ENV] now
valid, as we populate it from CROSS_LIBRARY_PATH.

Thanks again, greetings,
Jan

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