Bug 677043 - (deheader) Review Request: deheader - tool to find unnecesary includes in C/C++ source files
Review Request: deheader - tool to find unnecesary includes in C/C++ source f...
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
All Linux
unspecified Severity medium
: ---
: ---
Assigned To: Nobody's working on this, feel free to take it
Fedora Extras Quality Assurance
Depends On:
  Show dependency treegraph
Reported: 2011-02-12 15:44 EST by Patryk Obara
Modified: 2013-03-13 00:27 EDT (History)
6 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Last Closed: 2012-07-11 09:04:47 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---

Attachments (Terms of Use)

  None (edit)
Description Patryk Obara 2011-02-12 15:44:06 EST
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.
Comment 1 Patryk Obara 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.
Comment 2 Christopher Aillon 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 :-)
Comment 3 Patryk Obara 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:
Comment 4 Patryk Obara 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

I don't see area for improvement in this small specfile any more ;)
Comment 5 Michael Schwendt 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

$ 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

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?
Comment 6 Jason Tibbitts 2012-07-03 18:51:10 EDT
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.
Comment 7 Patryk Obara 2012-07-11 09:04:47 EDT
To be fair, I have no longer use for this software; closing this bug.

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