Bug 226495 - Merge Review: tmpwatch
Merge Review: tmpwatch
Status: CLOSED RAWHIDE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Jason Tibbitts
Fedora Package Reviews List
:
Depends On:
Blocks: F9MergeReviewTarget
  Show dependency treegraph
 
Reported: 2007-01-31 16:11 EST by Nobody's working on this, feel free to take it
Modified: 2009-01-14 09:55 EST (History)
3 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2009-01-13 20:53:01 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
tibbs: fedora‑review+


Attachments (Terms of Use)
Spec file for tmpwatch-2.9.12-3 (9.27 KB, text/plain)
2008-01-17 21:37 EST, Miloslav Trmač
no flags Details

  None (edit)
Description Nobody's working on this, feel free to take it 2007-01-31 16:11:09 EST
Fedora Merge Review: tmpwatch

http://cvs.fedora.redhat.com/viewcvs/devel/tmpwatch/
Initial Owner: mitr@redhat.com
Comment 1 Adel Gadllah 2007-12-22 06:35:19 EST
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

Comment 2 Jason Tibbitts 2008-01-11 14:17:04 EST
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.
Comment 3 Miloslav Trmač 2008-01-17 21:37:16 EST
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.
Comment 4 Miloslav Trmač 2008-01-20 22:02:01 EST
> > 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.
Comment 5 Robert Scheck 2008-12-20 09:38:56 EST
Jason, Miloslav - ping?
Comment 6 Jason Tibbitts 2009-01-13 20:53:01 EST
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
Comment 7 Miloslav Trmač 2009-01-14 05:54:51 EST
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.
Comment 8 Jason Tibbitts 2009-01-14 09:55:27 EST
I agree completely.  Thanks for your time.

Note You need to log in before you can comment on or make changes to this bug.