Note: This is a public test instance of Red Hat Bugzilla. The data contained within is a snapshot of the live data so any changes you make will not be reflected in the production Bugzilla. Email is disabled so feel free to test any aspect of the site that you want. File any problems you find or give feedback at bugzilla.redhat.com.
Bug 219610 - Review Request: uncrustify - code beautifier
Summary: Review Request: uncrustify - code beautifier
Keywords:
Status: CLOSED NEXTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Ed Hill
QA Contact: Fedora Package Reviews List
URL:
Whiteboard:
Depends On:
Blocks: FE-ACCEPT
TreeView+ depends on / blocked
 
Reported: 2006-12-14 13:55 UTC by Neal Becker
Modified: 2007-11-30 22:11 UTC (History)
2 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2007-01-03 19:25:29 UTC
Type: ---
Embargoed:


Attachments (Terms of Use)

Description Neal Becker 2006-12-14 13:55:53 UTC
Spec URL: http://nbecker.dyndns.org:8080/RPM/uncrustify.spec
SRPM URL: http://nbecker.dyndns.org:8080/RPM/uncrustify-0.30-1.fc6.src.rpm
Description: Source Code Beautifier for C, C++, C#, D, Java, and Pawn

Comment 1 Ed Hill 2006-12-14 22:23:20 UTC
Hi Neil, here are some observations and I'll post a more thorough review 
as soon as I get a chance:

good:
 + source matches upstream
 + builds in mock on FC6 i386

needswork:
 - rpmlint reports:
     W: uncrustify summary-not-capitalized reformat source
     W: uncrustify invalid-license gpl

Comment 2 Gwyn Ciesla 2006-12-21 13:28:08 UTC
I just downloaded, build and rpmlinted this.  I get none of the above issues. 
Neal, have you made changes to the available files since 12/14, or am I missing
something?

Comment 3 Neal Becker 2006-12-21 13:33:15 UTC
Yes, I updated the files.  I'm sorry for any confusion.

Comment 4 Ed Hill 2006-12-21 13:47:48 UTC
Hi Neal, when you update the files *please* always increase the EVR and 
please post a comment with a link to the new SRPM.  I find it quite 
confusing when people skip those two steps.

I'm very busy today/tomorrow but will make an effort to finish the review 
on Saturday.

Comment 5 Gwyn Ciesla 2006-12-21 13:54:50 UTC
That explains quite a bit. :) I would have envisioned a bumped release number
and new URLs.

Would it not be helpful to include a copy of the default config
(http://uncrustify.sourceforge.net/default.cfg) as ~/.uncrustify.cfg?  Running
it right after installation results in a complaint, and you can't even
uncrustify --show-config -o ~/.uncrustify.cfg without it already existing.

If I manually create ~/.uncrustify.cfg from default.cfg, it works fine.

Comment 6 Neal Becker 2006-12-21 13:59:55 UTC
I agree that's a problem.  (I didn't write this code :))

Anyway, how would that recommendation work? (I don't think it would).

Here is my proposal, which I think is consistent with other Fedora apps.  On 
install, a message is printed telling the user they need to do this.  OK?

Comment 7 Gwyn Ciesla 2006-12-21 14:01:11 UTC
After thinking for two seconds, I realized my above comment won't work.  I think
including default.cfg as /etc/uncrustify.cfg, and having it check that first,
then override with ~/uncrustify.cfg, would be better.

Comment 8 Neal Becker 2006-12-21 14:04:40 UTC
That is really how it should work, but I didn't write it.  If nobody has 
strong objections, I think I'd rather just print the message then spend time 
figuring out how to do fix the code.

Comment 9 Gwyn Ciesla 2006-12-21 14:36:23 UTC
I've played around with it, and am close, but I couldn't get it to compile.  Not
worth further time.

Comment 10 Ed Hill 2006-12-26 04:19:52 UTC
$ sha1sum uncrustify-0.30-1.fc6.src.rpm  
d4a0326889cd994e29422723fd9be1517e41edc8  uncrustify-0.30-1.fc6.src.rpm

good:
 + builds in mock for FC6 i386
 + rpmlint is silent
 + license is OK and correctly included
 + spec is very simple and clean
 + no shared libs and no *.la
 + perms and dir ownership OK
 + %clean OK

could be improved:
 - the "Summary: Reformat source" line is a little ambiguous and it
   might be more helpful to say "Tool for source code reformatting"
   or similar -- but that's just a suggestion, not a blocker

Its unfortunate how the config files are handled but I agree that it 
is an upstream problem.

APPROVED.


Comment 11 Christian Iseli 2007-01-03 00:45:40 UTC
Changed summary for tracking purposes.


Comment 12 Ed Hill 2007-01-04 03:06:24 UTC
Hi Neal, I don't mean to be picky but packages that are successfully 
built and pushed:

  http://buildsys.fedoraproject.org/build-status/job.psp?uid=24978

should (please) be closed as NEXTRELEASE (not UPSTREAM).  Thanks!

Comment 13 Neal Becker 2007-01-04 12:05:44 UTC
Yes, that was unintentional.

Comment 14 Christian Iseli 2007-01-04 12:24:52 UTC
oh well... :-)



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