Hide Forgot
Spec URL: http://dl.dropbox.com/u/420606/fedora-packages/deheader.spec SRPM URL: http://dl.dropbox.com/u/420606/fedora-packages/deheader-0.6-1.fc13.src.rpm Description: (from package description) deheader analyzes C and C++ files to determine which header inclusions can be removed while still allowing them to compile. This may result in substantial improvements in compilation time, especially on large C++ projects; it also sometimes exposes dependencies and cohesions of which developers were unaware. project page: http://www.catb.org/~esr/deheader I find this tool quite useful and decided I would prefer to see this included in fedora :) This is my first package submitted to fedora and I am looking for sponsor.
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.