Product: Fedora https://bugzilla.redhat.com/show_bug.cgi?id=956147 --- Comment #5 from Paul Wouters <pwouters@xxxxxxxxxx> --- - Is the copyright disclaimer on the spec file really needed? It's not normally put under its own copyright - The upstream version is kinda awkward, but it's probably best to stick with it as you did. - I would include your patches as source, instead of adding a github repository. Unless you think you will become the one-upstream, obsoleting the sf.net and ubuntu links. - I think it would be clearer if you attach all the patches from the debian ubuntu as PatchXX: - Why use ubuntu_release and my_release ? For release numbers, start with "1" and increment each release. Unless it is a pre-release, then you start with "0". If you do the above, then the first section of macros can all go and it would be much cleaner and standard. - Don't package for multiple fedora versions. You must package it for rawhide. When your package is accepted, you can pick older branches and EPEL branches. In those branches you change the spec file to their respective init systems. That also removes the conditions you have now in the BuildRequires: and the use of "with_systemd". Note there are different macros to use for f18+ and F17. This also removes chkconfig stuff in fedora. - Don't specify BuildRoot, unless building for EL-5 - Don't run "rm" in prep, unless building for EL-5 - Try to stick with using the %configure macro. It sets all the right values including various build options. So in your case: %configure --sysconfdir=%{_sysconfdir}/%{name} --enable-libdhcp=no - For make, at least use: make %{?_smp_mflags} - For make install, you might want: make DESTDIR=%{buildroot} if the software supports it - Add -p (preserve) to the "install" options to preserve datestamps, this helps rpm not create useless .rpmnew files. - You must enable hardening that's used for daemons Just add: %global _hardened_build 1 at the top of the spec file, and make sure the cflags/linker flags make it properly. You can check using this script that is not yet integrated into rpmlint: https://nohats.ca/checksec.sh - Add the sysconfig file(s) as SOURCEx: files, and remove the use of rh_dir - Don't use systemctl directly, use the proper macros - The exit 0 should not be needed in %post - Remove the %clean section, unless you're building for EL-5 - Remove the %defattr line in the %files section, unless building for EL-5 - Use "tmpfiles" for the run directory (see other daemon packages, eg xl2tpd or nsd) - You're using /etc/ppp which belongs to the ppp package, so add Requires: for it - Why do you require the static version of flex ? flex-static seems to just install flex-devel? I do see it fails to compile without flex-devel, but this seems like a workaround :) - The resolv.conf hackery is problematic, but that can be tackled after the package has made it in. (one should use a NM plugin to rewrite that file, and if local DNSSEC is enabled you cannot touch that file at all. And if you chown, you probably also need chcon for SElinux) - Looks like there are some "sample" configs. That's not allowed outside of the %doc area. The config files should work from package install if placed in /etc. Apart from the sample files I see mention in /etc/wide-dhcpv6 / dhcp6c.conf of ethernet devices by names that are not valid for everyone. That's also a problem. - Your /etc/sysconfig files have empty VAR= entries, which systemd really does not like. (at least it did not in the past, not sure how it handles it these days) I haven't run the full fedora-review yet, but I will do so when you've addressed most of the things listed here :) I'm also not entirely sure how to test any of this, as my IPv6 comes in its own special setup. -- You are receiving this mail because: You are on the CC list for the bug. Unsubscribe from this bug https://bugzilla.redhat.com/token.cgi?t=D4S5m2zTTK&a=cc_unsubscribe _______________________________________________ package-review mailing list package-review@xxxxxxxxxxxxxxxxxxxxxxx https://admin.fedoraproject.org/mailman/listinfo/package-review