Re: [PATCH v6 2/2] gnu: Add arduino-makefile.

[Ricardo Wurmus]

Hi Danny,

Thank you for your contributions!  I haven’t followed the discussion on
the previous versions, so I hope my comments below are not annoying.

> +(define-public arduino-makefile
> +  (package
> +    (name "arduino-makefile")
> +    (version "1.5.1")
> +    (source (origin
> +              (method url-fetch)
> +              (uri (string-append 
> "https://github.com/sudar/Arduino-Makefile/";
> +                                  "archive/" version ".tar.gz"))
> +              (sha256
> +               (base32
> +                "1gqmcg2jg62b915akbkivnqf8sx76gv719vx7azm47szd0w1i94i"))
> +              (file-name (string-append name "-" version ".tar.gz"))))
> +    (build-system python-build-system)
> +    (arguments
> +     `(#:tests? #f ; no tests exist
> +       #:phases
> +        (modify-phases %standard-phases
> +          (delete 'configure)
> +          (add-after 'unpack 'patch-paths
> +            (lambda* (#:key inputs outputs #:allow-other-keys)
> +             (let ((avr-gcc (assoc-ref inputs "avr-gcc-5"))
> +                   (avr-binutils (assoc-ref inputs "avr-binutils")))
> +              (substitute* "bin/ard-reset-arduino"
> +                (("#!/usr/bin/env python") "#!/usr/bin/python3"))

This looks unnecessary.  When “python-wrapper” is among the inputs the
shebang would be replaced automatically.

> +              (substitute* "Arduino.mk"
> +                (("#    => ARDUINO_DIR.*")
> +                   (string-append "ARDUINO_DIR = "
> +                                  (assoc-ref %build-inputs 
> "arduino-libraries")

Could you use “inputs” instead of “%build-inputs” here?

> +                                  "/share/arduino\n"))
> +                ; ; defaults to "hardware/tools/avr"

; ; –> ;;

> +                (("#    => AVR_TOOLS_DIR.*")
> +                   (string-append "AVR_TOOLS_DIR = /\n"
> +                                  "AVR_TOOLS_PATH = /\n"
> +                                  "AVRDUDE = " (assoc-ref %build-inputs 
> "avrdude") "\n"))
> +                (("CC_NAME[ ]*=.*")
> +                   (string-append "CC_NAME = " avr-gcc "/bin/avr-gcc\n"))
> +                (("CXX_NAME[ ]*=.*")
> +                   (string-append "CXX_NAME = " avr-gcc "/bin/avr-g++\n"))
> +                (("OBJCOPY_NAME[ ]*=.*")
> +                   (string-append "OBJCOPY_NAME = " avr-binutils 
> "/bin/avr-objcopy\n"))
> +                (("OBJDUMP_NAME[ ]*=.*")
> +                   (string-append "OBJDUMP_NAME = " avr-binutils 
> "/bin/avr-objdump\n"))
> +                (("AR_NAME[ ]*=.*")
> +                   (string-append "AR_NAME = " avr-binutils "/bin/avr-ar\n"))
> +                (("SIZE_NAME[ ]*=.*")
> +                   (string-append "SIZE_NAME = " avr-binutils 
> "/bin/avr-size\n"))
> +                (("NM_NAME[ ]*=.*")
> +                   (string-append "NM_NAME = " avr-binutils "/bin/avr-nm\n"))
> +                ; What about ld ?

What about it?  :)

> +                (("#    => ARDMK_DIR.*")
> +                   (string-append "ARDMK_DIR = "
> +                                  (assoc-ref %outputs "out")
> +                                  "/share/arduino\n"))))))

The indentation of this whole block looks a bit off.  Don’t worry about
it, though.  We can fix this before pushing the patch.

> +          (delete 'build)
> +          (replace 'install
> +            (lambda* (#:key outputs #:allow-other-keys)
> +              (let* ((out (assoc-ref outputs "out"))
> +                     (out-mk (string-append out "/share/arduino"))
> +                     (out-doc (string-append out "/share/doc"))
> +                     (out-bin (string-append out "/bin"))
> +                     (out-man (string-append out "/share/man/man1")))
> +                    (for-each (lambda (name)
> +                                (install-file name out-mk))
> +                              '("Arduino.mk" "arduino-mk-vars.md" 
> "chipKIT.mk" "Common.mk"))
> +                    (mkdir-p out-doc)
> +                    (copy-recursively "examples" out-doc)
> +                    (install-file "bin/ard-reset-arduino" out-bin)
> +                    (install-file "ard-reset-arduino.1" out-man)))))))

The phase should end with #t because “install-file” has no useful return value.

> +    (inputs
> +     `(("python" ,python)
> +       ("python-pyserial" ,python-pyserial)
> +       ("arduino-libraries" ,arduino-libraries)
> +       ("avrdude" ,avrdude)
> +       ("avr-gcc-5" ,avr-gcc-5)
> +       ("avr-binutils" ,avr-binutils)))

I’m a little confused about this.  Why is avrdude among the inputs?  Why
python-pyserial?  Nothing is built here.  You just copy the files.

If “bin/ard-reset-arduino” or any other script uses pyserial you will
probably have to wrap the executables such that they set the PYTHONPATH
to include the output of python-pyserial.  Otherwise they won’t be able
to find anything.

> +    (synopsis "Arduino Makefile Include file")

Please use lower case.

> +    (description "@code{arduino-makefile} allows you to build Arduino 
> sketches using a very tiny Makefile.")

Please break this line.  Is this really just a Makefile?  What else is
there?

> +    (home-page "https://github.com/sudar/Arduino-Makefile";)
> +    ;(supported-systems '("avr"))

This should be removed.

> +    (license license:lgpl2.1)))

Only version 2.1, or does it have the “or later” clause?

~~ Ricardo