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
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
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?
Yes, I updated the files. I'm sorry for any confusion.
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.
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.
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?
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.
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.
I've played around with it, and am close, but I couldn't get it to compile. Not worth further time.
$ 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.
Changed summary for tracking purposes.
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!
Yes, that was unintentional.
oh well... :-)