Note: This is a beta release of Red Hat Bugzilla 5.0. The data contained within is a snapshot of the live data so any changes you make will not be reflected in the production Bugzilla. Also email is disabled so feel free to test any aspect of the site that you want. File any problems you find or give feedback here.
Bug 677043 (deheader) - Review Request: deheader - tool to find unnecesary includes in C/C++ source files
Summary: Review Request: deheader - tool to find unnecesary includes in C/C++ source f...
Keywords:
Status: CLOSED WONTFIX
Alias: deheader
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
unspecified
medium
Target Milestone: ---
Assignee: Nobody's working on this, feel free to take it
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks: FE-DEADREVIEW
TreeView+ depends on / blocked
 
Reported: 2011-02-12 20:44 UTC by Patryk Obara
Modified: 2013-03-13 04:27 UTC (History)
6 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2012-07-11 13:04:47 UTC


Attachments (Terms of Use)

Description Patryk Obara 2011-02-12 20:44:06 UTC
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 20:58:43 UTC
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 22:51:18 UTC
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 17:03:26 UTC
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

Comment 4 Patryk Obara 2011-02-19 23:43:40 UTC
$ 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 ;)

Comment 5 Michael Schwendt 2011-04-01 13:32:26 UTC
> 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?

Comment 6 Jason Tibbitts 2012-07-03 22:51:10 UTC
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 13:04:47 UTC
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.