On 2018-06-25, Marius Bakke wrote:
> Vagrant Cascadian <vagrant@debian.org> writes:
>
>> Add new package definition for python-libusb1, needed to upgrade
>> python-trezor to newer versions.
>
> Thanks!  I have some minor nitpicks:
>
> [...]
>
>> * gnu/packages/libusb.scm (python-libusb1): New variable.
>
> Can you also add yourself to the copyright header in this file?

Sure.


>> +(define-public python-libusb1
>> +  (package
>> +    (name "python-libusb1")
>> +    (version "1.6.4")
>> +    (source
>> +     (origin
>> +       (method url-fetch)
>> +       (uri (pypi-uri "libusb1" version))
>> +       (sha256
>> +        (base32
>> +         "03b7xrz8vqg8w0za5r503jhcmbd1ls5610jcja1rqz833nf0v4wc"))))
>> +    (build-system python-build-system)
>> +    (arguments
>> +     `(#:modules ((srfi srfi-1)
>> +                  (srfi srfi-26)
>
> Is srfi-26 actually used here?

No idea, I admittedly cargo-culted this from python-pyusb...
fix-libusb-reference didn't work until I added the modules block, and I
didn't narrow-down which of the specific modules were actually needed.


>> +                  (guix build utils)
>> +                  (guix build python-build-system))
>> +       #:phases
>> +       (modify-phases %standard-phases
>> +         (add-after 'unpack 'fix-libusb-reference
>> +           (lambda* (#:key inputs #:allow-other-keys)
>> +             (substitute* "usb1/libusb1.py"
>> +               (("libusb_path = ctypes.util.find_library\\(base_name\\)")
>> +                (string-append
>> +                 "libusb_path = \""
>> +                 (find (negate symbolic-link?)
>> +                       (find-files (assoc-ref inputs "libusb")
>> +                                   "^libusb.*\\.so\\..*"))
>> +                 "\"")))
>> +             #t)))))
>> +    (inputs `(("libusb" ,libusb)))
>> +    (home-page
>> +     "http://github.com/vpelletier/python-libusb1")
>
> This line break is unnecessary :-)

Ok.


>> +    (synopsis "Pure-python wrapper for libusb-1.0")
>> +    (description
>> +     "Pure-python wrapper for libusb-1.0")
>
> This description is rather thin.  Can you try to make it into one or
> more full sentences?

Will try.


>> +    (license lgpl2.1+)))
>
> I noticed the source contains GPL2 in COPYING, and LGPL2.1 as
> "COPYING.LESSER".  Can you make sure we don't install the former?

I was a bit confused by that, but looking at all the actual code, it
only appears to be LGPL2.1. So you're saying to explicitly remove
COPYING from the package?


> Can you send an updated patch?  Thanks in advance!

Sure. Thanks for the review!


live well,
  vagrant