Fedora Merge Review: tmpwatch http://cvs.fedora.redhat.com/viewcvs/devel/tmpwatch/ Initial Owner: mitr
Ok some comments on this one: * No URL: Add an URL tag pointing to the upstream homepage. * No Source URL: You have to use the URL to the upstream tarball in the Source tag and not just the tarball name. See: http://fedoraproject.org/wiki/Packaging/Guidelines#head-c17fb8c1ce9be40da720a2b25d1e2a241062038f NOTE: if it has any homepage... I was unable to find it. * Parallel make: Append to make %{?_smp_mflags} in %{build}, unless you have a reason not to do so (package does not build with parallel make). See: http://fedoraproject.org/wiki/Packaging/Guidelines#head-525c7d76890cb22df33b759c65c35c82bf434d2e * Macro usage: Use %{_sysconfdir} instead of /etc
I took a further look and have a couple of additional issues. To sumarize: I there's really no upstream, please make a note of that in the spec. http://fedoraproject.org/wiki/Packaging/SourceURL If it's hosted internally to Red Hat, please consider transferring it to some externally visible site such as fedorahosted.org. Parallel make: there's only one source file so there's really no point, but if you're fixin things you might as well future-proof thins. Then there's this rpmlint complaint: %{_sysconfdir} is generally preferred to /etc. tmpwatch.x86_64: E: executable-marked-as-config-file /etc/cron.daily/tmpwatch This one is interesting. I'm guessing that this was marked as %config in the past because someone was annoyed that a package update wiped out their changes. However: A shell script is a really poor configuration interface. If we end up having to protect an additional directory in /tmp from cleanup, there will be issues because we can't update that script. Would it be at all possible to move the bits that people might want to configure into a file in /etc/sysconfig? It doesn't look like it should be difficult at all. The only licensing information I can see is in tmpwatch.c, which says only "under the terms of the GPL". Isn't Red Hat developed code supposed to be GPLv2 only? I'm told it's corporate policy. BuildRoot: is not correct; it needs to have at least %release, but would be better if it were it were one of the recommended values. Checklist: ? can't compare sources with upstream. * package meets naming and versioning guidelines. * specfile is properly named, is cleanly written. X should use %{_sysconfdir} * summary is OK. * description is OK. X build root is improper. ? license field matches the actual license. ? not sure if the license is correct. * BuildRequires are proper (none) * compiler flags are appropriate. * %clean is present. * package builds in mock (rawhide, x86_64). * package installs properly * debuginfo package looks complete. X rpmlint has valid complaints. * final provides and requires are sane: config(tmpwatch) = 2.9.12-2 tmpwatch = 2.9.12-2 = /bin/sh config(tmpwatch) = 2.9.12-2 psmisc * %check is not present. * no shared libraries are added to the regular linker search paths. * neither creates nor owns any directories. * no duplicates in %files. * file permissions are appropriate. * no scriptlets present. * code, not content. * documentation is small, so no -docs subpackage is necessary. * %docs are not necessary for the proper functioning of the package. * no headers. * no pkgconfig files. * no static libraries. * no libtool .la files.
Created attachment 292099 [details] Spec file for tmpwatch-2.9.12-3 Thank you both for the reviews. (In reply to comment #1) > * Parallel make: > Append to make %{?_smp_mflags} in %{build}, unless you have a reason not to do > so (package does not build with parallel make). It does not make any difference right now, because (make) runs only one command - but future-proofing the spec file doesn't hurt. Fixed. > * Macro usage: > > Use %{_sysconfdir} instead of /etc I'm not sure it makes sense for /etc/cron.daily; I understand one can override %{_prefix} and have a special set of packages rooted e.g. under /opt/fedora-backports, but cron.daily/tmpwatch must be in a directory that is actually processed by the system crontab, not under /opt/fedora-backports. This is all hypothetical though, I'm quite willing to use the macro if you think it is necessary. (In reply to comment #2) > I there's really no upstream, please make a note of that in the spec. > http://fedoraproject.org/wiki/Packaging/SourceURL Added. > If it's hosted internally to Red Hat, please consider transferring it to some > externally visible site such as fedorahosted.org. It is actually on elvis. I should really move all my packages to hosted, but there's always some more urgent work. > Parallel make: there's only one source file so there's really no point, but if > you're fixin things you might as well future-proof thins. Added. > Then there's this rpmlint complaint: > %{_sysconfdir} is generally preferred to /etc. See above. > tmpwatch.x86_64: E: executable-marked-as-config-file /etc/cron.daily/tmpwatch > This one is interesting. I'm guessing that this was marked as %config in the > past because someone was annoyed that a package update wiped out their changes. > However: > A shell script is a really poor configuration interface. > If we end up having to protect an additional directory in /tmp from cleanup, > there will be issues because we can't update that script. > > Would it be at all possible to move the bits that people might want to configure > into a file in /etc/sysconfig? It doesn't look like it should be difficult at all. People "might want to configure" most of the script (it would be "reasonable" to add at least 6 variables), and I'm afraid I don't know how to make the script configurable in a reasonable future-proof way that wouldn't encourage adding progressively convoluted variables every few months, and wouldn't be horribly overengineered. > The only licensing information I can see is in tmpwatch.c, which says only > "under the terms of the GPL". Isn't Red Hat developed code supposed to be GPLv2 > only? I'm told it's corporate policy. Yes; I guess this was an oversight - but over time, patches from external contributors were accepted and I don't think this would be fair to them. I'll ask our lawyers. > BuildRoot: is not correct; it needs to have at least %release, but would be > better if it were it were one of the recommended values. Fixed.
> > The only licensing information I can see is in tmpwatch.c, which says only > > "under the terms of the GPL". Isn't Red Hat developed code supposed to be GPLv2 > > only? I'm told it's corporate policy. > Yes; I guess this was an oversight - but over time, patches from external > contributors were accepted and I don't think this would be fair to them. > > I'll ask our lawyers. The next version of tmpwatch will use GPLv2.
Jason, Miloslav - ping?
I've no idea why I haven't looked at this in a year; I guess I was waiting for that "next version" to exist, which happened last February. I'm going to say at this point that everything looks fine to me. However, I think that it would be better to reference the tarball you uploaded to the releases directory instead of the one attached to the wiki, as then the URL is much simpler and you can use it directly: Source0: https://fedorahosted.org/releases/t/m/tmpwatch/tmpwatch-2.9.13.tar.bz2 This is, however, minor. APPROVED
Thanks. I'll fix the URL in the next release (which won't be available as a wiki attachment), I don't think it's worth the effort to fix it in 2.9.13.
I agree completely. Thanks for your time.