https://bugzilla.redhat.com/show_bug.cgi?id=1585365 --- Comment #3 from Michael Cronenworth <mike@xxxxxxxxxx> --- (In reply to Robert-André Mauchin from comment #1) > - Don't ship the INSTALL file: > > miniupnpd.x86_64: W: install-file-in-docs /usr/share/doc/miniupnpd/INSTALL It contains notes about the conf file. Nothing in the guidelines denies providing it. Numerous other packages in Fedora provide it. "less" "openssl" "glibc"... all provide it. > - Perm should be 644: > > miniupnpd.src: W: strange-permission miniupnpd.service 640 My local copy of the file was set this way. The installed file was correct, but I've fixed this anyway. > - You need to include SystemD Requires: > > %{?systemd_requires} > BuildRequires: systemd Yep. Missed it. Thanks. (In reply to Eamon Walsh from comment #2) > In the service unit file: > ip6tables_init.sh and ip6tables_removeall.sh are not executed, > add ExecStartPre and ExecStopPost lines for them. Sure. > In the service unit file: > -After=network.target network-online.target > +After=network-online.target Not really any negatives from providing both. I'm happier by leaving both. > In the service unit file, suggest: > -PIDFile=/run/miniupnpd.pid > +PIDFile=/var/run/miniupnpd.pid > since that's where the daemon thinks it is. > Alternately, suggest passing "-P /run/miniupnpd.pid" to the daemon. The /run and /var/run paths are the same. The symlink to /var/run is /run. > In the service unit file, suggest: > +Documentation: http://miniupnp.free.fr/ Why? The man page is better. > In %build: > -make %{?_smp_mflags} -f Makefile.linux config.h > #Enable IPv6/IGDv2 support > -sed -i 's/\/*#define ENABLE_IPV6*\//#define ENABLE_IPV6/' config.h > -sed -i 's/\/*#define IGD_V2*\//#define IGD_V2/' config.h > +export CONFIG_OPTIONS="--ipv6 --igd2" > (see Makefile.linux line 8) > Also, the sed lines don't work as intended, the asterisks are not escaped Thanks. > In %build, suggest: > -export CFLAGS="$RPM_OPT_FLAGS" > +export CFLAGS="%{optflags}" > based on https://pagure.io/package-cleanup-service "Style Guildelines" (6th > bullet) Personal preference... but I've changed it. > In %install, suggest: > -export STRIP="true" > +export STRIP=/bin/true Not necessary. Path is expected to be present otherwise your "install" or other commands would fail. > In %install, suggest: > -mkdir -p %{buildroot}%{_unitdir} > -cp -p %{SOURCE1} %{buildroot}%{_unitdir}/%{name}.service > -chmod 644 %{buildroot}%{_unitdir}/%{name}.service > +install -Dpm 644 %{SOURCE1} %{buildroot}%{_unitdir}/%{name}.service OK. New spec: http://michael.cronenworth.com/RPMS/miniupnpd.spec New SRPM: http://michael.cronenworth.com/RPMS/miniupnpd-2.1-2.fc28.src.rpm -- 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://getfedora.org/code-of-conduct.html List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines List Archives: https://lists.fedoraproject.org/archives/list/package-review@xxxxxxxxxxxxxxxxxxxxxxx/message/6B7ZEQLVPJOXLHCRLGRQ5OSF4LS2GQPF/
