[Bug 677043] New: Review Request: deheader - tool to find unnecesary includes in C/C++ source files
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
Summary: Review Request: deheader - tool to find unnecesary includes in C/C++ source files
https://bugzilla.redhat.com/show_bug.cgi?id=677043
Summary: Review Request: deheader - tool to find unnecesary includes in C/C++ source files Product: Fedora Version: rawhide Platform: All OS/Version: Linux Status: NEW Severity: medium Priority: unspecified Component: Package Review AssignedTo: nobody@fedoraproject.org ReportedBy: dreamer.tan@gmail.com QAContact: extras-qa@fedoraproject.org CC: notting@redhat.com, fedora-package-review@redhat.com Estimated Hours: 0.0 Classification: Fedora
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.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=677043
Patryk Obara dreamer.tan@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Blocks| |177841(FE-NEEDSPONSOR)
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=677043
Patryk Obara dreamer.tan@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |dreamer.tan@gmail.com
--- Comment #1 from Patryk Obara dreamer.tan@gmail.com 2011-02-12 15:58:43 EST --- 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.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=677043
Christopher Aillon caillon@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |caillon@redhat.com
--- Comment #2 from Christopher Aillon caillon@redhat.com 2011-02-15 17:51:18 EST --- 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 :-)
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=677043
--- Comment #3 from Patryk Obara dreamer.tan@gmail.com 2011-02-19 12:03:26 EST --- 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
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=677043
--- Comment #4 from Patryk Obara dreamer.tan@gmail.com 2011-02-19 18:43:40 EST --- $ 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 ;)
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=677043
Patryk Obara dreamer.tan@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Alias| |deheader
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=677043
--- Comment #5 from Michael Schwendt mschwendt@gmail.com 2011-04-01 09:32:26 EDT ---
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?
package-review@lists.fedoraproject.org
-
Red Hat Bugzilla