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
Summary: | Review Request: uncrustify - code beautifier | ||
---|---|---|---|
Product: | [Fedora] Fedora | Reporter: | Neal Becker <ndbecker2> |
Component: | Package Review | Assignee: | Ed Hill <ed> |
Status: | CLOSED NEXTRELEASE | QA Contact: | Fedora Package Reviews List <fedora-package-review> |
Severity: | medium | Docs Contact: | |
Priority: | medium | ||
Version: | rawhide | CC: | gwync, panemade |
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: | 2007-01-03 19:25:29 UTC | Type: | --- |
Regression: | --- | Mount Type: | --- |
Documentation: | --- | CRM: | |
Verified Versions: | Category: | --- | |
oVirt Team: | --- | RHEL 7.3 requirements from Atomic Host: | |
Cloudforms Team: | --- | Target Upstream Version: | |
Embargoed: | |||
Bug Depends On: | |||
Bug Blocks: | 163779 |
Description
Neal Becker
2006-12-14 13:55:53 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 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... :-) |