Bug 193480 - Review Request: sunifdef
Summary: Review Request: sunifdef
Status: CLOSED NEXTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review   
(Show other bugs)
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Ralf Corsepius
QA Contact: Fedora Package Reviews List
URL:
Whiteboard:
Keywords:
Depends On:
Blocks: FE-ACCEPT
TreeView+ depends on / blocked
 
Reported: 2006-05-29 14:29 UTC by Jonathan Underwood
Modified: 2010-04-07 14:45 UTC (History)
2 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2006-06-07 16:01:05 UTC
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---


Attachments (Terms of Use)

Description Jonathan Underwood 2006-05-29 14:29:55 UTC
Spec URL: http://physics.open.ac.uk/~ju83/sunifdef.spec
SRPM URL: http://physics.open.ac.uk/~ju83/sunifdef-1.0-1.src.rpm
Description: 
Sunifdef is a commandline tool for simplifying the preprocessor conditionals in
source code (\#if and related directives) based on the the user's chosen
interpretation of the preprocessor symbols. It is a more powerful successor to
the FreeBSD 'unifdef' tool.

Comment 1 Parag AN(पराग) 2006-06-01 06:03:51 UTC
SPEC looks OK but SOURCE compilations gives some warnings

Comment 2 Ralf Corsepius 2006-06-01 07:03:17 UTC
# rpmlint sunifdef-*
E: sunifdef description-line-too-long Sunifdef is most useful to developers of
constantly evolving products with large
E: sunifdef-debuginfo script-without-shellbang
/usr/src/debug/sunifdef/src/ptr_vector.c
E: sunifdef-debuginfo wrong-script-end-of-line-encoding
/usr/src/debug/sunifdef/src/ptr_vector.c
E: sunifdef-debuginfo script-without-shellbang
/usr/src/debug/sunifdef/src/ptr_vector.h
E: sunifdef-debuginfo wrong-script-end-of-line-encoding
/usr/src/debug/sunifdef/src/ptr_vector.h
E: sunifdef-debuginfo script-without-shellbang
/usr/src/debug/sunifdef/src/state_utils.c
E: sunifdef-debuginfo wrong-script-end-of-line-encoding
/usr/src/debug/sunifdef/src/state_utils.c

=> Shorten your description

=> Wrong permissions on source files.
Running
find \( -name '*.c' -o -name '*.h' \) -exec chmod -x {} \;
inside of %prep fixes this.

Further issues:
* The "source-tarball" ships an autom4te.cache, i.e. it is rather carelessly
packaged. 
I recommend to remove this autom4te.cache in %prep, in advance to running
configure. I.e. you might consider to
rm -rf autom4te.cache
in %prep

* The "source-tarball" ships a precompiled sunifdef linux binary.
I recommend to remove it in %prep, i.e. consider to add
rm -rf build-bin
to %prep

* The sources contain a test suite. I recommend to add a %check section to the
spec to run it:
%check
make check

* Many of the warnings are "punned pointer" warnings. These should not be taken
lightly. They are an indication that a package contains bad code that can causem
"random" problems caused by side-effects of compiler optimization.

Comment 3 Jonathan Underwood 2006-06-01 11:10:35 UTC
Thanks Ralf and Parag for your comments - I will implement all your suggestions
at the weekend.

Regarding the compilation warnings - I have already been in touch with the
author about these, and he's taking a look. I'm in the process of looking
through them myself too, to see if I can offer fixes.

Comment 4 Ralf Corsepius 2006-06-01 13:03:03 UTC
(In reply to comment #3)
> Regarding the compilation warnings - I have already been in touch with the
> author about these, and he's taking a look. I'm in the process of looking
> through them myself too, to see if I can offer fixes.
Note: These warnings aren't blockers. I am willing to approve the package
without them being fixed, once the testsuite is enabled.


Comment 5 Jonathan Underwood 2006-06-02 20:02:02 UTC
Updated spec and SRPM reflecting all of Ralf's comments:

Spec URL: http://physics.open.ac.uk/~ju83/sunifdef.spec
SRPM URL: http://physics.open.ac.uk/~ju83/sunifdef-1.0-2.src.rpm

Regarding the warnings - the package author has got back to me saying he's fixed
them and will make a release soon, at which point I'll update the source.

Comment 6 Ralf Corsepius 2006-06-03 01:43:11 UTC
Please put the "make check" into a %check section

--- sunifdef.spec~      2006-06-03 03:30:41.000000000 +0200
+++ sunifdef.spec       2006-06-03 03:30:41.000000000 +0200
@@ -33,6 +33,8 @@
 %build
 %configure
 make %{?_smp_mflags}
+
+%check
 make check

 %install


Comment 7 Jonathan Underwood 2006-06-03 15:12:22 UTC
Done. I had missed the point about %check before, sorry. I didn't actually know
%check existed. Live and learn.

Spec URL: http://physics.open.ac.uk/~ju83/sunifdef.spec
SRPM URL: http://physics.open.ac.uk/~ju83/sunifdef-1.0-3.src.rpm

Comment 8 Ralf Corsepius 2006-06-05 02:09:32 UTC
APPROVED


Comment 9 Ralf Corsepius 2006-06-07 15:58:10 UTC
Jonathan, would you please close this PR, once the package has been pushed to
the repos (Which AFAIS, already happened), TIA.


Comment 10 Eric Smith 2010-04-07 02:29:40 UTC
Package Change Request
======================
Package Name: unifdef
New Branches: F-13
Owners: brouhaha

Comment 11 Jonathan Underwood 2010-04-07 10:03:18 UTC
Eric - what?

Comment 12 Eric Smith 2010-04-07 14:45:21 UTC
Sorry, meant to attach that to the unifdef review request, not sunifdef.  Please disregard.


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