Spec URL: https://gitlab.com/musicinmybrain/pngcheck-rpm/-/raw/master/pngcheck.spec Patch URL: https://gitlab.com/musicinmybrain/pngcheck-rpm/-/raw/master/pngcheck-2.3.0-format-security.patch SRPM URL: https://kojipkgs.fedoraproject.org//work/tasks/1383/53081383/pngcheck-2.3.0-1.fc34.src.rpm Description: pngcheck verifies the integrity of PNG, JNG and MNG files (by checking the internal 32-bit CRCs [checksums] and decompressing the image data); it can optionally dump almost all of the chunk-level information in the image in human-readable form. For example, it can be used to print the basic statistics about an image (dimensions, bit depth, etc.); to list the color and transparency info in its palette (assuming it has one); or to extract the embedded text annotations. This is a command-line program with batch capabilities. Included with pngcheck (since version 2.1.0) are two helper utilities: - pngsplit - break a PNG, MNG or JNG image into constituent chunks (numbered for easy reassembly) - png-fix-IDAT-windowsize - fix minor zlib-header breakage caused by libpng 1.2.6 Fedora Account System Username: music Koji build for Fedora 34: https://koji.fedoraproject.org/koji/taskinfo?taskID=53081365 Koji build for Fedora 33: https://koji.fedoraproject.org/koji/taskinfo?taskID=53081635 Koji build for Fedora 32: https://koji.fedoraproject.org/koji/taskinfo?taskID=53081851 Koji build for EPEL8: https://koji.fedoraproject.org/koji/taskinfo?taskID=53081964 Koji build for EPEL7: https://koji.fedoraproject.org/koji/taskinfo?taskID=53082044 Note that pngcheck itself is under a minimal MIT license, while the helper utilities, packaged in -extras, are GPLv2+. The (sub)package licenses reflect this. Upstream is not very active. There is no bug tracker, and the last release was 13 years ago. The utility itself nevertheless remains useful. A small patch is used to allow the program to build without disabling -Werror=format-security. This patch, and a separate file containing the MIT license text, would generally be good suggestions to push back upstream; however, given the lack of upstream activity, it does seem unlikely that a new release would be made merely to accommodate Fedora’s preferences. ---- This is my second package for Fedora. I am seeking sponsorship into the packager group; see my first approved package, https://bugzilla.redhat.com/show_bug.cgi?id=1885684, for details.
Very nice and comprehensive description and big props for running Koji builds on several Fedora releases! > A small patch is used to allow the program to build without disabling -Werror=format-security. This patch, and a separate file containing the MIT license text, would generally be good suggestions to push back upstream; however, given the lack of upstream activity, it does seem unlikely that a new release would be made merely to accommodate Fedora’s preferences. I would still contact upstream and see what they have to say about it, and whether they're responsive at all. Worst case scenario, you can fork the project and add the missing components into your fork. > Source0: https://downloads.sourceforge.net/png-mng/%{name}-%{version}.tar.gz > > Patch0: pngcheck-2.3.0-format-security.patch You can reuse %{name} and %{version} in the Patch0 line. Also, please add a note above explaining why the patch is needed. That information needs to be in the SPEC file. You already have it in your first post: > A small patch is used to allow the program to build without disabling -Werror=format-security. > %package extras > Summary: Helper utilities distributed with %{name} > License: GPLv2+ Should pngcheck-extras depend on pngcheck? If so, and it's very likely, it needs an appropriate line like this: > Requires: %{name}%{?_isa} = %{version}-%{release} > %install > install -d '%{buildroot}%{_bindir}' > install -d '%{buildroot}%{_mandir}/man1' > find . -maxdepth 1 -type f -perm /0111 | while read -r bin > do > install -t '%{buildroot}%{_bindir}' "${bin}" > install -t '%{buildroot}%{_mandir}/man1' -m 0644 "${bin}.1" > done Since you're not using %make_install, please add the -p flag to your "install" calls to preserve timestamps. Extra comments in the full review below: 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]: If your application is a C or C++ application you must list a BuildRequires against gcc, gcc-c++ or clang. [x]: Package does not contain any libtool archives (.la) [x]: Rpath absent or only used for internal libs. Generic: [x]: Package successfully compiles and builds into binary rpms on at least one supported primary architecture. Note: Using prebuilt packages [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. Note: Checking patched sources after %prep for licenses. Licenses found: "Unknown or generated", "NTP License", "GNU Lesser General Public License", "GPL (v2 or later) (with incorrect FSF address)". 8 files have unknown license. Detailed output of licensecheck in /home/amender/rpmbuild/SPECS/pngcheck/pngcheck/licensecheck.txt [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. [-]: Large documentation must go in a -doc subpackage. Large could be size (~1MB) or number of files. Note: Documentation size is 40960 bytes in 4 files. [x]: Package complies to the Packaging Guidelines [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]: 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 must not depend on deprecated() packages. [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]: Reviewer should test that the package builds in mock. [ ]: If the source package does not include license text(s) as a separate file from upstream, the packager SHOULD query upstream to include it. Review: mentioned in an earlier comment. [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 pngcheck-extras Review: mentioned in an earlier comment. [x]: Package functions as described. [x]: Latest version is packaged. [x]: Package does not include license text files separate from upstream. [ ]: Patches link to upstream bugs/comments/lists or are otherwise justified. Review: mentioned in an earlier comment. [-]: Sources are verified with gpgverify first in %prep if upstream publishes signatures. Note: gpgverify is not used. [-]: 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. [-]: %check is present and all tests pass. [ ]: Packages should try to preserve timestamps of original installed files. Review: mentioned in an earlier comment. [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]: Packager, Vendor, PreReq, Copyright tags should not be in spec file [x]: Sources can be downloaded from URI in Source: tag [x]: SourceX is a working URL. [x]: Spec use %global instead of %define unless justified. ===== EXTRA items ===== Generic: [x]: Rpmlint is run on debuginfo package(s). Note: No rpmlint messages. [x]: Rpmlint is run on all installed packages. Note: No rpmlint messages. [x]: Large data in /usr/share should live in a noarch subpackage if package is arched. Rpmlint ------- Checking: pngcheck-2.3.0-1.fc34.x86_64.rpm pngcheck-extras-2.3.0-1.fc34.x86_64.rpm pngcheck-debuginfo-2.3.0-1.fc34.x86_64.rpm pngcheck-debugsource-2.3.0-1.fc34.x86_64.rpm pngcheck-2.3.0-1.fc34.src.rpm pngcheck.x86_64: W: spelling-error %description -l en_US checksums -> check sums, check-sums, checks pngcheck.x86_64: W: spelling-error %description -l en_US sTER -> st Er, stet, steer pngcheck.x86_64: W: spelling-error %description -l en_US brokensuite -> broken suite, broken-suite, brokenhearted pngcheck-extras.x86_64: W: spelling-error %description -l en_US png -> pg, ping, pang pngcheck-extras.x86_64: W: spelling-error %description -l en_US windowsize -> window size, window-size, downsize pngcheck-extras.x86_64: W: spelling-error %description -l en_US zlib -> lib, glib, z lib pngcheck-extras.x86_64: E: incorrect-fsf-address /usr/share/licenses/pngcheck-extras/COPYING pngcheck.src: W: spelling-error %description -l en_US checksums -> check sums, check-sums, checks pngcheck.src: W: spelling-error %description -l en_US sTER -> st Er, stet, steer pngcheck.src: W: spelling-error %description -l en_US brokensuite -> broken suite, broken-suite, brokenhearted pngcheck.src:67: W: mixed-use-of-spaces-and-tabs (spaces: line 1, tab: line 67) 5 packages and 0 specfiles checked; 1 errors, 10 warnings. Rpmlint (debuginfo) ------------------- Checking: pngcheck-debuginfo-2.3.0-1.fc34.x86_64.rpm pngcheck-extras-debuginfo-2.3.0-1.fc34.x86_64.rpm 2 packages and 0 specfiles checked; 0 errors, 0 warnings. Rpmlint (installed packages) ---------------------------- (none): E: no installed packages by name pngcheck (none): E: no installed packages by name pngcheck-debugsource (none): E: no installed packages by name pngcheck-extras-debuginfo (none): E: no installed packages by name pngcheck-extras (none): E: no installed packages by name pngcheck-debuginfo 0 packages and 0 specfiles checked; 0 errors, 0 warnings. Source checksums ---------------- https://downloads.sourceforge.net/png-mng/pngcheck-2.3.0.tar.gz : CHECKSUM(SHA256) this package : 77f0a039ac64df55fbd06af6f872fdbad4f639d009bbb5cd5cbe4db25690f35f CHECKSUM(SHA256) upstream package : 77f0a039ac64df55fbd06af6f872fdbad4f639d009bbb5cd5cbe4db25690f35f Requires -------- pngcheck (rpmlib, GLIBC filtered): libc.so.6()(64bit) libz.so.1()(64bit) rtld(GNU_HASH) pngcheck-extras (rpmlib, GLIBC filtered): libc.so.6()(64bit) libz.so.1()(64bit) rtld(GNU_HASH) pngcheck-debuginfo (rpmlib, GLIBC filtered): pngcheck-debugsource (rpmlib, GLIBC filtered): Provides -------- pngcheck: pngcheck pngcheck(x86-64) pngcheck-extras: pngcheck-extras pngcheck-extras(x86-64) pngcheck-debuginfo: debuginfo(build-id) pngcheck-debuginfo pngcheck-debuginfo(x86-64) pngcheck-debugsource: pngcheck-debugsource pngcheck-debugsource(x86-64)
Thank you very much for your time and your careful review. > I would still contact upstream and see what they have to say about it, and whether they're responsive at all. A reasonable request. I have emailed Greg Roelofs to provide the -Werror=format-security patch and the separate MIT license file suggestion upstream. I will comment again here if I receive a prompt response. > Worst case scenario, you can fork the project and add the missing components into your fork. I respect your suggestion. However, I have to say that I have never seen the Fedora guidelines encourage the creation of upstream forks, even in difficult cases such as https://docs.fedoraproject.org/en-US/packaging-guidelines/SourceURL/#when-upstream-uses-prohibited-code, much less in order to forcibly upstream a trivial patch. In fact, it seems like such a fork would go against the spirit of https://fedoraproject.org/wiki/Staying_close_to_upstream_projects by obscuring the relationship of the packaged software to the “true” upstream. Certainly, the guidelines only advise that patches should be submitted upstream, and do not say that they must be accepted. In this case, https://docs.fedoraproject.org/en-US/packaging-guidelines/PatchUpstreamStatus/#_if_upstream_doesnt_have_a_bug_tracker would seem to apply. Similarly, https://docs.fedoraproject.org/en-US/packaging-guidelines/LicensingGuidelines/#_license_text and the package review checklist require that upstream should be queried to include a separate license file when one is absent, but do not require that upstream accepts the suggestion. In both areas, I think the guidelines are entirely satisfied by my email to Greg Roelofs and by adding the appropriate comments to the spec file. > You can reuse %{name} and %{version} in the Patch0 line. I’m not sure about using %{version} here. I would expect the version in the patch should be the version against which the patch was created. If a future version 2.3.1 comes out, but the patch against 2.3.0 is still needed and applies cleanly without modification, then I would not expect the version number in the patch name to be updated. In the end, I think this is a matter of opinion rather than of Fedora policy, and opinions may validly differ. As a reasonable compromise, I have therefore incorporated the %{name} macro but removed the version number from the patch name altogether, i.e., %{name}-format-security.patch. > Also, please add a note above explaining why the patch is needed. That information needs to be in the SPEC file. Thanks; I have done so. > Should pngcheck-extras depend on pngcheck? If so, and it's very likely, … This is actually one of the rare cases where the dependency on the base package does not exist, either explicitly or implicitly. Each extra utility is a stand-alone executable built from a single C source file, and is perfectly useful without the pngcheck executable from the base package. The “extras” just happen to be released together with pngcheck in the same source tarball. (The -extras subpackage exists primarily to divide the binary RPMs by license, as advised in https://docs.fedoraproject.org/en-US/packaging-guidelines/LicensingGuidelines/#_multiple_licensing_scenarios, to avoid a multiple-licensed RPM.) > Since you're not using %make_install, please add the -p flag to your "install" calls to preserve timestamps. Good catch. I added -p for the file installations. (I can’t use %make_install because the upstream makefile lacks an install target.) ---- I think the above covers all of your review comments. Here are the updated URLs: Spec URL: https://gitlab.com/musicinmybrain/pngcheck-rpm/-/raw/master/pngcheck.spec Patch URL: https://gitlab.com/musicinmybrain/pngcheck-rpm/-/raw/master/pngcheck-format-security.patch SRPM URL: https://kojipkgs.fedoraproject.org//work/tasks/7401/53307401/pngcheck-2.3.0-1.fc34.src.rpm Koji build for Fedora 34: https://koji.fedoraproject.org/koji/taskinfo?taskID=53307375 Koji build for Fedora 33: https://koji.fedoraproject.org/koji/taskinfo?taskID=53307975 Koji build for Fedora 32: https://koji.fedoraproject.org/koji/taskinfo?taskID=53308374 Koji build for EPEL8: https://koji.fedoraproject.org/koji/taskinfo?taskID=53308440 Koji build for EPEL7: https://koji.fedoraproject.org/koji/taskinfo?taskID=53308517
> I respect your suggestion. However, I have to say that I have never seen the Fedora guidelines encourage the creation of upstream forks, [...] What I was trying to say was that if the project is worth it, but is completely unmaintained and you might have the time and/or know-how to maintain it, it would be worthwhile to fork it. It's indirectly mentioned in "Unmaintained Or Unresponsive Upstream Projects" from the guidelines you posted. Before that happens, of course, upstream should be informed and that discussed with them properly, and indicated that the project is a fork in the SPEC file. As for the review, very thoroughly done. Package approved!
> What I was trying to say was… That makes sense and highlights a section of the guidelines I hadn’t poked through thoroughly. Thanks again for your time and thorough review.
This comment is just to report that Greg Roelofs responded to my email rather promptly; upstream should be considered active even though the software is “done.” He was happy to hear of my packaging efforts and was willing to consider making a future upstream release with a separate MIT license file and with the -Werror=format-security patch or similar.
Awesome! Thank you very much for the update.
(fedscm-admin): The Pagure repository was created at https://src.fedoraproject.org/rpms/pngcheck