Bug 1323966 - Review Request: micropython - Implementation of Python 3 with very low memory footprint
Summary: Review Request: micropython - Implementation of Python 3 with very low memory...
Keywords:
Status: CLOSED RAWHIDE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Mairi Dulaney
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
: 1113915 (view as bug list)
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2016-04-05 08:22 UTC by Mairi Dulaney
Modified: 2016-04-13 18:49 UTC (History)
5 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2016-04-13 18:49:32 UTC
Type: ---
ncoghlan: fedora-review+


Attachments (Terms of Use)

Description Mairi Dulaney 2016-04-05 08:22:49 UTC
Spec URL: http://rpms.jdulaney.com/review/micropython.spec
SRPM URL: http://rpms.jdulaney.com/review/micropython-1.6-1.fc24.src.rpm
Description:  Implementation of Python 3 with very low memory footprint
Fedora Account System Username:  jdulaney

Comment 1 Mairi Dulaney 2016-04-05 08:31:22 UTC
I note that I disable debuginfo excraction as the build process itself does not generate it.  I will work with upstream on this.

Comment 2 Nick Coghlan 2016-04-05 11:03:01 UTC
Unfortunately, I found a potentially "fun" licensing problem with the USB code in stmhal/usbhost: https://github.com/micropython/micropython/issues/26

Specifically, the code in that directory is under http://www.st.com/web/en/resource/legal/legal_agreement/license_agreement/ultimate-liberty-v2.txt?sc=software_license_agreement_liberty_v2 which isn't an open source license

The notice of that license in the source tarball is in stmhal/usbhost/Release_Notes.html

The other slightly dubious piece in the embedded system parts of the tarball is what looks to be a pre-built binary at cc3200/bootmanager/relocator/relocator.bin

The MicroPython-for-Unix build doesn't *use* any of those pieces, but I'm not sure of the potential implications of having them in the SRPM. Should this BZ be set to block FE-Legal to ask them the question?

A couple of other items worth taking a second look at:

- fedora-review complained about the LICENSE file being marked as %doc instead of %license

- there's a micropython-upip tarball embedded in the tools directory which I haven't looked inside yet


(I'm still working through the rest of the review checklist, but figured it made sense to raise these items immediately rather than waiting)

Comment 3 Nick Coghlan 2016-04-05 11:05:53 UTC
fedora-review & rpmlint both complained about not being able to retrieve the upstream source tarball, but I believe that's a false alarm caused by GitHub's use of 302 redirects to locate the actual tarball: https://fedorahosted.org/FedoraReview/ticket/270

Comment 4 Zbigniew Jędrzejewski-Szmek 2016-04-05 14:33:23 UTC
*** Bug 1113915 has been marked as a duplicate of this bug. ***

Comment 5 Zbigniew Jędrzejewski-Szmek 2016-04-05 14:46:25 UTC
> The MicroPython-for-Unix build doesn't *use* any of those pieces, but I'm not 
> sure of the potential implications of having them in the SRPM. Should this BZ 
> be set to block FE-Legal to ask them the question?

My reading of the license says that it is fine to have the file in SRPM: the license allows free copying, but limits *use* ("only on our crappy hardware"). As long as that file or anything built from it is not included in the binary rpms, it is not possible to violate the license.
To make sure that it is not used, remove the file in %prep.

> relocator.bin

https://github.com/micropython/micropython/commit/f8146d021ba933034e7a6244e4267a6626357cf7

I think we can treat this as "firmware" [https://fedoraproject.org/wiki/Licensing:Main#Binary_Firmware], and distribute it. It must have the right license though.

Comment 6 Mairi Dulaney 2016-04-05 15:24:06 UTC
(In reply to Nick Coghlan from comment #2)
> Unfortunately, I found a potentially "fun" licensing problem with the USB
> code in stmhal/usbhost: https://github.com/micropython/micropython/issues/26
> 
> Specifically, the code in that directory is under
> http://www.st.com/web/en/resource/legal/legal_agreement/license_agreement/
> ultimate-liberty-v2.txt?sc=software_license_agreement_liberty_v2 which isn't
> an open source license
> 
> The notice of that license in the source tarball is in
> stmhal/usbhost/Release_Notes.html
> 
> The other slightly dubious piece in the embedded system parts of the tarball
> is what looks to be a pre-built binary at
> cc3200/bootmanager/relocator/relocator.bin
> 
> The MicroPython-for-Unix build doesn't *use* any of those pieces, but I'm
> not sure of the potential implications of having them in the SRPM. Should
> this BZ be set to block FE-Legal to ask them the question?



I went ahead and did so, better to be certain.


> 
> A couple of other items worth taking a second look at:
> 
> - fedora-review complained about the LICENSE file being marked as %doc
> instead of %license
> 


Oops, I will fix that.  I had grabbed the spec from the previous review.


> - there's a micropython-upip tarball embedded in the tools directory which I
> haven't looked inside yet
> 
> 
> (I'm still working through the rest of the review checklist, but figured it
> made sense to raise these items immediately rather than waiting)



(In reply to Zbigniew Jędrzejewski-Szmek from comment #5)
> > The MicroPython-for-Unix build doesn't *use* any of those pieces, but I'm not 
> > sure of the potential implications of having them in the SRPM. Should this BZ 
> > be set to block FE-Legal to ask them the question?
> 
> My reading of the license says that it is fine to have the file in SRPM: the
> license allows free copying, but limits *use* ("only on our crappy
> hardware"). As long as that file or anything built from it is not included
> in the binary rpms, it is not possible to violate the license.
> To make sure that it is not used, remove the file in %prep.
> 
> > relocator.bin
> 
> https://github.com/micropython/micropython/commit/
> f8146d021ba933034e7a6244e4267a6626357cf7
> 
> I think we can treat this as "firmware"
> [https://fedoraproject.org/wiki/Licensing:Main#Binary_Firmware], and
> distribute it. It must have the right license though.


I will ask upstream about the nature of this, though it doesn't actually get included in the 'Unix' build.  If need be, I can remove it via patch.

Comment 7 Mairi Dulaney 2016-04-05 18:58:46 UTC
I asked upstream about the binary:

https://github.com/micropython/micropython/issues/1968

I received the following response:

> Hi,
>
>  It's part of the CC3200 SDK supplied by TI. It is licensed the same way
> as the other files from Texas Instruments. The source code is also
> available in the SDK, I just used the .bin for convenience...
>
> Cheers, Daniel


Looking around, it looks like an MIT-ish license.

However, it appears to not be included in the build on Fedora, and to be safe, I added a line to the spec file to rm it.


I also went ahead and fixed the %license.

Comment 9 Tom "spot" Callaway 2016-04-05 19:21:34 UTC
I concur with Zbigniew in comment 5. Just delete those files in %prep. Lifting FE-Legal.

Comment 10 Mairi Dulaney 2016-04-05 20:08:18 UTC
Okay, went ahead and removed that entire directory just to be sure (rather than pick and choose the non-freely licensed files)

Did a scratchbuild that may be found here:

http://koji.fedoraproject.org/koji/taskinfo?taskID=13567775

Comment 12 Nick Coghlan 2016-04-13 02:20:09 UTC
Thanks folks! I'll finish up the rest of the review today and post the full checklist.

Comment 13 Nick Coghlan 2016-04-13 03:55:53 UTC
Package looks good to me - there are a couple of items in the SHOULD section which I haven't checked (e.g. multi-arch support), or which are definitely missing (parallel build support, non-English translations)

I also wasn't sure if/how the "-debuginfo" item applied to a binary like MicroPython, so I flagged it as not evaluated, rather than not applicable.

Package Review
==============

Legend:
[x] = Pass, [!] = Fail, [-] = Not applicable, [?] = Not evaluated
[ ] = Manual review needed

===== MUST items =====

C/C++:
[x]: Package does not contain kernel modules.
[x]: Package contains no static executables.
[x]: Package does not contain any libtool archives (.la)
[x]: Rpath absent or only used for internal libs.

Generic:
[x]: Package is licensed with an open-source compatible license and meets
     other legal requirements as defined in the legal section of Packaging
     Guidelines.
[x]: License field in the package spec file matches the actual license.
[x]: %build honors applicable compiler flags or justifies otherwise.
[x]: Package contains no bundled libraries without FPC exception.
[x]: Changelog in prescribed format.
[x]: Sources contain only permissible code or content.
[-]: Package contains desktop file if it is a GUI application.
[x]: Development files must be in a -devel package
[x]: Package uses nothing in %doc for runtime.
[x]: Package consistently uses macros (instead of hard-coded directory
     names).
[x]: Package is named according to the Package Naming Guidelines.
[x]: Package does not generate any conflict.
[x]: Package obeys FHS, except libexecdir and /usr/target.
[-]: If the package is a rename of another package, proper Obsoletes and
     Provides are present.
[x]: Requires correct, justified where necessary.
[x]: Spec file is legible and written in American English.
[-]: Package contains systemd file(s) if in need.
[?]: Useful -debuginfo package or justification otherwise.
[x]: Package is not known to require an ExcludeArch tag.
[-]: Large documentation must go in a -doc subpackage. Large could be size
     (~1MB) or number of files.
     Note: Documentation size is 10240 bytes in 1 files.
[x: Package complies to the Packaging Guidelines
[x]: Package successfully compiles and builds into binary rpms on at least
     one supported primary architecture.
[x]: Package installs properly.
[x]: Rpmlint is run on all rpms the build produces.
     Note: There are rpmlint messages (see attachment).
[x]: If (and only if) the source package includes the text of the
     license(s) in its own file, then that file, containing the text of the
     license(s) for the package is included in %license.
[x]: Package requires other packages for directories it uses.
[x]: Package must own all directories that it creates.
[x]: Package does not own files or directories owned by other packages.
[x]: All build dependencies are listed in BuildRequires, except for any
     that are listed in the exceptions section of Packaging Guidelines.
[x]: Package uses either %{buildroot} or $RPM_BUILD_ROOT
[x]: Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the
     beginning of %install.
[x]: Macros in Summary, %description expandable at SRPM build time.
[x]: Dist tag is present.
[x]: Package does not contain duplicates in %files.
[x]: Permissions on files are set properly.
[x]: Package use %makeinstall only when make install DESTDIR=... doesn't
     work.
[x]: Package is named using only allowed ASCII characters.
[x]: Package does not use a name that already exists.
[x]: Package is not relocatable.
[x]: Sources used to build the package match the upstream source, as
     provided in the spec URL.
[x]: Spec file name must match the spec package %{name}, in the format
     %{name}.spec.
[x]: File names are valid UTF-8.
[x]: Packages must not store files under /srv, /opt or /usr/local

===== SHOULD items =====

Generic:
[!]: Uses parallel make %{?_smp_mflags} macro.
[-]: If the source package does not include license text(s) as a separate
     file from upstream, the packager SHOULD query upstream to include it.
[x]: Final provides and requires are sane (see attachments).
[x]: Package functions as described.
[x]: Latest version is packaged.
[x]: Package does not include license text files separate from upstream.
[!]: Description and summary sections in the package spec file contains
     translations for supported Non-English languages, if available.
[?]: Package should compile and build into binary rpms on all supported
     architectures.
[x]: %check is present and all tests pass.
[?]: Packages should try to preserve timestamps of original installed
     files.
[x]: Packager, Vendor, PreReq, Copyright tags should not be in spec file
[x]: Sources can be downloaded from URI in Source: tag
[x]: Reviewer should test that the package builds in mock.
[x]: Buildroot is not present
[x]: Package has no %clean section with rm -rf %{buildroot} (or
     $RPM_BUILD_ROOT)
[x]: No file requires outside of /etc, /bin, /sbin, /usr/bin, /usr/sbin.
[x]: SourceX is a working URL.
[x]: Spec use %global instead of %define unless justified.

===== EXTRA items =====

Generic:
[x]: Rpmlint is run on all installed packages.
     Note: There are rpmlint messages (see attachment).
[x]: Large data in /usr/share should live in a noarch subpackage if package
     is arched.


Rpmlint
-------
Checking: micropython-1.6-3.fc25.x86_64.rpm
          micropython-1.6-3.fc25.src.rpm
micropython.x86_64: W: no-manual-page-for-binary micropython
2 packages and 0 specfiles checked; 0 errors, 1 warnings.




Rpmlint (installed packages)
----------------------------
sh: /usr/bin/python: No such file or directory
micropython.x86_64: W: no-manual-page-for-binary micropython
1 packages and 0 specfiles checked; 0 errors, 1 warnings.



Requires
--------
micropython (rpmlib, GLIBC filtered):
    libc.so.6()(64bit)
    libdl.so.2()(64bit)
    libffi.so.6()(64bit)
    libm.so.6()(64bit)
    rtld(GNU_HASH)



Provides
--------
micropython:
    micropython
    micropython(x86-64)



Source checksums
----------------
https://github.com/micropython/micropython/archive/v1.6.tar.gz :
  CHECKSUM(SHA256) this package     : f8ea3faffa797de1a06c16c8f57c784c665b318ca08ac17f74dcbc95322d47de
  CHECKSUM(SHA256) upstream package : f8ea3faffa797de1a06c16c8f57c784c665b318ca08ac17f74dcbc95322d47de


Generated by fedora-review 0.6.0 (3c5c9d7) last change: 2015-05-20
Command line :/usr/bin/fedora-review --rpm-spec -b 1323966
Buildroot used: fedora-rawhide-x86_64
Active plugins: Generic, Shell-api, C/C++
Disabled plugins: Java, Python, fonts, SugarActivity, Ocaml, Perl, Haskell, R, PHP, Ruby
Disabled flags: EXARCH, DISTTAG, EPEL5, BATCH, EPEL6

Comment 14 Gwyn Ciesla 2016-04-13 13:07:21 UTC
Package request has been approved: https://admin.fedoraproject.org/pkgdb/package/rpms/micropython

Comment 15 Mairi Dulaney 2016-04-13 18:49:32 UTC
Builds for F24 and Rawhide are done, thanks for the review!


Note You need to log in before you can comment on or make changes to this bug.