https://bugzilla.redhat.com/show_bug.cgi?id=1756582 --- Comment #2 from Michal Schorm <mschorm@xxxxxxxxxx> --- Package Review ============== Legend: [x] = Pass, [!] = Fail, [-] = Not applicable, [?] = Not evaluated Issues: ======= 1) The package does not own '/usr/libexec/sshguard' directory, but it should. 2) Systemd X SysVinit The package has an option to not build sysvinit stuff. Which it fine - we want systemd files in Fedora instead. However the condition on the first 3 lines is broken. On Fedora it will expand to "0 <= 6", which is always TRUE, so it will always pack sysvinit stuff over systemd stuff. 2.1) Fix the condition 2.2) Logrotate is used only with sysvinit and not with systemd - is that an intention? 2.3) When the package is build with systemd service instead of sysvinit script, do you think it still worth to ship the example '/usr/share/doc/sshguard/sshguard.service' file, even if it's nearly the same as the actual service file? 2.4) Please note, that the base package contains systemd service file, but the package does not require systemd. Thus it can end up in a state, when it is installed, but systemd is not. If the main functionality remains even without systemd, it's fine. If the functionality depends on the service, you need to fix the package requirements. Please check if it's OK. Also note that the e.g. teh firewalld subpackage depends on firewalld which depends on systemd ... so in that case the systemd will be pulled in. 2.5) The systemd service contains e.g. "After=firewalld.service". If the service is not present or not started, this won't have any effect. Thus you can end up with sshguard service running but firewalld service not running; and an error message in the systemd journal: "sshguard[1518]: sshg-fw-firewalld: Could not initialize firewall" Check if that's OK. 3) I'd suggest to have every changelog entry (each header) separated by a newline, to have it consistent with both itself and all other pkgs in Fedora. 4) I saw two bundled libraries that I suspect they are bundled, can you please confirm? * simclist library? * Fowler/Noll/Vo Hash (fnv) library ? They are mentioned here too: https://bitbucket.org/sshguard/sshguard/src/036efe21bd46122fde9d3d85aa71ee72b4c8d7e4/COPYING And i see them amongst the debugsource files, so they were used during the build process. If they are packed in Fedora, those packages MUST be used instead. If they are not packaged in Fedora, you MUST pack them too. Only in very special cases, when neither of those two steps above are a good solution, you may bundle it. But in such case you MUST mark that the package provide those bundles. ===== 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. "sshguard is available under the terms of the OpenBSD license, which is based on the ISC License." The "OpenBSD" license is not specified here: https://fedoraproject.org/wiki/Licensing:Main?rd=Licensing#SoftwareLicenses, so it need aditional check. [x]: License file installed when any subpackage combination is installed. [x]: If the package is under multiple licenses, the licensing breakdown must be documented in the spec. [!]: Package requires other packages for directories it uses. Note: No known owner of /usr/libexec/sshguard [!]: Package must own all directories that it creates. Note: Directories without known owners: /usr/libexec/sshguard [x]: %build honors applicable compiler flags or justifies otherwise. [!]: 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. [x]: 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. [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 6 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]: 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 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]: %config files are marked noreplace or the reason is justified. [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]: No %config files under /usr. [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. [!]: Final provides and requires are sane. Reviewer note: NOT sane until packed with SysVinit [x]: Fully versioned dependency in subpackages if applicable. Note: No Requires: %{name}%{?_isa} = %{version}-%{release} in sshguard-iptables , sshguard-firewalld , sshguard-nftables [?]: Package functions as described. [X]: Latest version is packaged. [x]: Package does not include license text files separate from upstream. [?]: Scriptlets must be sane, if used. [x]: Sources are verified with gpgverify first in %prep if upstream publishes signatures. Note: gpgverify is not used. [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. https://koji.fedoraproject.org/koji/taskinfo?taskID=37927883 [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]: 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 all installed packages. [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. Generated by fedora-review 0.7.3 (44b83c7) last change: 2019-09-18 Command line :/usr/bin/fedora-review -n sshguard Buildroot used: fedora-rawhide-x86_64 Active plugins: Generic, Shell-api, C/C++ Disabled plugins: Java, SugarActivity, R, Python, PHP, fonts, Ocaml, Perl, Haskell Disabled flags: EPEL6, EPEL7, DISTTAG, BATCH, EXARCH -- You are receiving this mail because: You are on the CC list for the bug. You are always notified about changes to this product and component _______________________________________________ package-review mailing list -- package-review@xxxxxxxxxxxxxxxxxxxxxxx To unsubscribe send an email to package-review-leave@xxxxxxxxxxxxxxxxxxxxxxx Fedora Code of Conduct: https://docs.fedoraproject.org/en-US/project/code-of-conduct/ List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines List Archives: https://lists.fedoraproject.org/archives/list/package-review@xxxxxxxxxxxxxxxxxxxxxxx
