Re: [PATCH v2 3/3] gnu: Add openocd.

[Theodoros Foradis <theodoros.for@openmailbox.org>]

Ricardo Wurmus writes:

> Theodoros Foradis <theodoros.for@openmailbox.org> writes:
>
>> +;; We build openocd from git, because the JTAG library libjaylink
>> +;; is not included in tarball releases.
>
> Is there a separate release of libjaylink?  Does the git version bundle
> libjaylink?  I’d prefer packaging proper releases of libjaylink and
> openocd, if that’s the case.
>

The git version does not bundle libjaylink. It is included in a
submodule. Should it be make clear in that initial comment?

>> +(define-public openocd
>> +  (let* ((commit "674141e8a7a6413cb803d90c2a20150260015f81")
>> +         (revision "1"))
>> +    (package
>> +      (name "openocd")
>> +      (version (string-append "0.9.0-" revision "."
>> +                              (string-take commit 7)))
>> +      (source (origin
>> +                (method git-fetch)
>> +                (uri (git-reference
>> +                      (url (string-append "git://git.code.sf.net/p/" name "/code.git"))
>> +                      (commit commit)
>> +                      (recursive? #t)))
>> +                (sha256
>> +                 (base32 "0p8rcqhkx3f29j08w33fkp8xnzj4xxa41lzdfq5wd1i4x8s07s0p"))
>> +                (file-name (string-append name "-" version "-checkout.tar.xz"))
>> +                (patches
>> +                 (search-patches "openocd-nrf52.patch"))))
>> +      (build-system gnu-build-system)
>> +      (arguments
>> +       '(#:configure-flags
>> +         (append (list "--disable-werror")
>> +                 (map (lambda (programmer)
>> +                        (string-append "--enable-" programmer))
>> +                      '("amtjtagaccel" "armjtagew" "buspirate" "ftdi"
>> +                        "gw16012" "jlink" "oocd_trace" "opendous" "osbdm"
>> +                        "parport" "aice" "cmsis-dap" "dummy" "jtag_vpi" "remote-bitbang"
>> +                        "rlink" "stlink" "ti-icdi" "ulink" "usbprog" "vsllink"
>> +                        "usb-blaster-2" "usb_blaster" "presto" "openjtag")))
>> +         #:phases
>> +         (modify-phases %standard-phases
>> +           (add-before 'configure 'autoreconf
>> +             (lambda _
>> +               (zero? (system* "autoreconf" "-vfi"))))
>> +           (add-after 'autoreconf 'patch-configure
>> +             (lambda _
>> +               (substitute* "configure"
>> +                 (("SHELL = /bin/sh") (string-append "SHELL = "
>> (which "sh"))))
>
> Is this really necessary?  Or can we just pass “SHELL” as a configure
> flag to override?
>

This is in fact not necessary, and removed.

>> +               (substitute* "configure"
>> +                 (("srcdir/src/jtag/drivers/libjaylink/configure.gnu")
>> +                  (string-append "echo -e '#!" (which "sh") "\nexec \"`dirname \"'\\$'0\"`
>> +/configure\" --enable-subproject-build \"'\\$'@\"' > \"
>> +$srcdir/src/jtag/drivers/libjaylink/configure.gnu\"")))
>
> This isn’t clear to me.  What happens here?  Why is the additional
> configure flag needed?  Could you add a comment as to the intent of this
> substitution?
>

This substitution was changed, to substitute with an empty string. The
actual intention of the substitution was to replace /bin/sh in the generated
configure.gnu, which proved to be unneeded, so it's just removed, and
the file will be run with the correct /bin/sh.

>> +               #t)))))
>> +      (inputs
>> +       `(("hidapi" ,hidapi)
>> +         ("libftdi" ,libftdi)
>> +         ("libusb" ,libusb)
>> +         ("libusb-compat" ,libusb-compat)))
>
> Both libusb AND libusb-compat?
>

Yes, it won't build without both.

>> +      (native-inputs
>> +       `(("autoconf" ,autoconf)
>> +         ("automake" ,automake)
>> +         ("libtool" ,libtool)
>> +         ("pkg-config" ,pkg-config)))
>> +      (home-page "http://openocd.org")
>> +      (synopsis "Open On-Chip Debugger")
>
> s/Open//
>
> I know that that’s what OpenOCD stands for, but everything is free
> software (or “open source”) in Guix anyway, so we usually don’t mention
> “free” or “open”.
>

Understood. I removed it.

>> +      (description
>> +       "OpenOCD provides on-chip programming and debugging support with a
>> +layered architecture of JTAG interface and TAP support.")
>> +      (license (list license:gpl2       ;; openocd and git2cl submodule
>> +                     license:gpl2+      ;; libjaylink submodule
>> +                     license:bsd-2))))) ;; jimctl submodule
>
> Please use a single “;” for margin comments like this.
>

Done.

>> diff --git a/gnu/packages/patches/openocd-nrf52.patch b/gnu/packages/patches/openocd-nrf52.patch
>> new file mode 100644
>> index 0000000..d136735
>> --- /dev/null
>> +++ b/gnu/packages/patches/openocd-nrf52.patch
>> @@ -0,0 +1,843 @@
>> +This patch adds support for nRF52 series devices. It is patchset 7 from
>> +http://openocd.zylin.com/#/c/3511/ , which has been tested, but not
>> +merged yet in master.
>
> Nitpick: please use two spaces between sentences and surround the URL in
> <…>, so that the space between URL and comma can be removed.
>
> ~~ Ricardo

Changed this as well. Thanks for the information, and excuse me for
ignoring those conventions.

I reply to this with the updated patches.

Theodoros Foradis (3):
      gnu: Add gdb-arm-none-eabi.
      gnu: Add hidapi.
      gnu: Add openocd.

 gnu/local.mk                             |   1 +
 gnu/packages/embedded.scm                |  77 ++++++++
 gnu/packages/libusb.scm                  |  38 ++++
 gnu/packages/patches/openocd-nrf52.patch | 843 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 959 insertions(+)

-- 
Theodoros Foradis