Bug 677043 (deheader)

I can't find any sort of connection between bugzilla and fedoraproject.org users, so just a note: my username in there is dreamertan and I have exactly same mail address filled in both systems.

Not a formal review, just some quick comments:

1. There's no need to specify a BuildRoot, or remove it in %install, or for the %clean section to exist.  Feel free to kill those parts.  (Yes rpmdev-newspec generates them, but they don't need to be there).  See http://fedoraproject.org/wiki/Packaging/Guidelines#BuildRoot_tag

2. The following is unnecessary
    mkdir -p %{buildroot}%{_docdir}/%{name}-%{version}
    cp {COPYING,NEWS,README} %{buildroot}%{_docdir}/%{name}-%{version}

   It happens automatically with %doc COPYING NEWS README

3. You should preserve timestamps.  See http://fedoraproject.org/wiki/Packaging/Guidelines#Timestamps

4. It seems like it's not needed to run make at all...  so don't :-)

1) Removed
2) Fixed; cool, I didn't know, that doc files don't need to be installed under buildroot :)
3) Fixed
4) Right :) man file is conveniently generated in tarball already. Removed %install and BuildRequires

Updated specfile and srpm:
http://dl.dropbox.com/u/420606/fedora-packages/deheader.spec
http://dl.dropbox.com/u/420606/fedora-packages/deheader-0.6-1.fc13.src.rpm

$ rpmlint deheader-0.6-1.fc13.noarch.rpm 
deheader.noarch: W: spelling-error %description -l en_US cohesions -> cohesion, cohesion's, cohesion s
1 packages and 0 specfiles checked; 0 errors, 1 warnings.

I think word is properly used in context ("many cohesions").

$ rpm -qlp deheader-0.6-1.fc13.noarch.rpm
/usr/bin/deheader
/usr/share/doc/deheader-0.6
/usr/share/doc/deheader-0.6/COPYING
/usr/share/doc/deheader-0.6/NEWS
/usr/share/doc/deheader-0.6/README
/usr/share/man/man1/deheader.1.gz

I don't see area for improvement in this small specfile any more ;)

> I don't see area for improvement in this small specfile any more ;)

I do. ;)


$ rpm -qpR deheader-0.6-1.fc14.noarch.rpm | grep -v ^rpm
/usr/bin/env  
python2  

$ head -1 /usr/bin/deheader 
#!/usr/bin/env python

This dependency ought to be improved. You've set a dependency on "python2", but the script is executed with the first "python" found in $PATH. Changing the shebang to /usr/bin/python (or /usr/bin/python2 if absolutely necessary) would be the preferred way.


> Requires:       python2

$ repoquery --whatprovides python2
python-0:2.7-8.fc14.1.i686

It would be good practice to add a comment to such explicit "Requires". To explain *why* an explicit dependency is necessary. Now I don't think this dependency is necessary. But if it were, it would be added value to also add "BuildRequires: python2", so you could check for the needed run-time dependency already at build-time.


> Summary:        Find (optionally remove) unneeded includes in C or C++ source files

If one removes the brackets, an "and" is missing, so IMO better would be:
Find (and optionally remove) unneeded includes in C or C++ source files


> I think word is properly used in context ("many cohesions").

No big issue at all.  That plural form is very unsual/rare, even if Eric's included "control" file also uses it. The plural for cohesion as a type of measurement is "cohesion" as in "much cohesion" -- no cohesion, low/weak cohesion, high/strong cohesion - it's an uncountable noun, isn't it: "25 cohesions?" when the level of cohesion is important. Probably one can derive "cohesions" when referring to "cohesion in many files". Argh :-)

[...]

I notice this is your only package review request. Have you done any reviews yet?

The submitter is already sponsored, so I'll remove this from the NEEDSPONSOR list.  However there was no response to the above commentary in 15 months so I don't know if there's still any desire to get this into Fedora.  Please let me know.

To be fair, I have no longer use for this software; closing this bug.