Spec URL: https://raphgro.fedorapeople.org/review/net/docsis/docsis.spec SRPM URL: https://raphgro.fedorapeople.org/review/net/docsis/docsis-0.9.6-1.20140107git423fcb3.fc21.src.rpm Description: Encode a DOCSIS binary configuration file Fedora Account System Username: raphgro rawhide build http://koji.fedoraproject.org/koji/taskinfo?taskID=8552846 rpmlint > E: incorrect-fsf-address /usr/share/doc/docsis/COPYING https://github.com/rlaager/docsis/issues/8 (bug #700095) > W: name-repeated-in-summary C DOCSIS Unsure if that is a false positive. DOCSIS may mean the underlying and used standard, not the package/project name itself. fedora-review > [!]: Uses parallel make %{?_smp_mflags} macro. Currently, it is not possible to build in parallel cause of the lexer job. > [!]: Package should not use obsolete m4 macros https://github.com/rlaager/docsis/issues/10
Release #2 with some master patches. Spec URL: https://raphgro.fedorapeople.org/review/net/docsis/docsis.spec SRPM URL: https://raphgro.fedorapeople.org/review/net/docsis/docsis-0.9.6-2.20140108git.fc21.src.rpm
I've got some spare time tonight; might as well use it on a package review.
Three things to fix up: 1. Source10 (the master branch version of COPYING) isn't good; it's a github page, not the raw text, and it changes every time it's downloaded. Could you get the raw text instead? 2. You have a bundled md5 implementation, but don't have the appropriate provides: https://fedoraproject.org/wiki/Packaging:No_Bundled_Libraries#Packages_granted_exceptions - I think it's bundled(md5-plumb) in this case. 3. I don't understand why you've put some of the documentation into /usr/share/docsis (the examples and the config format documentation), rather than into /usr/share/doc/docsis or into a separate docsis-doc subpackage. And some personal notes: * I don't like naming the package "docsis" - it's too general for my liking, given how big the DOCSIS spec is. Perhaps "docsis-config-encoder"? * 1.4M of the resulting installed package is "noarch" MIBs, to 500k of arch-dependent binaries. Is there any use for these MIBs without the config encoder? If so, it might be nice to split them out into their own noarch subpackage.
Hi Simon, thanks for your comments. I'll fix in near future since I'm currently very busy with other things.
(In reply to Simon Farnsworth from comment #3) > Three things to fix up: > > 1. Source10 (the master branch version of COPYING) isn't good; it's a > github page, not the raw text, and it changes every time it's downloaded. > Could you get the raw text instead? Fixed. There's no sense in linking any COPYING separately. Dunno why I did that. > 2. You have a bundled md5 implementation, but don't have the appropriate > provides: > https://fedoraproject.org/wiki/Packaging: > No_Bundled_Libraries#Packages_granted_exceptions - I think it's > bundled(md5-plumb) in this case. Fixed. > 3. I don't understand why you've put some of the documentation into > /usr/share/docsis (the examples and the config format documentation), rather > than into /usr/share/doc/docsis or into a separate docsis-doc subpackage. Fixed. > And some personal notes: > > * I don't like naming the package "docsis" - it's too general for my > liking, given how big the DOCSIS spec is. Perhaps "docsis-config-encoder"? Hmm, you should better discuss that with upstream. But okay, fixed. > * 1.4M of the resulting installed package is "noarch" MIBs, to 500k of > arch-dependent binaries. Is there any use for these MIBs without the config > encoder? If so, it might be nice to split them out into their own noarch > subpackage. Fixed.
Spec URL: https://raphgro.fedorapeople.org/review/net/docsis/docsis-config-encoder.spec SRPM URL: https://raphgro.fedorapeople.org/review/net/docsis/docsis-config-encoder-0.9.6-3.20150218git.fc21.src.rpm
rawhide scratch: http://koji.fedoraproject.org/koji/taskinfo?taskID=9072993
Uh… for armv7 we get: RunTests.sh: line 34: 21971 Segmentation fault (core dumped) $DOCSIS -e $TEST.txt $KEYFILE $TEST.cm.new Interesting.
SPEC: https://raphgro.fedorapeople.org/review/net/docsis/docsis-config-encoder.spec SRPM: https://raphgro.fedorapeople.org/review/net/docsis/docsis-config-encoder-0.9.6-4.20150218git8ef81f6.fc21.src.rpm rawhide scratch: http://koji.fedoraproject.org/koji/taskinfo?taskID=9103205 * Sat Feb 28 2015 Raphael Groner <projects.rg> - 0.9.6-4.20150218git8ef81f6 - disable a failing test on ARM
Getting closer: Must fix: * URL needs updating now that the package has been renamed for clarity. Should be http://docsis.sf.net not http://%{name}.sf.net * Docs are duplicated in the -doc subpackage and in the main package. Personal comments: * Is "Suggests" right for the snmp subpackage? The main package looks fairly useless without the MIBs - it'll technically work, but you won't be able to encode anything.
(In reply to Simon Farnsworth from comment #10) > Getting closer: > > Must fix: > > * URL needs updating now that the package has been renamed for clarity. > Should be http://docsis.sf.net not http://%{name}.sf.net Okay. Will be modified in next upload. > * Docs are duplicated in the -doc subpackage and in the main package. Which files do you mean? This seems to be caused by the bug in rpmbuild to not handle individual files %{_pkgdocdir} properly when they're %doc. > Personal comments: > > * Is "Suggests" right for the snmp subpackage? The main package looks > fairly useless without the MIBs - it'll technically work, but you won't be > able to encode anything. Okay. Changed that to Requires.
(In reply to Raphael Groner from comment #11) > (In reply to Simon Farnsworth from comment #10) > > Getting closer: > > > > Must fix: > > > > * URL needs updating now that the package has been renamed for clarity. > > Should be http://docsis.sf.net not http://%{name}.sf.net > > Okay. Will be modified in next upload. > > > * Docs are duplicated in the -doc subpackage and in the main package. > > Which files do you mean? This seems to be caused by the bug in rpmbuild to > not handle individual files %{_pkgdocdir} properly when they're %doc. > I mean that everything in the -doc subpackage is also contained in the main package, causing file conflicts between the two packages. > > Personal comments: > > > > * Is "Suggests" right for the snmp subpackage? The main package looks > > fairly useless without the MIBs - it'll technically work, but you won't be > > able to encode anything. > > Okay. Changed that to Requires. Oh, and a side note; while I'm not happy with "docsis" as a package name, I have no problem with the binary that comes out of it being named "docsis". My goal is just to make sure that "yum search docsis" is clear about what the package does; binary names can be the same as upstream if that makes your life easier.
(In reply to Simon Farnsworth from comment #12) … > Oh, and a side note; while I'm not happy with "docsis" as a package name, I > have no problem with the binary that comes out of it being named "docsis". > My goal is just to make sure that "yum search docsis" is clear about what > the package does; binary names can be the same as upstream if that makes > your life easier. It won't be a good idea to rename the package itself but keep the legacy name for the binary. This may probably confuse the user surely and could bring trouble in future when there are more packages named alike with partly docsis.
SPEC: https://raphgro.fedorapeople.org/review/net/docsis/docsis-config-encoder.spec SRPM: https://raphgro.fedorapeople.org/review/net/docsis/docsis-config-encoder-0.9.6-5.20150218git8ef81f6.fc21.src.rpm rawhide scratch: http://koji.fedoraproject.org/koji/taskinfo?taskID=9110163 * Sun Mar 01 2015 Raphael Groner <projects.rg (AT) smart.ms> - 0.9.6-5.20150218git8ef81f6 - prevent documentation duplication - run configure once only - fix URL - R: snmp subpackage - add license to subpackages - disable failing tests, e.g. at ARM
There are two remaining complaints from rpmlint, then the package passes: 1. COPYING is only included in a %license tag, which means it doesn't appear in /usr/share/doc - it looks like the %doc macro would normally copy it into place, but you're facing problems with that. Ideally, all three subpackages would have a copy of COPYING in /usr/share/doc, but I'm only fussed about it for the binary package. 2. The binary's permissions are 0775, not 0755. With these two complaints fixed, the package passes review.
(In reply to Simon Farnsworth from comment #15) > There are two remaining complaints from rpmlint, then the package passes: > > 1. COPYING is only included in a %license tag, which means it doesn't > appear in /usr/share/doc … You seem to be wrong in that point. There's a new guideline to use %license for all license texts, this was introduced some weeks ago. For F21, it's like a SHOULD currently but I think it will become a requirement in near future: "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 must be included in %license." https://fedoraproject.org/wiki/Packaging:LicensingGuidelines#License_Text > 2. The binary's permissions are 0775, not 0755. Will be fixed in the next upload. > With these two complaints fixed, the package passes review. Please do not forget to execute the fedora-review tool after the formal things got fixed. This tool found a lot of not obvious issues in my other packages. https://fedoraproject.org/wiki/Package_Review_Process#Reviewer
(In reply to Raphael Groner from comment #16) > (In reply to Simon Farnsworth from comment #15) … > > 2. The binary's permissions are 0775, not 0755. > > Will be fixed in the next upload. Not true. A standard executable should have permission set to 0755. If you get this message, it means that you have a wrong executable permissions in some files included in your package. http://wiki.rosalab.ru/en/index.php/Rpmlint_Errors#non-standard-executable-perm But have done some more other changes, e.g. use the latest commit from upstream.
(In reply to Raphael Groner from comment #16) > (In reply to Simon Farnsworth from comment #15) > > There are two remaining complaints from rpmlint, then the package passes: > > > > 1. COPYING is only included in a %license tag, which means it doesn't > > appear in /usr/share/doc … > > You seem to be wrong in that point. There's a new guideline to use %license > for all license texts, this was introduced some weeks ago. For F21, it's > like a SHOULD currently but I think it will become a requirement in near > future: "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 must be included in %license." > https://fedoraproject.org/wiki/Packaging:LicensingGuidelines#License_Text > Looks like F21's fedora-review is not up to date with this change - it's complaining that COPYING isn't in /usr/share/doc. Will ignore, as it's in /usr/share/licenses, and it looks like the guidelines are up to date. > > 2. The binary's permissions are 0775, not 0755. > > Will be fixed in the next upload. > > > With these two complaints fixed, the package passes review. > > Please do not forget to execute the fedora-review tool after the formal > things got fixed. This tool found a lot of not obvious issues in my other > packages. > https://fedoraproject.org/wiki/Package_Review_Process#Reviewer Every time I respond, I've been looking at the output from fedora-review. These are the last two complaints it has, and none of the manual check entries look wrong to me.
(In reply to Simon Farnsworth from comment #18) … > Looks like F21's fedora-review is not up to date with this change - it's > complaining that COPYING isn't in /usr/share/doc. Will ignore, as it's in > /usr/share/licenses, and it looks like the guidelines are up to date. Indeed. This is fixed in the devel branch at fedorahosted, so I guess it'll fixed in some future packaging. https://bugzilla.redhat.com/show_bug.cgi?id=1127916 As I do not plan to package for F20, it is not needed to use %doc here. … > Every time I respond, I've been looking at the output from fedora-review. > These are the last two complaints it has, and none of the manual check > entries look wrong to me. Ah okay. Sorry then for any confusion.
SPEC: https://raphgro.fedorapeople.org/review/net/docsis/docsis-config-encoder.spec SRPM: https://raphgro.fedorapeople.org/review/net/docsis/docsis-config-encoder-0.9.8-0.1.20150302git68e622f.fc21.src.rpm rawhide scratch: http://koji.fedoraproject.org/koji/taskinfo?taskID=9133743 * Tue Mar 03 2015 Raphael Groner <projects.rg> - 0.9.8-0.1.20150302git68e622f - switch to rlaager cause of merged with AdrianSimionov - new pre-version 0.9.8 (mentioned in ChangeLog) - make binary executable - remove Suggests: doc subpackage - move snmp files into data subpackage Thanks for the review!
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]: License file installed when any subpackage combination is installed. [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. [-]: 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]: Useful -debuginfo package or justification otherwise. [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 10240 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]: 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]: 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 do not use a name that already exist [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. Passing this, as there is a link to an upstream bug report [-]: 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]: Fully versioned dependency in subpackages if applicable. Note: No Requires: %{name}%{?_isa} = %{version}-%{release} in docsis- config-encoder-data , docsis-config-encoder-doc [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. [x]: Package should compile and build into binary rpms on all supported architectures. [x]: %check is present and all tests pass. All tests enabled in the package pass, but some have been disabled due to failure. As there are suitable comments linking to bug reports, this is a 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]: Dist tag is present (not strictly required in GL). [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. [x]: Package should not use obsolete m4 macros [x]: Spec file according to URL is the same as in SRPM. Rpmlint ------- Checking: docsis-config-encoder-0.9.8-0.1.20150302git68e622f.fc23.x86_64.rpm docsis-config-encoder-data-0.9.8-0.1.20150302git68e622f.fc23.noarch.rpm docsis-config-encoder-doc-0.9.8-0.1.20150302git68e622f.fc23.noarch.rpm docsis-config-encoder-0.9.8-0.1.20150302git68e622f.fc23.src.rpm docsis-config-encoder-data.noarch: W: no-documentation docsis-config-encoder-doc.noarch: W: file-not-utf8 /usr/share/doc/docsis-config-encoder/examples/docsis20_stresstest.cfg docsis-config-encoder-doc.noarch: W: file-not-utf8 /usr/share/doc/docsis-config-encoder/examples/docsis1.1_oss_v3coex.cfg docsis-config-encoder.src:26: W: unversioned-explicit-provides bundled(md5-plumb) 4 packages and 0 specfiles checked; 0 errors, 4 warnings. Rpmlint (installed packages) ---------------------------- docsis-config-encoder-data.noarch: W: no-documentation docsis-config-encoder-doc.noarch: W: file-not-utf8 /usr/share/doc/docsis-config-encoder/examples/docsis20_stresstest.cfg docsis-config-encoder-doc.noarch: W: file-not-utf8 /usr/share/doc/docsis-config-encoder/examples/docsis1.1_oss_v3coex.cfg 3 packages and 0 specfiles checked; 0 errors, 3 warnings. Requires -------- docsis-config-encoder-data (rpmlib, GLIBC filtered): docsis-config-encoder (rpmlib, GLIBC filtered): docsis-config-encoder-data libc.so.6()(64bit) libcrypto.so.10()(64bit) libm.so.6()(64bit) libnetsnmp.so.30()(64bit) libresolv.so.2()(64bit) libssl.so.10()(64bit) rtld(GNU_HASH) docsis-config-encoder-doc (rpmlib, GLIBC filtered): Provides -------- docsis-config-encoder-data: docsis-config-encoder-data docsis-config-encoder: bundled(md5-plumb) docsis-config-encoder docsis-config-encoder(x86-64) docsis-config-encoder-doc: docsis-config-encoder-doc Source checksums ---------------- https://github.com/rlaager/docsis/archive/68e622ff36ac05115a3b7e74b6e116451f23ba96.tar.gz#/docsis-68e622f.tar.gz : CHECKSUM(SHA256) this package : 9ac5764b0a78a56be4e1a65cfdbdd7fa486eb9b30270bd517fb372e593e982d6 CHECKSUM(SHA256) upstream package : 9ac5764b0a78a56be4e1a65cfdbdd7fa486eb9b30270bd517fb372e593e982d6 Generated by fedora-review 0.5.2 (63c24cb) last change: 2014-07-14 Command line :/bin/fedora-review -m fedora-rawhide-x86_64 -b 1179982 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, EPEL5, BATCH, DISTTAG Looks good to me: setting fedora-review to +.
New Package SCM Request ======================= Package Name: docsis-config-encoder Short Description: Encode a DOCSIS binary configuration file Upstream URL: http://docsis.sf.net Owners: raphgro Branches: f21 f22 InitialCC:
Git done (by process-git-requests).
docsis-config-encoder-0.9.8-0.1.20150302git68e622f.fc22 has been submitted as an update for Fedora 22. https://admin.fedoraproject.org/updates/docsis-config-encoder-0.9.8-0.1.20150302git68e622f.fc22
docsis-config-encoder-0.9.8-0.1.20150302git68e622f.fc21 has been submitted as an update for Fedora 21. https://admin.fedoraproject.org/updates/docsis-config-encoder-0.9.8-0.1.20150302git68e622f.fc21
docsis-config-encoder-0.9.8-0.1.20150302git68e622f.fc22 has been pushed to the Fedora 22 testing repository.
docsis-config-encoder-0.9.8-0.1.20150302git68e622f.fc22 has been pushed to the Fedora 22 stable repository.
docsis-config-encoder-0.9.8-0.1.20150302git68e622f.fc21 has been pushed to the Fedora 21 stable repository.
docsis-config-encoder-0.9.8-1.fc23 has been submitted as an update to Fedora 23. https://bodhi.fedoraproject.org/updates/FEDORA-2015-152bb83c9f
docsis-config-encoder-0.9.8-1.fc23 has been pushed to the Fedora 23 testing repository. If problems still persist, please make note of it in this bug report. If you want to test the update, you can install it with $ su -c 'dnf --enablerepo=updates-testing update docsis-config-encoder' You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2015-152bb83c9f
docsis-config-encoder-0.9.8-1.fc23 has been pushed to the Fedora 23 stable repository. If problems still persist, please make note of it in this bug report.