Bug 219610 - Review Request: uncrustify - code beautifier
Review Request: uncrustify - code beautifier
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
All Linux
medium Severity medium
: ---
: ---
Assigned To: Ed Hill
Fedora Package Reviews List
Depends On:
  Show dependency treegraph
Reported: 2006-12-14 08:55 EST by Neal Becker
Modified: 2007-11-30 17:11 EST (History)
2 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Last Closed: 2007-01-03 14:25:29 EST
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 Neal Becker 2006-12-14 08:55:53 EST
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 17:23:20 EST
Hi Neil, here are some observations and I'll post a more thorough review 
as soon as I get a chance:

 + source matches upstream
 + builds in mock on FC6 i386

 - rpmlint reports:
     W: uncrustify summary-not-capitalized reformat source
     W: uncrustify invalid-license gpl
Comment 2 Gwyn Ciesla 2006-12-21 08:28:08 EST
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
Comment 3 Neal Becker 2006-12-21 08:33:15 EST
Yes, I updated the files.  I'm sorry for any confusion.
Comment 4 Ed Hill 2006-12-21 08:47:48 EST
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 08:54:50 EST
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 08:59:55 EST
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 09:01:11 EST
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 09:04:40 EST
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 09:36:23 EST
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-25 23:19:52 EST
$ sha1sum uncrustify-0.30-1.fc6.src.rpm  
d4a0326889cd994e29422723fd9be1517e41edc8  uncrustify-0.30-1.fc6.src.rpm

 + 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.

Comment 11 Christian Iseli 2007-01-02 19:45:40 EST
Changed summary for tracking purposes.
Comment 12 Ed Hill 2007-01-03 22:06:24 EST
Hi Neal, I don't mean to be picky but packages that are successfully 
built and pushed:


should (please) be closed as NEXTRELEASE (not UPSTREAM).  Thanks!
Comment 13 Neal Becker 2007-01-04 07:05:44 EST
Yes, that was unintentional.
Comment 14 Christian Iseli 2007-01-04 07:24:52 EST
oh well... :-)

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