diff mbox

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

Message ID 20160830190155.11668-3-dannym@scratchpost.org
State New
Headers show

Commit Message

Danny Milosavljevic Aug. 30, 2016, 7:01 p.m. UTC
* gnu/packages/arduino.scm (arduino-makefile): New variable.
---
 gnu/packages/arduino.scm | 81 ++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 81 insertions(+)

Comments

Ricardo Wurmus Aug. 31, 2016, 8:02 p.m. UTC | #1
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
Danny Milosavljevic Aug. 31, 2016, 8:34 p.m. UTC | #2
Hi Ricardo,

On Wed, 31 Aug 2016 22:02:55 +0200
Ricardo Wurmus <rekado@elephly.net> wrote:
> > +             (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.

It's impossible for anyone to reliably detect the major Python version that
the script expects.

Therefore, this clarifies that we need Python3 for it.
The shebang will then be automatically updated correctly.

> > +              (substitute* "Arduino.mk"
> > +                (("#    => ARDUINO_DIR.*")
> > +                   (string-append "ARDUINO_DIR = "
> > +                                  (assoc-ref %build-inputs "arduino-libraries")  
> 
> Could you use “inputs” instead of “%build-inputs” here?

Yes. What's the difference?

> > +                ; What about ld ?  
> 
> What about it?  :)

I don't know. Seems to work without it but it just irks me.
There's no variable for ld's name in the makefile.

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

It's supposed to be something like an arduino-toolchain. You just install it
and it will pull the compiler, flasher etc and use it for your projects
automagically as long as they "include Arduino.mk".

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

Probably.

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

Yeah, huge makefile. Allows you to build and flash the stuff.

> > +    ;(supported-systems '("avr"))  
> 
> This should be removed.

As I said I'm not a fan of obfuscating what system this is for.
It is *good* for maintenance to have little hints of what to expect.

If it worked usefully, I'd even make the (supported-system '("avr")) active!

> > +    (license license:lgpl2.1)))  
> 
> Only version 2.1, or does it have the “or later” clause?

lgpl2.1+

Thanks,
    Danny
Ricardo Wurmus Sept. 1, 2016, 2:57 p.m. UTC | #3
Danny Milosavljevic <dannym@scratchpost.org> writes:

> Hi Ricardo,
>
> On Wed, 31 Aug 2016 22:02:55 +0200
> Ricardo Wurmus <rekado@elephly.net> wrote:
>> > +             (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.
>
> It's impossible for anyone to reliably detect the major Python version that
> the script expects.
>
> Therefore, this clarifies that we need Python3 for it.
> The shebang will then be automatically updated correctly.

I don’t understand this.  When the “python-wrapper” package is an input
it will be the only package providing a “python” executable, so the
shebang patcher will replace “#!/usr/bin/env python” with
“#!/gnu/store/…-python-3…/bin/python”.  There’s nothing ambiguous about
this.

>> > +              (substitute* "Arduino.mk"
>> > +                (("#    => ARDUINO_DIR.*")
>> > +                   (string-append "ARDUINO_DIR = "
>> > +                                  (assoc-ref %build-inputs "arduino-libraries")  
>> 
>> Could you use “inputs” instead of “%build-inputs” here?
>
> Yes. What's the difference?

We prefer to use a properly bound “inputs” over the magical
“%build-inputs”.  “%build-inputs” can be used in the specification of
#:make-flags where “inputs” cannot be bound.

>> > +                ; What about ld ?  
>> 
>> What about it?  :)
>
> I don't know. Seems to work without it but it just irks me.
> There's no variable for ld's name in the makefile.
>
>> 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.
>
> It's supposed to be something like an arduino-toolchain. You just install it
> and it will pull the compiler, flasher etc and use it for your projects
> automagically as long as they "include Arduino.mk".

Then shouldn’t the inputs be propagated?  As the build artifacts of this
package retain no reference to these regular inputs it’s almost the same
as not specifying them in the first place.

>> > +    ;(supported-systems '("avr"))  
>> 
>> This should be removed.
>
> As I said I'm not a fan of obfuscating what system this is for.
> It is *good* for maintenance to have little hints of what to expect.
>
> If it worked usefully, I'd even make the (supported-system '("avr")) active!

This is not what “supported-systems” is used for.  This field is used to
tell the build machines in the build farm not to bother building
packages that are not supported on the current architecture.

The software provided by this package does not “run” on AVRs.  (It
doesn’t run at all.)  It is used on any host system to build software
for AVRs.  That’s very different from what the “supported-systems” field
describes.

~~ Ricardo
Danny Milosavljevic Sept. 1, 2016, 6:44 p.m. UTC | #4
Hi Ricardo,

On Thu, 01 Sep 2016 16:57:32 +0200
Ricardo Wurmus <rekado@elephly.net> wrote:

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

... with Python 3's name. So okay, I'll remove this part. (I still don't get what Guix does (not do) with major versions - but fine)

ard-reset-arduino source code should have specified which Python version it expects in the shebang to begin with, I was just bugfixing it... 

> “#!/gnu/store/…-python-3…/bin/python”.  There’s nothing ambiguous about
                         ^ yeah, I've checked out python-wrapper now and as long as it does that now and in the future, okay :)

> We prefer to use a properly bound “inputs” over the magical
> “%build-inputs”.  “%build-inputs” can be used in the specification of
> #:make-flags where “inputs” cannot be bound.

Ok.

> Then shouldn’t the inputs be propagated?  As the build artifacts of this
> package retain no reference to these regular inputs it’s almost the same
> as not specifying them in the first place.

I don't know about this. I thought propagated meant that if I installed arduino-makefile into the profile it would also install avrdude into the profile. That is not what I wanted. The Makefile refers to the /gnu/store/....avrdude... - that's it. But we can also propagate - I'm just not sure whether that's needed and useful.

> This is not what “supported-systems” is used for.  This field is used to
> tell the build machines in the build farm not to bother building
> packages that are not supported on the current architecture.

Ah okay. I'll remove it then.

Cheers,
   Danny
Danny Milosavljevic Sept. 1, 2016, 6:52 p.m. UTC | #5
On Thu, 1 Sep 2016 20:44:20 +0200
Danny Milosavljevic <dannym@scratchpost.org> wrote:

> I don't know about this. I thought propagated meant that if I installed arduino-makefile into the profile it would also install avrdude into the profile. That is not what I wanted. The Makefile refers to the /gnu/store/....avrdude... - that's it. But we can also propagate - I'm just not sure whether that's needed and useful.

On second thought, gcc-toolchain propagates ld etc too. So if you want, we can propagate avr-gcc-5, avr-binutils, arduino-libraries and avrdude.

Propagating Python would probably be technically necessary but actually not desireable because Arduino has very little to do with Python.

So I'm fine with it either way.
diff mbox

Patch

diff --git a/gnu/packages/arduino.scm b/gnu/packages/arduino.scm
index 5309587..08ed1cf 100644
--- a/gnu/packages/arduino.scm
+++ b/gnu/packages/arduino.scm
@@ -89,3 +89,84 @@  hardware spec files (boards.txt).")
                    license:asl2.0
                    license:expat
                    license:bsd-3))))
+
+(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"))
+              (substitute* "Arduino.mk"
+                (("#    => ARDUINO_DIR.*")
+                   (string-append "ARDUINO_DIR = "
+                                  (assoc-ref %build-inputs "arduino-libraries")
+                                  "/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 ?
+                (("#    => ARDMK_DIR.*")
+                   (string-append "ARDMK_DIR = "
+                                  (assoc-ref %outputs "out")
+                                  "/share/arduino\n"))))))
+          (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)))))))
+    (inputs
+     `(("python" ,python)
+       ("python-pyserial" ,python-pyserial)
+       ("arduino-libraries" ,arduino-libraries)
+       ("avrdude" ,avrdude)
+       ("avr-gcc-5" ,avr-gcc-5)
+       ("avr-binutils" ,avr-binutils)))
+    (synopsis "Arduino Makefile Include file")
+    (description "@code{arduino-makefile} allows you to build Arduino sketches using a very tiny Makefile.")
+    (home-page "https://github.com/sudar/Arduino-Makefile")
+    ;(supported-systems '("avr"))
+    (license license:lgpl2.1)))