Bug 1210941 - Review Request: isrcsubmit - Script to submit ISRCs from disc to MusicBrainz
Summary: Review Request: isrcsubmit - Script to submit ISRCs from disc to MusicBrainz
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Jonathan Underwood
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2015-04-11 08:42 UTC by Ville Skyttä
Modified: 2015-07-10 19:12 UTC (History)
3 users (show)

Fixed In Version: isrcsubmit-2.0.1-2.fc22
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2015-07-10 19:12:50 UTC
Type: ---
Embargoed:
jonathan.underwood: fedora-review+
gwync: fedora-cvs+


Attachments (Terms of Use)

Description Ville Skyttä 2015-04-11 08:42:51 UTC
https://scop.fedorapeople.org/packages/isrcsubmit.spec
https://scop.fedorapeople.org/packages/isrcsubmit-2.0.0-1.fc21.src.rpm

Description: isrcsubmit extracts ISRCs from audio cds and submits them to
MusicBrainz. Features: read and submit ISRCs from disc, search for
releases with the TOC of the disc, submit discIds / TOCs, display
release information from MusicBrainz, duplicate ISRC detection (local
and on server), keyring support for login information.

Fedora Account System Username: scop

Comment 1 Alexander Ploumistos 2015-04-18 18:02:01 UTC
Hello,

This is an informal review. I've had some years of experience with Python, but everything I ever wrote was very narrow-scope and vastly different than this, so please excuse (but do point out) any blunders.

On to the review.

I had checked the upstream package before getting on to running fedora-review, so it struck me as odd to see a no-manual-page-for-binary warning from rpmlint, as there exists an isrcsubmit.1.rst file. However, the resulting rpm does not contain "isrcsubmit.1". I have never used sphinx to produce documentation, but perhaps the problem stems from the .rst file extension, as all scripts that reference the file do so without its extension. Have you tried adding a
%{_mandir}/man1/%{name}.1*
in your %files section? Also, the source file is not groff formatted and that could be part of the problem. I think it's worth exploring.


I had also noticed that there is a test file included in the upstream package (which probably should be considered CC0 instead of GPLv3+ if it were to be included) and I was wondering why you hadn't placed it in a %check section in your spec file, but I ran the tests myself and a couple of them required to have a CD inserted, while the rest that made use of the included samples failed with the highly enlightening "False is not true" message. Do you have any idea why that happens?


I believe you could get rid of the "rm -rf $RPM_BUILD_ROOT" in your %install section, unless you want the package to build against rhel 5. Other than that, your file looks clean, well-structured and up to the latest specs.


As for the python-keyring requirement, since we do not have something like optional dependencies, I guess I would put it just like you have.


Every other item on fedora-review's checklist seems in order and according to the Python Packaging guidelines.

Comment 2 Ville Skyttä 2015-04-19 10:20:34 UTC
https://scop.fedorapeople.org/packages/isrcsubmit.spec
https://scop.fedorapeople.org/packages/isrcsubmit-2.0.0-2.fc21.src.rpm

* Sun Apr 19 2015 Ville Skyttä <ville.skytta> - 2.0.0-2
- Add man page, fix and run test suite (#1210941)

The "False is not True" messages are actually just the tracebacks of the same previously failed CD insertion requiring checks, not any other ones. Anyway the test suite is fixed and run now.

"rm -rf $RPM_BUILD_ROOT" in %install is not a matter of distro version but whether redhat-rpm-config is installed or not. I prefer to have my packages building properly also when it isn't, especially when it's not listed as a build dep.

Comment 3 Alexander Ploumistos 2015-04-19 14:17:14 UTC
(In reply to Ville Skyttä from comment #2)
> * Sun Apr 19 2015 Ville Skyttä <ville.skytta> - 2.0.0-2
> - Add man page, fix and run test suite (#1210941)

Do you think that test_isrcsubmit.py could be covered by GPLv3+ or does it need another license (Public Domain or CC0)? I find upstream's wording a bit confusing.

Although the man page is now in its proper place, rpmlint still complains. Is it because the binary is named "isrcsubmit.py" while the man page is for "isrcsubmit"? If that is true, do you know if there is a "proper" way of dealing with it, e.g. renaming the one or the other? I couldn't find something relevant in the packaging guidelines.

> The "False is not True" messages are actually just the tracebacks of the
> same previously failed CD insertion requiring checks, not any other ones.
> Anyway the test suite is fixed and run now.

Very nicely done.
 
> "rm -rf $RPM_BUILD_ROOT" in %install is not a matter of distro version but
> whether redhat-rpm-config is installed or not.

Could you please elaborate on that a bit, or point me to pertinent documentation? I have no experience building on rhel or anything older than F17. On my F21 system, repoquery --whatrequires redhat-rpm-config yields:

fedora-gnat-project-common-0:3.8-2.fc21.noarch
fedora-packager-0:0.5.10.4-2.fc21.noarch
fedora-packager-0:0.5.10.5-1.fc21.noarch
fedpkg-0:1.19-1.fc21.noarch
ghc-rpm-macros-0:1.2.16-1.fc21.x86_64
ghc-rpm-macros-0:1.2.19-1.fc21.x86_64
kernel-rpm-macros-0:26-1.fc21.noarch
kernel-rpm-macros-0:27-1.fc21.noarch
koji-builder-0:1.9.0-8.fc21.noarch
koji-builder-0:1.9.0-10.fc21.gitcd45e886.noarch
nodejs-packaging-0:7-2.fc21.noarch
pyrpkg-0:1.28-1.fc21.noarch
rpm-build-0:4.12.0.1-3.fc21.x86_64
rpm-build-0:4.12.0.1-5.fc21.x86_64
scl-utils-build-0:20140815-1.fc21.x86_64
scl-utils-build-1:2.0.1-2.fc21.x86_64
varnish-0:4.0.1-2.fc21.x86_64
varnish-0:4.0.3-3.fc21.x86_64

Is it possible to build rpms and not have redhat-rpm-config installed?

Comment 4 Ville Skyttä 2015-04-19 16:46:08 UTC
(In reply to Alexander Ploumistos from comment #3)
> Do you think that test_isrcsubmit.py could be covered by GPLv3+ or does it
> need another license (Public Domain or CC0)? I find upstream's wording a bit
> confusing.

I personally think this is a moot issue as we're not distributing test_isrcsubmit.py as part of the binary packages nor does it contribute the binary package contents in any way.

> If that is true, do you know if there is a "proper" way of
> dealing with it, e.g. renaming the one or the other? I couldn't find
> something relevant in the packaging guidelines.

I don't know if it's in the guidelines, but to me the proper way is to follow upstream, and if someone's interested enough, ask upstream to drop the *.py suffix from the executable as it becomes an issue if the script is sometimes implemented in something else besides python. I don't personally care that much.

> > "rm -rf $RPM_BUILD_ROOT" in %install is not a matter of distro version but
> > whether redhat-rpm-config is installed or not.
> 
> Could you please elaborate on that a bit

Not sure if there's actual documentation about it anywhere, but you can compare the output of "rpm -E %__spec_install_pre" with and without redhat-rpm-config installed on the distro version of your choice. See also (at least on F-20):
$ grep -A 4 __spec_install_pre /usr/lib/rpm/redhat/macros

> Is it possible to build rpms and not have redhat-rpm-config installed?

It certainly is. Note that the dependency that pulls in redhat-rpm-config in rpm-build on recent Fedora versions is on system-rpm-config, not redhat-rpm-config in particular (r-r-c just provides it, but it can be satisfied by another package providing it). And some active distributions don't even have that dependency, such as CentOS/RHEL/EPEL 6.

Comment 5 Alexander Ploumistos 2015-04-19 20:24:18 UTC
That was all very interesting and enlightening, thank you.

I hope you get the package promptly reviewed.

Cheers!

Comment 7 Jonathan Underwood 2015-06-27 19:17:30 UTC
Package Review
==============

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



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

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.
[!]: License field in the package spec file matches the actual license.
     Note: Checking patched sources after %prep for licenses. Licenses
     found: "GPL (v3 or later)", "Unknown or generated". 4 files have
     unknown license. Detailed output of licensecheck in
     /home/jgu/Fedora/1210941-isrcsubmit/licensecheck.txt

These files need to have GPLv3 headers added upstream:
isrcsubmit-2.0.1/doc/conf.py
isrcsubmit-2.0.1/isrcsubmit.sh
isrcsubmit-2.0.1/setup.py
isrcsubmit-2.0.1/test_isrcsubmit.py

[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.
[-]: 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.
[x]: Package is not known to require an ExcludeArch tag.
[x]: Large documentation must go in a -doc subpackage. Large could be size
     (~1MB) or number of files.
     Note: Documentation size is 20480 bytes in 3 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:
[x]: 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.
[x]: Description and summary sections in the package spec file contains
     translations for supported Non-English languages, if available.
[x]: Package should compile and build into binary rpms on all supported
     architectures.
[x]: %check is present and all tests pass.
[x]: 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]: Spec file according to URL is the same as in SRPM.


Rpmlint
-------
Checking: isrcsubmit-2.0.1-1.fc22.noarch.rpm
          isrcsubmit-2.0.1-1.fc22.src.rpm
isrcsubmit.noarch: W: spelling-error %description -l en_US cds -> cs, cads, cods
isrcsubmit.noarch: W: spelling-error %description -l en_US discIds -> disc Ids, disc-ids, disc
isrcsubmit.noarch: W: spelling-error %description -l en_US keyring -> keying, key ring, key-ring
isrcsubmit.noarch: W: no-manual-page-for-binary isrcsubmit.py
isrcsubmit.src: W: spelling-error %description -l en_US cds -> cs, cads, cods
isrcsubmit.src: W: spelling-error %description -l en_US discIds -> disc Ids, disc-ids, disc
isrcsubmit.src: W: spelling-error %description -l en_US keyring -> keying, key ring, key-ring
2 packages and 0 specfiles checked; 0 errors, 7 warnings.




Rpmlint (installed packages)
----------------------------
isrcsubmit.noarch: W: spelling-error %description -l en_US cds -> cs, cads, cods
isrcsubmit.noarch: W: spelling-error %description -l en_US discIds -> disc Ids, disc-ids, disc
isrcsubmit.noarch: W: spelling-error %description -l en_US keyring -> keying, key ring, key-ring
isrcsubmit.noarch: W: no-manual-page-for-binary isrcsubmit.py
1 packages and 0 specfiles checked; 0 errors, 4 warnings.



Requires
--------
isrcsubmit (rpmlib, GLIBC filtered):
    /usr/bin/python2
    python-keyring
    python-libdiscid
    python-musicbrainzngs



Provides
--------
isrcsubmit:
    isrcsubmit



Source checksums
----------------
http://isrcsubmit.jonnyjd.net/downloads/isrcsubmit-2.0.1.tar.gz :
  CHECKSUM(SHA256) this package     : 5ee97e4a59afa7372ca64be214576486ea67304f03afae3e018e23e7802c44c4
  CHECKSUM(SHA256) upstream package : 5ee97e4a59afa7372ca64be214576486ea67304f03afae3e018e23e7802c44c4


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



[Note, I also ran fedora-review with -P python as well afterwards and no additional issues arise].


So, in summary, the only item is asking upstream to add the GPL header to a couple of files. I'll let the package pass review, but please do follow up on that.

Comment 8 Ville Skyttä 2015-06-27 20:07:28 UTC
I must say I disagree with fedora-review wrt the copyright header stuff:

isrcsubmit-2.0.1/doc/conf.py: configuration autogenerated by sphinx-quickstart, I don't think is copyrightable.

isrcsubmit-2.0.1/isrcsubmit.sh: 3 lines of trivial code, probably not copyrightable, and FWIW in any case not shipped in the binary package nor during build.

isrcsubmit-2.0.1/setup.py: I don't think I've ever seen a copyright notice in a setup.py.

isrcsubmit-2.0.1/test_isrcsubmit.py: Contains a copyright notice.

Comment 9 Ville Skyttä 2015-06-27 20:09:16 UTC
New Package SCM Request
=======================
Package Name: isrcsubmit
Short Description: Script to submit ISRCs from disc to MusicBrainz
Upstream URL: http://jonnyjd.github.io/musicbrainz-isrcsubmit/
Owners: scop
Branches: f22
InitialCC:

Comment 10 Gwyn Ciesla 2015-06-29 15:59:32 UTC
Git done (by process-git-requests).

Comment 11 Fedora Update System 2015-06-29 19:57:07 UTC
isrcsubmit-2.0.1-2.fc22 has been submitted as an update for Fedora 22.
https://admin.fedoraproject.org/updates/isrcsubmit-2.0.1-2.fc22

Comment 12 Fedora Update System 2015-06-30 20:13:12 UTC
isrcsubmit-2.0.1-2.fc22 has been pushed to the Fedora 22 testing repository.

Comment 13 Fedora Update System 2015-07-10 19:12:50 UTC
isrcsubmit-2.0.1-2.fc22 has been pushed to the Fedora 22 stable repository.


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