[Bug 207896] Review Request: astyle - Source code formatter

[bugzilla at redhat.com]

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 at dux-linux.org  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 at buildbox ~]$ rpmlint /sw/BUILDING/RPMS/i386/astyle-1.20.2-2.fc7.i386.rpm 
[a at buildbox ~]$ 

[a at buildbox ~]$ rpmlint --version
rpmlint version 0.80 Copyright (C) 1999-2006 Frederic Lepied, Mandriva
[a at 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.