Hide Forgot
Spec URL: https://raw.githubusercontent.com/marcusmueller/signify-package/master/signify.spec SRPM URL: https://github.com/marcusmueller/signify-package/raw/master/signify-26-1.fc29.src.rpm Description: Sign and verify signatures on files, as used by the OpenBSD release maintainers. Fedora Account System Username: marcusmueller
This is informal review only. Feedback appreciated. I would advise you to contact upstream regarding license file. Also it would be nice to have %check and tests. Package Review ============== Legend: [x] = Pass, [!] = Fail, [-] = Not applicable, [?] = Not evaluated [ ] = Manual review needed ===== MUST items ===== C/C++: [x]: Provides: bundled(gnulib) in place as required. Note: Sources not installed [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]: Header files in -devel subpackage, if present. [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. [-]: 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]: License field in the package spec file matches the actual license. Note: There is no build directory. Running licensecheck on vanilla upstream sources. No licenses found. Please check the source files for licenses manually. [!]: License file installed when any subpackage combination is installed. [x]: Package does not own files or directories owned by other packages. Note: Dirs in package are owned also by: /usr/share/doc/signify(C, to, set, defaulting, locale,, Failed) [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. [?]: 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. [x]: 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 20480 bytes in 2 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: No rpmlint messages. [x]: Package requires other packages for directories it uses. [x]: Package must own all directories that it creates. [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: [?]: 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). [?]: Package functions as described. [x]: Latest version is packaged. [x]: Package does not include license text files separate from upstream. [-]: 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. [?]: Package should compile and build into binary rpms on all supported architectures. [!]: %check is present and all tests pass. [x]: Packages should try to preserve timestamps of original installed files. [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]: Fully versioned dependency in subpackages if applicable. [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: There are rpmlint messages (see attachment). [x]: Large data in /usr/share should live in a noarch subpackage if package is arched. [x]: Spec file according to URL is the same as in SRPM. Rpmlint ------- Checking: signify-26-1.fc32.x86_64.rpm signify-debuginfo-26-1.fc32.x86_64.rpm signify-debugsource-26-1.fc32.x86_64.rpm signify-26-1.fc32.src.rpm 4 packages and 0 specfiles checked; 0 errors, 0 warnings. Rpmlint (debuginfo) ------------------- Checking: signify-debuginfo-26-1.fc32.x86_64.rpm 1 packages and 0 specfiles checked; 0 errors, 0 warnings. Rpmlint (installed packages) ---------------------------- #problem on my side perl: warning: Setting locale failed. perl: warning: Please check that your locale settings: LANGUAGE = (unset), LC_ALL = (unset), LC_CTYPE = "C.UTF-8", LANG = "cs_CZ.UTF-8" are supported and installed on your system. perl: warning: Falling back to the standard locale ("C"). perl: warning: Setting locale failed. perl: warning: Please check that your locale settings: LANGUAGE = (unset), LC_ALL = (unset), LC_CTYPE = "C.UTF-8", LANG = "cs_CZ.UTF-8" are supported and installed on your system. perl: warning: Falling back to the standard locale ("C"). signify-debugsource.x86_64: W: invalid-url URL: https://github.com/aperezdc/signify <urlopen error [Errno -2] Name or service not known> signify-debuginfo.x86_64: W: invalid-url URL: https://github.com/aperezdc/signify <urlopen error [Errno -2] Name or service not known> signify.x86_64: W: invalid-url URL: https://github.com/aperezdc/signify <urlopen error [Errno -2] Name or service not known> 3 packages and 0 specfiles checked; 0 errors, 3 warnings. Source checksums ---------------- https://github.com/aperezdc/signify/releases/download/v26/signify-26.tar.xz : CHECKSUM(SHA256) this package : 5c2d9414cb302f0b127d8a930843461dac305c5e0e28828b6e4aaf55b00f549b CHECKSUM(SHA256) upstream package : 5c2d9414cb302f0b127d8a930843461dac305c5e0e28828b6e4aaf55b00f549b Requires -------- signify (rpmlib, GLIBC filtered): libbsd.so.0()(64bit) libbsd.so.0(LIBBSD_0.0)(64bit) libbsd.so.0(LIBBSD_0.2)(64bit) libbsd.so.0(LIBBSD_0.8)(64bit) libc.so.6()(64bit) rtld(GNU_HASH) signify-debuginfo (rpmlib, GLIBC filtered): signify-debugsource (rpmlib, GLIBC filtered): Provides -------- signify: signify signify(x86-64) signify-debuginfo: debuginfo(build-id) signify-debuginfo signify-debuginfo(x86-64) signify-debugsource: signify-debugsource signify-debugsource(x86-64) Generated by fedora-review 0.7.3 (44b83c7) last change: 2019-09-18 Command line :/usr/bin/fedora-review -u https://bugzilla.redhat.com/show_bug.cgi?id=1768027 Buildroot used: fedora-rawhide-{{ target_arch }} Active plugins: Generic, C/C++, Shell-api Disabled plugins: R, Java, Perl, Ocaml, fonts, PHP, Python, SugarActivity, Haskell Disabled flags: EPEL6, EPEL7, DISTTAG, BATCH, EXARCH
* Wrong use of PREFIX in install section - Usage of '/usr' is definitely not right. If nothing else, there should be used %{_prefix} macro for this purpose - I have not looked into the project, but wouldn't be better to override the DESTDIR? * Bundled libwaive - It appears that at leas in upstream repository, there is bundled libwaive. Would it be possible to unbundle this library? If not, there should be bundled provide [1] in .spec file mentioning this library. [1] https://docs.fedoraproject.org/en-US/packaging-guidelines/#bundling
(In reply to Vít Ondruch from comment #2) > * Wrong use of PREFIX in install section > - Usage of '/usr' is definitely not right. If nothing else, there should > be used %{_prefix} macro for this purpose Fixing that. > - I have not looked into the project, but wouldn't be better to override > the DESTDIR? fails when setting DESTDIR, works with PREFIX > * Bundled libwaive > - It appears that at leas in upstream repository, there is bundled > libwaive. Would it be possible to unbundle this library? If not, there > should be bundled provide [1] in .spec file mentioning this library. libwaive is included in the upstream source, but not being used (unless you `make PLEDGE=waive`, which I don't), so no action required.
(In reply to Ondřej Pohořelský from comment #1) > This is informal review only. Feedback appreciated. Feedback: thank you! Very much appreciated. > I would advise you to contact upstream regarding license file. Done! https://github.com/aperezdc/signify/pull/24 > Also it would > be nice to have %check and tests. On it. As soon as https://github.com/aperezdc/signify/commit/b82aa79bd2d679a31efd53f4eddcea5e56bf2ceb is part of a release, will include %check > [!]: License file installed when any subpackage combination is installed. As soon as https://github.com/aperezdc/signify/pull/24 is part of a release, this will be done. > ===== SHOULD items ===== > Generic: > [?]: If the source package does not include license text(s) as a separate > file from upstream, the packager SHOULD query upstream to include it. in process, see above > [?]: Package functions as described. Fundamental truth: Software is buggy, but this is relatively fine. > [!]: %check is present and all tests pass. in process, see above
(In reply to Marcus Müller from comment #3) to be specific: > libwaive is included in the upstream source, but not being used (unless you > `make PLEDGE=waive`, which I don't), so no action required. It's not being compiled, and not installed as library, either, so I really can't claim anything provides this.
(In reply to Marcus Müller from comment #5) > (In reply to Marcus Müller from comment #3) > > to be specific: > > > libwaive is included in the upstream source, but not being used (unless you > > `make PLEDGE=waive`, which I don't), so no action required. > > It's not being compiled, and not installed as library, either, so I really > can't claim anything provides this. Remove the code in %prep > [x]: %build honors applicable compiler flags or justifies otherwise. Are you sure that flags are used? `%make_build CFLAGS=-g` is fundamentally wrong
> Remove the code in %prep Removed! Didn't expect it to work without. > > [x]: %build honors applicable compiler flags or justifies otherwise. > > Are you sure that flags are used? > > `%make_build CFLAGS=-g` is fundamentally wrong I feel the same, but: without `-g`, there's no debug symbols to be extracted, and rpmbuilding fails. It took me a while to figure out that supplying `-g` helps. ``` + /usr/lib/rpm/find-debuginfo.sh -j8 --strict-build-id -m -i --build-id-seed 27-1.fc29 --unique-debug-suffix -27-1.fc29.x86_64 --unique-debug-src-base signify-27-1.fc29.x86_64 --run-dwz --dwz-low-mem-die-limit 10000000 --dwz-max-die-limit 110000000 -S debugsourcefiles.list /home/marcus/src/signify-package/signify-27 explicitly decompress any DWARF compressed ELF sections in /home/marcus/rpmbuild/BUILDROOT/signify-27-1.fc29.x86_64/usr/bin/signify extracting debug info from /home/marcus/rpmbuild/BUILDROOT/signify-27-1.fc29.x86_64/usr/bin/signify gdb-add-index: No index was created for /home/marcus/rpmbuild/BUILDROOT/signify-27-1.fc29.x86_64/usr/bin/signify gdb-add-index: [Was there no debuginfo? Was there already an index?] /usr/lib/rpm/sepdebugcrcfix: Updated 0 CRC32s, 1 CRC32s did match. […] Processing files: signify-debugsource-27-1.fc29.x86_64 error: Empty %files file /home/marcus/src/signify-package/signify-27/debugsourcefiles.list Empty %files file /home/marcus/src/signify-package/signify-27/debugsourcefiles.list ``` With `-g`, the debug symbols get extracted, then the binary gets stripped and a stripped binary gets installed: ``` $ file $(which signify) /usr/bin/signify: ELF 64-bit LSB executable, x86-64, version 1 (SYSV), dynamically linked, interpreter /lib64/ld-linux-x86-64.so.2, for GNU/Linux 3.2.0, BuildID[sha1]=e481012b03a1ccefe987d294d18eafc9f4b49935, stripped ``` Which led me to believe this was the right way to go.
%build %set_build_flags %make_build %install %make_install PREFIX=%{_prefix}
(In reply to Antonio from comment #8) > %set_build_flags That works, thanks! Updated: Spec URL: https://raw.githubusercontent.com/marcusmueller/signify-package/master/signify.spec SRPM URL: https://github.com/marcusmueller/signify-package/raw/master/signify-27-1.fc29.src.rpm
(In reply to Marcus Müller from comment #9) > (In reply to Antonio from comment #8) > > %set_build_flags > > That works, thanks! > > Updated: > Spec URL: > https://raw.githubusercontent.com/marcusmueller/signify-package/master/ > signify.spec > SRPM URL: > https://github.com/marcusmueller/signify-package/raw/master/signify-27-1. > fc29.src.rpm Include the License file when possible and remove bundled libraries in %prep section, also perform a scratch build on Rawhide, please. Then, we can do a definitive review.
> remove bundled libraries in %prep > section Could you elaborate on that? Does that mean `rm -r libwaive/` or something else?
(In reply to Marcus Müller from comment #11) > > remove bundled libraries in %prep > > section > > Could you elaborate on that? Does that mean `rm -r libwaive/` or something > else? Exactly; something like: %prep %autosetup rm -rf libwaive %build ...
ah, excellent, without the `%autosetup` I was struggling to be in the right directory. By the way, you've been tremendously helpful, especially by pointing me to the right macros to use. I might have learned basics of SPEC authorship too long ago to be "up to date" with modern Fedora RPM macros. Is there a reference SPEC or documentation you can recommend to learn about things like `%set_build_flags`? In the meantime: scratch build submitted. https://koji.fedoraproject.org/koji/taskinfo?taskID=40403945
The LICENSE file I currently consider a blocker: I've got a pull-request to the upstream github repo open, which I'd like the owner of that repo to review. After all, this is ISC license with a list of authors, and I'd like that list to be correct. It's, on the other hand, just a condensation of the copyright holders in the license headers in the individual source files. Haven't found a OpenBSD nor a Fedora policy for how to do that.
(In reply to Marcus Müller from comment #13) > > Is there a reference SPEC or documentation you can recommend to learn about > things like `%set_build_flags`? > https://docs.fedoraproject.org/en-US/packaging-guidelines/ > The LICENSE file I currently consider a blocker: I've got a pull-request to the upstream github repo open, which I'd like the owner of that repo to review. > After all, this is ISC license with a list of authors, and I'd like that list to be correct. https://docs.fedoraproject.org/en-US/packaging-guidelines/LicensingGuidelines/#_license_text
Use `ld` as linker: ... %build %set_build_flags export LD=ld %make_build ...
Bump, seems v29 fixes license issue.
also add newline between your changelog entries.
Marcus, are you still interested in maintaining this package? I think all your blockers are resolved now, and I'd be happy to do the final review on a v30 version of this package.
@spotrh@gmail.com on it.
@spotrh@gmail.com I might need an adult to help me through this (it's been a year since I last touched fedpkg/koji), because `fedpkg scratch-build` leads to a failing build attempt: https://koji.fedoraproject.org/koji/taskinfo?taskID=62627512 The problem seems to be that unlike my previous scratch-build (from a year ago... shame on me), this fails to git clone, checkout.log tells me ``` $ git clone -n https://src.fedoraproject.org/git@github.com:marcusmueller/signify-package.git /var/lib/mock/f35-build-25888185-3053391/root/chroot_tmpdir/scmroot/signify-package Cloning into '/var/lib/mock/f35-build-25888185-3053391/root/chroot_tmpdir/scmroot/signify-package'... fatal: repository 'https://src.fedoraproject.org/git@github.com:marcusmueller/signify-package.git/' not found ``` For now I'm `fedpkg scratch-build --srpm signify-30-1.fc35.src.rpm`, but this is a bit confusing.
Hmm. I always do scratch builds with the SRPM and not a git checkout, so I'm not sure I can help you there. Did the SRPM scratch-build succeed?
OK, then that's no big deal, as indeed, the SRPM build works beautifully on all rawhide archs.
Nice package. Couple of minor issues: 1. You have macros in the changelog. rpmlint warns on those. Those are easily fixed by replacing macro references (%check) with escaped references (%%check). 2. There are a few more licenses in play in this code base, but they're all compatible. Correct License tag should be License: ISC and MIT and BSD 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]: Header files in -devel subpackage, if present. [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. [ ]: License field in the package spec file matches the actual license. Note: Checking patched sources after %prep for licenses. Licenses found: "Unknown or generated", "ISC License", "*No copyright* ISC License", "BSD 4-clause "Original" or "Old" License", "Expat License", "ISC License Beerware License", "BSD 3-clause "New" or "Revised" 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. [-]: 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 20480 bytes in 2 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]: 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: [-]: 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. [-]: Package does not include license text files separate from upstream. [-]: 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. [x]: %check is present and all tests pass. [x]: Packages should try to preserve timestamps of original installed files. [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]: Fully versioned dependency in subpackages if applicable. [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. [x]: Spec file according to URL is the same as in SRPM. Rpmlint ------- Checking: signify-30-2.fc33.x86_64.rpm signify-debuginfo-30-2.fc33.x86_64.rpm signify-debugsource-30-2.fc33.x86_64.rpm signify-30-2.fc33.src.rpm signify.src:56: W: macro-in-%changelog %check signify.src:57: W: macro-in-%changelog %set_build_flags 4 packages and 0 specfiles checked; 0 errors, 2 warnings. Rpmlint (debuginfo) ------------------- Checking: signify-debuginfo-30-2.fc33.x86_64.rpm 1 packages and 0 specfiles checked; 0 errors, 0 warnings. Rpmlint (installed packages) ---------------------------- 3 packages and 0 specfiles checked; 0 errors, 0 warnings. Source checksums ---------------- https://github.com/aperezdc/signify/releases/download/v30/signify-30.tar.xz : CHECKSUM(SHA256) this package : f68406c3085ef902e85500e6c0b90e4c3f56347e5efffc0da7b6fb47803c8686 CHECKSUM(SHA256) upstream package : f68406c3085ef902e85500e6c0b90e4c3f56347e5efffc0da7b6fb47803c8686 Requires -------- signify (rpmlib, GLIBC filtered): libbsd.so.0()(64bit) libbsd.so.0(LIBBSD_0.0)(64bit) libbsd.so.0(LIBBSD_0.2)(64bit) libc.so.6()(64bit) rtld(GNU_HASH) signify-debuginfo (rpmlib, GLIBC filtered): signify-debugsource (rpmlib, GLIBC filtered): Provides -------- signify: signify signify(x86-64) signify-debuginfo: debuginfo(build-id) signify-debuginfo signify-debuginfo(x86-64) signify-debugsource: signify-debugsource signify-debugsource(x86-64) APPROVED (but please update the License: tag before committing).
> APPROVED (but please update the License: tag before committing). YES! Thank you :) Done, see git. Also, rpmlint on spec, srpm and installed rpms is all clean.
(fedscm-admin): The Pagure repository was created at https://src.fedoraproject.org/rpms/signify