Andy Wingo writes:

>> +                           (system* "tar" "xf" mingw-source)
>> +                           (copy-file (string-append mingw-headers "/crt/_mingw.h.in")
>> +                                      (string-append mingw-headers "/crt/_mingw.h"))
>> +                           (substitute* (string-append mingw-headers "/crt/_mingw.h")
>> +                             (("@MINGW_HAS_SECURE_API@") "#define MINGW_HAS_SECURE_API 1"))
>
> What's this last bit about? You would think that GCC's configure was
> meant to handle this.  It could be the right thing but a comment is
> necessary.

I have added these comments

         ;; libc is false, so we are building xgcc-sans-libc
         ;; Add essential headers from mingw-w64.
         (let ((mingw-source (assoc-ref inputs "mingw-source"))
               (mingw-headers
                (string-append (getcwd) "/mingw-w64-v5.0-rc2/mingw-w64-headers")))
           (system* "tar" "xf" mingw-source)
           ;; We need _mingw.h which will gets built from
           ;; _mingw.h.in by mingw-w64's configure.  We cannot
           ;; configure mingw-w64 until we have
           ;; xgcc-sans-libc; substitute to the rescue.
           (copy-file (string-append mingw-headers "/crt/_mingw.h.in")
                      (string-append mingw-headers "/crt/_mingw.h"))
           (substitute* (string-append mingw-headers "/crt/_mingw.h")
             (("@MINGW_HAS_SECURE_API@") "#define MINGW_HAS_SECURE_API 1"))


>>  (define* (cross-gcc target
>> -                    #:optional (xbinutils (cross-binutils target)) libc)
>> +                    #:optional (xbinutils (cross-binutils target)) (libc #f))
>
> FWIW this change doesn't change anything -- if a default isn't given to
> an optional or keyword argument, the default is #f.  It is equally good
> both ways, so no feedback to you other than to make sure you know this
> is the case :)

Thanks!  Removed, masking hide my previous ignorance about this :)

>> -           (if libc
>> +           (cond
>> +            ((mingw-target? target)
>> +             (if libc
>> +                 `(("libc" ,mingw-w64)
>> +                   ,@inputs)
>> +                 `(("mingw-source" ,(package-source mingw-w64))
>> +                   ,@inputs)))
>> +            (libc
>>                 `(("libc" ,libc)
>
> Please fix indentation here.

Ok.  In my own code base I always just do indent-region on the whole
file, but that makes for lots of changes in Guix...

>> +  (cond
>> +   ((cross-newlib? target)
>> +    (cross-newlib? target))
>
> Also (cond (x x)) is the same as (cond (x)).

Ah, nice.  Changed now to (as a response to your remarks below)

    (cond
     ((cross-newlib? target)
      (native-libc target))

> But what's this about?  I thought this procedure should return a
> package, not a boolean.

...
>> +(define (native-libc target)
>> +  (if (mingw-target? target)
>> +      mingw-w64
>> +      glibc))
>> +
>> +(define (cross-newlib? target)
>> +  (and (not (eq? (native-libc target) glibc))
>> +       (native-libc target)))
>
> Aaaah I was confused because cross-newlib? was written as if it returned
> a boolean.  Please rename the function to, for example,
> `maybe-cross-newlib' or something.  Other names welcome but please,
> nothing ending in '?' :)

...Ah, okay... Changed to

    (define (native-libc target)
      (if (mingw-target? target)
          mingw-w64
          glibc))

    (define (cross-newlib? target)
      (not (eq? (native-libc target) glibc)))

I have adopted the meme in my own code to have any function? always
return the thing itself; that's also always true (as long as you don't
do things like (eq? (function?) #t).  That allows code like

   (or (foo?) bar)

Something I wanted to ask and I guess now why don't we let functions
like pair?, null?, string-prefix? not return the thing itself?

I guess doing that for new code is frowned up because it breaks the
principle of least surprise.

Thanks!
Greetings, Jan