Re: [PATCH 2/5] gnu: Add avr-gcc.

[Thompson, David]

On Thu, Apr 14, 2016 at 1:25 PM, Ludovic Courtès <address@hidden> wrote:
> David Thompson <address@hidden> skribis:
>
>> * gnu/packages/avr.scm (avr-gcc): New variable.
>
> [...]
>
>> +          `(modify-phases ,phases
>> +             ;; Without a working multilib build, the resulting GCC lacks
>> +             ;; support for nearly every AVR chip.
>> +             (add-after 'unpack 'fix-genmultilib
>> +               (lambda _
>> +                 (substitute* "gcc/genmultilib"
>> +                   (("#!/bin/sh") (string-append "#!" (which "sh"))))
>
> Just: (patch-shebang "gcc/genmultilib").
>
> I think the reason this file is not automatically patched during the
> ‘patch-shebangs’ phase is because it does not have the executable bit.
> Would be worth mentioning in a comment IMO.
>
> What’s unclear, though, is why the invalid shebang is a problem at all
> given that this file is not executable anyway.  Thoughts?

It turns out that gcc/genmultilib is a script that generates many
scripts, and thus has many instances of #!/bin/sh in it.  So,
patch-shebang was inadequate for the job and I've stuck with the
original solution.

>> +         ((#:configure-flags flags)
>> +          '(list "--target=avr"
>> +                 "--enable-languages=c,c++"
>> +                 "--disable-nls"
>> +                 "--disable-libssp"
>> +                 "--with-dwarf2"))))
>
> I think we should minimize target-specific changes and justify them in a
> comment when they’re unavoidable.
>
> Here, I think we can safely remove --target and --disable-nls.
> --disable-libssp and --enable-languages are already in
> ‘cross-gcc-arguments’, so that leaves us with just --with-dwarf2, IIUC.
>
> Why is it needed?

I've removed all of these.

>> +      (native-search-paths
>> +       (list (search-path-specification
>> +              (variable "CROSS_CPATH")
>> +              (files '("avr/include")))
>> +             (search-path-specification
>> +              (variable "CROSS_LIBRARY_PATH")
>> +              (files '("avr/lib"))))))))
>
> That these go in ‘native-search-paths’ feels wrong.
>
> But I think it’s because we’re trying to build avr-libc like a “normal”
> package with a cross-toolchain as its input.
>
> Instead, the “intended use” is that libc is treated specially as in
> ‘cross-libc’ in cross-base.scm.  Probably we need to:
>
>   1. Remove #:configure-flags and ‘native-inputs’ from the ‘avr-libc’
>      package.
>
>   2. Add (supported-systems '()) or similar to the ‘avr-libc’ package.
>
>   3. Use something similar to ‘cross-libc’ but that uses avr-libc
>      instead of glibc in cross-base.scm, thus setting CROSS_CPATH and
>      CROSS_LIBRARY_PATH appropriately.

As explained in the thread about the avr-toolchain, this is a special
scenario because the only use for avr-gcc is to cross-compile and you
cannot actually port any part of Guix to the AVR architecture.  Maybe
there's still something to revisit later, but having a working AVR
toolchain is a big win for now.

Pushed with the other issues addressed.  Thanks!

- Dave