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... :-) |