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
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.
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.
(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?
(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.
That was all very interesting and enlightening, thank you. I hope you get the package promptly reviewed. Cheers!
https://scop.fedorapeople.org/packages/isrcsubmit.spec https://scop.fedorapeople.org/packages/isrcsubmit-2.0.1-1.fc22.src.rpm - Update to 2.0.1
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.
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.
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:
Git done (by process-git-requests).
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
isrcsubmit-2.0.1-2.fc22 has been pushed to the Fedora 22 testing repository.
isrcsubmit-2.0.1-2.fc22 has been pushed to the Fedora 22 stable repository.