Bug 677043 (deheader)

Summary: Review Request: deheader - tool to find unnecesary includes in C/C++ source files
Product: [Fedora] Fedora Reporter: Patryk Obara <pobara>
Component: Package ReviewAssignee: Nobody's working on this, feel free to take it <nobody>
Status: CLOSED WONTFIX QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: medium Docs Contact:
Priority: unspecified    
Version: rawhideCC: bgeorges, fedora-package-review, jrb, notting, pobara, tibbs
Target Milestone: ---   
Target Release: ---   
Hardware: All   
OS: Linux   
Whiteboard:
Fixed In Version: Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2012-07-11 09:04:47 EDT Type: ---
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
Bug Depends On:    
Bug Blocks: 201449    

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:
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 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 ;)
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
/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 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.