Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report. Summary: Review Request: astyle - Source code formatter https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=207896 ------- Additional Comments From gnome@xxxxxxxxxxxxx 2007-06-18 07:49 EST ------- > General Review --> >Well, If you want to do the review of this style, please make >the summary of the review so that everyone (including submitter) can >read your review easily. Hmmm... I thought I was being very thorough in my review. I followed every aspect of the review process to the "T," but from now on will not post everything at once. > 2) NAMING GUIDELINES: > *no underscores in name (since the source contains underscores you could > use underscores if you wanted, do you want to? > But the name "astyle" has no underscore.. I don't know what > is the problem with the name of this pkgname? There aren't any underscores in the name hence the comment "no underscores in the name...". However, the source package has underscores so I simply stated she could, if she wanted, use them because it matches one of the listed exceptions to the rule. > *no subpackages included - NEEDINFO > -- It would probably be best to include the libs in another pkg. > This can be left to how the submittter judges. Great! Thank you for answering my question, as I wasn't sure about this one. > -- astyle is licensed under the LGPL, not GPL Yes. As I highlighted she needs to correct this portion. > *pkg from scratch matches minimal spec except %configure - NEEDINFO > For this package this can be ignored IMO > +1 for Ralf's comment Great! I wasn't sure if there was a precedent for this and it's good to know something can be packaged in this manner. Thank you for the answer. > *rpmlint - PASS > Umm??? Don't pass (see below) This is very strange indeed b/c the rpmlint passes for me with flying colors. [a@buildbox ~]$ rpmlint /sw/BUILDING/RPMS/i386/astyle-1.20.2-2.fc7.i386.rpm [a@buildbox ~]$ [a@buildbox ~]$ rpmlint --version rpmlint version 0.80 Copyright (C) 1999-2006 Frederic Lepied, Mandriva [a@buildbox ~]$ I'm using F7 as a build host. > *changelog - CHANGE > -- should remove the last changelog comment about a different version > Why? Why? Because it's stated in the guidelines that changelog comments should match the version of the pkg a person is packaging. If she wants to pkg a different version, she should have the comments for that version in its' spec file, and not in this one. > -- if another pkg exists with that version number then you should put > the comments in its' spec file > -- comment for initial version should match version of the pkg and > still exist so history of the pkg is maintained > Currently I don't understand what you want to say here The changelog contains comments for another version of this program. It is potentially confusing for a maintainer to have two versions of a pkg mentioned in the same spec file. I thought each version of pkg should have it's own rpm and spec file, am I incorrect? What I asked her to do was to replace the comment with one for the proper version like so: * Thu Sep 21 2006 Mary Ellen Foster <mefoster gmail com> 1.20.2-1 - Initial package > -- mock produces the following during the debug ... > cpio: astyle/<built-in>: No such file or directory > Can be ingored Great! Thanks for the info. > *debuginfo pkgs - PASS > Actually, don't pass (related with rpmlint - see below) Again, I had no problems with rpmlint. Am I doing something incorrectly? > *libraries - NEEDINFO > -- I think this package might need to be split apart into a libraries > pkg and a program pkg. > IMO this is not needed for this package. Great! Thanks for the info. Can you explain why a separate lib package isn't needed beyond what's in the documentation? > *parallel make - NEEDINFO > This is not needed for this package (g++ *.cpp meets the demand) So g++ will auto-optimize the job? Or do we not need it b/c they're individual source files already and will have their own job at compile time? > 4) LICENSING: > *need to correct the License: field - CHANGE Yes it needs to be set to License: LGPL. >* For rpmlint: >----------------------------------------------------------- >E: astyle-debuginfo script-without-shebang >/usr/src/debug/astyle/src/astyle_main.cpp >W: astyle-debuginfo spurious-executable-perm /usr/src/debug/astyle/src/astyle.h >E: astyle-debuginfo script-without-shebang >/usr/src/debug/astyle/src/ASEnhancer.cpp >E: astyle-debuginfo script-without-shebang >/usr/src/debug/astyle/src/ASResource.cpp >E: astyle-debuginfo script-without-shebang >/usr/src/debug/astyle/src/ASFormatter.cpp >E: astyle-debuginfo script-without-shebang >/usr/src/debug/astyle/src/ASBeautifier.cpp >------------------------------------------------------------- > - All of these are permission issues. Please fix these. I didn't get any of these errors. Please see my output. >* Macros > - /usr/bin/install <- use macros for /usr/bin. %{_bindir}/install should work nicely. Sorry I missed this... >* Documentation > - Please check if "install.html" is needed for %doc. This seems > to be needed for people who want to rebuild this package by > themselves and does not seem to be needed for rpm users. It contains information on what each component is used for, not just installation/build instructions so it seems relevant. But you're correct, it is more pertinent to those installing from source rather than rpm. I will try to be a little less verbose when doing reviews. Sorry for the length Mamoru and Ralf. -- Configure bugmail: https://bugzilla.redhat.com/bugzilla/userprefs.cgi?tab=email ------- You are receiving this mail because: ------- You are the QA contact for the bug, or are watching the QA contact. _______________________________________________ Fedora-package-review mailing list Fedora-package-review@xxxxxxxxxx http://www.redhat.com/mailman/listinfo/fedora-package-review