Bug 207896

Summary: Review Request: astyle - Source code formatter
Product: [Fedora] Fedora Reporter: Mary Ellen Foster <mefoster>
Component: Package ReviewAssignee: Mamoru TASAKA <mtasaka>
Status: CLOSED ERRATA QA Contact: Fedora Package Reviews List <fedora-package-review>
Severity: medium Docs Contact:
Priority: medium    
Version: rawhideCC: gnome, haircut, kevin, mtasaka
Target Milestone: ---Flags: mtasaka: fedora-review+
kevin: fedora-cvs+
Target Release: ---   
Hardware: All   
OS: Linux   
Whiteboard:
Fixed In Version: 1.21-5.fc7 Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2007-07-11 15:20:12 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:
Attachments:
Description Flags
specfile for v1.20.2
none
patch against 1.20.2-1.spec
none
Mock Build Log
none
Mock Root Log none

Description Mary Ellen Foster 2006-09-25 08:11:02 UTC
Spec URL: http://www6.informatik.tu-muenchen.de/~foster/extras/astyle.spec
SRPM URL: http://www6.informatik.tu-muenchen.de/~foster/extras/astyle-1.19-1.src.rpm

Description:
Artistic Style is a source code indenter, source code formatter, and
source code beautifier for the C, C++, C# and Java programming
languages. The project homepage is http://astyle.sourceforge.net/.

NB: I'm still waiting for a response from the developer about whether he's okay
with this being packaged, but I figured I'd get the review process started anyway.

This is my first package, and I am looking for a sponsor.

Comment 1 Mary Ellen Foster 2006-09-25 16:20:35 UTC
p.s. -- Just heard back from the developer, and he's fine with this being in Extras.

Comment 2 Nigel Jones 2007-04-27 09:33:01 UTC
Ping?  (Also adding FE-NEW & FE-SPONSOR so someone can find this package)

Comment 3 Adam Monsen 2007-05-12 21:48:15 UTC
Created attachment 154591 [details]
specfile for v1.20.2

I needed a specfile for 1.20.2 (the current release), so I made one. Hope this
helps. The Makefile patch no longer works, so I removed it.

Comment 4 Kevin Fenzi 2007-06-03 03:57:56 UTC
Hey Mary. 

Are you still interested in submitting this package?
If so could you update to the latest version and post an updated spec/src.rpm
link? You may also want to look at: 
http://fedoraproject.org/wiki/PackageMaintainers/HowToGetSponsored

If I don't hear from you in 1 week I will go ahead and close this submission. 

Comment 5 Mary Ellen Foster 2007-06-03 20:28:20 UTC
I am definitely still interested in packaging this, but I'll be travelling 
until next Sunday (10 June). I'll update the package as soon as I get back. 
Thanks for responding! :)

Comment 6 Mary Ellen Foster 2007-06-13 12:41:15 UTC
Okay, I've uploaded the latest spec and srpm to:

http://www6.informatik.tu-muenchen.de/~foster/extras/astyle.spec
http://www6.informatik.tu-muenchen.de/~foster/extras/astyle-1.20.2-1.src.rpm

The spec is basically the revised one sent by Adam Monsen -- thanks! :)

Comment 7 Ralf Corsepius 2007-06-14 08:30:35 UTC
This package will need some pretty intensive love, because its Makefile
(build/Makefile) doesn't really fit into rpm's demands.

MUSTFIX:
* Package doesn't acknowledge RPM_OPT_FLAGS
The cause is before-mentioned Makefile. You need to find a way to correctly pass
RPM_OPT_FLAGS to CFLAGS of this package (More on this below).

* The Makefile strips the installed binary (/usr/bin/astyle)
Packages are not supposed to strip binaries. rpm needs the unstripped binaries
to generate debuginfos from them and strips binaries itself after having
generated the debuginfos

* The spec should use %{_bindir} instead of /usr/bin

* BuildRequires: gcc-c++ libstdc++
Putting them into BuildRequires is frowned upon, because these are defaults
components of the buildsystem. Please remove.

Wrt. build/Makefile: 
Frankly speaking, build/Makefile is in a shape, I'd seriously recommend not to
use it, but to manually compile astyle from inside of the spec. 
This approach would be feasible if you don't plan to also ship the astyle libs.
If you plan to do so, patching build/Makefile would be an appropriate approach.






Comment 8 Ralf Corsepius 2007-06-14 08:32:35 UTC
Created attachment 156965 [details]
patch against 1.20.2-1.spec

This patch contains changes implementing what I wrote in previous comment.

Comment 9 Mary Ellen Foster 2007-06-15 06:53:38 UTC
Thanks for the suggestions; I've incorporated them into the spec (and also 
added a %{dist} to the version -- is that right?)

New stuff here:
http://www6.informatik.tu-muenchen.de/~foster/extras/astyle.spec
http://www6.informatik.tu-muenchen.de/~foster/extras/astyle-1.20.2-2.fc7.src.rpm

Comment 10 Adam M. Dutko 2007-06-18 03:38:12 UTC
Created attachment 157246 [details]
Mock Build Log

Comment 11 Adam M. Dutko 2007-06-18 03:38:32 UTC
Created attachment 157247 [details]
Mock Root Log

Comment 12 Adam M. Dutko 2007-06-18 03:38:55 UTC
General Review -->

MUSTS:

1) RPMLINT: rpmlint produces no output - PASS
2) NAMING GUIDELINES:
      *no underscores in name (since the source contains underscores you could
use underscores if you wanted, do you want to?  If downstream contains them,
which it does, it would be good to be consistent. ) - NEEDINFO
      *spec file name correct - PASS
      *package version matches source and correct - PASS
      *release number correct - PASS
      *%{?dist} used properly - PASS
      *package not case sensitive - PASS
      *no renaming/replacing existing packages - PASS
      *no subpackages included - NEEDINFO

         -- Cannot find astyle library files?
         -- Are the libs he referred to standard C++ includes?
         -- Are the libs source files in src/ ?
         -- It would probably be best to include the libs in another pkg.

      *no addon packages - PASS
3) PACKAGING GUIDELINES:
      *licensing incorrect - CHANGE
         -- astyle is licensed under the LGPL, not GPL
         -- please correct License:
         -- LGPL documentation included as license.html document
      *no known patents - NA 
      *not an emulator - NA
      *no binary firmware - NA
      *inclusion of libs and/or binaries - NEEDINFO (see subpackages)
      *pkg from scratch matches minimal spec except %configure - NEEDINFO
         -- make %{?_smp_mflags}  --> should be included if possible 
         -- because the pkg is compiled manually maybe a g++ switch?
         -- g++ -o astyle $RPM_OPT_FLAGS src/*.cpp could be? - NOT SURE IF
NEEDED  It's a single (and small) source file so you shouldn't need it I'd think.
         -- I don't believe %configure is required - NEEDINFO
      *not modifying an existing spec - NA
      *filesystem layout - PASS
      *rpmlint - PASS
      *changelog - CHANGE
         -- should remove the last changelog comment about a different version
         -- 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
      *buildroot - PASS
      *requires - 
      *prereq - NA
      *file dependencies - NA
      *buildrequires - NEEDINFO
         -- mock produces the following during the debug ... 
            cpio: astyle/<built-in>: No such file or directory
	 -- I am not sure if the above "error" is relevant.
         -- I've attached the mock build log.
         -- builds cleanly in mock.
      *rpmdev-rmdevelrpms - NA
      *summary and description - PASS
      *encoding - PASS
      *non-ascii filenames - NA
      *documentation - PASS
         -- It would be nice to have a brief manpage for this pkg that the
maintainers would incorporate.  
      *compiler flags - UNKNOWN
      *debuginfo pkgs - PASS
      *static linked libraries/executables - NA
      *libraries - NEEDINFO
         -- I think this package might need to be split apart into a libraries
pkg and a program pkg.  I'm not totally sure though, since I'm somewhat "new." 
Please read up on packaging guidelines to determine the best way to get around
the way the makefile is structured. 
(http://fedoraproject.org/wiki/Packaging/Guidelines)
      *duplication of sys libs - NA
      *rpath and removing rpath - NA
      *configuration file - NA
      *init scripts - NA
      *desktop file - NA
      *macros - PASS
      *locale files - NA
      *parallel make - NEEDINFO
         -- because this pkg uses g++ instead of make in the build section and
doesn't have a configure section I don't know how to address this "nice to
have."  Anyone have an idea?
      *timestamps - PASS
      *scriptlets - NA
      *conditional dependencies - NA
      *relocatable pkg - NA
      *code vs. content - PASS
      *file and dir ownership - PASS
      *web app - NA
      *confilcts - NA
4) LICENSING: 
      *need to correct the License: field - CHANGE
      *other than the problem above - PASS
5) LICENSE FIELD:
      *see #4 - CHANGE
6) DOC LICENSE: included in proper dir - PASS
7) SPEC FILE IN ENGLISH: - PASS
8) SPEC FILE LEGIBLE: - PASS
9) SOURCES MATCH UPSTREAM: need to replace last comment - PASS
10) COMPILES/BUILDS: built, and installed on my system (pkg runs) - PASS
11) EXCLUDEARCH: are there any arches it doesn't build for? I assume all are
fine since the comments state any arch that has g++. - NEEDINFO 
12) BUILDREQUIRES: none - PASS
13) LOCALES: none - PASS
14) SHARED LIBS: I think you should strip out the libs and put them in another
pkg and change the makefile, but I'm new so a more experienced reviewer should
chime in please... - NEEDINFO
15) RELOCATABLE PKG: n/a - NA
16) DIR OWNERSHIP: - PASS
17) DUPLICATE FILES: none - PASS
18) PERMS: - PASS
19) CLEAN SECTION: - PASS
20) CONSISTENT MACROS: - PASS
21) CODE/CONTENT: - PASS
22) LARGE DOCS: n/a - NA
23) MISSING DOCS: moved docs from docdir to home dir and binary functioned
properly - PASS
24) HEADER FILES: there is a header in src but I don't think it needs to g into
a -devel pkg - NEEDINFO (from a pkg reviewer)
25) STATIC LIBS: again, needinfo - NEEDINFO
26) PKGCONFIG: n/a - NA
27) LIBS w/ SUFFIXES: again, see #25 - NEEDINFO
28) DEVEL PKG: n/a - NA
29) LIBTOOL ARCHIVES: n/a - NA
30) GUI APP DESKTOP FILE: n/a - NA
31) OWN NON-SPECIFIC DIRS/FILES: no this pkg doesn't - PASS
32) REMOVE BUILDROOT IN INSTALL: - PASS
33) UTF-8 FILENAMES: - PASS


SHOULDS:

1) Includes LGPL license text. - PASS
2) Description short and concise but no none English translations.  This is up
to the maintainer if they desire to have this included.  - NEEDINFO
3) The package builds in mock with one potential issue: - NEEDINFO
     *mock produces the following during the debug stage ... 
            cpio: astyle/<built-in>: No such file or directory
     *I'm not certain this is even an issue b/c the debuginfo pkgs build fine.
4) Package runs but I haven't tested the functionality further than 'astyle
--help'. - PASS
5) Scriptlets are not used. - NA
6) No subpackages. There might be a lib pkg generated but again, that's a topic
up for discussion with more experienced pkg reviewers. - NEEDINFO
7) Pkgconfig not used. - NA
8) No other known file dependencies other than the libs. - NEEDINFO

Comment 13 Mamoru TASAKA 2007-06-18 06:14:51 UTC
(In reply to comment #12)
> 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.

> 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?

>       *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.

>          -- astyle is licensed under the LGPL, not GPL

>       *pkg from scratch matches minimal spec except %configure - NEEDINFO
  For this package this can be ignored IMO
  +1 for Ralf's comment

>       *rpmlint - PASS
   Umm??? Don't pass (see below)

>       *changelog - CHANGE
>          -- should remove the last changelog comment about a different version
   Why?

>          -- 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

>          -- mock produces the following during the debug ... 
>             cpio: astyle/<built-in>: No such file or directory
  Can be ingored

>       *debuginfo pkgs - PASS
  Actually, don't pass (related with rpmlint - see below)

>       *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.

>       *parallel make - NEEDINFO
  This is not needed for this package (g++ *.cpp meets the demand)

> 4) LICENSING: 
>       *need to correct the License: field - CHANGE

* 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.

* Macros
  - /usr/bin/install <- use macros for /usr/bin.

* 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.


Comment 14 Ralf Corsepius 2007-06-18 06:48:49 UTC
(In reply to comment #13)
> (In reply to comment #12)
> > 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.
FWIW: I find this style of review unreadable and not to be actually helpful.



Comment 15 Adam M. Dutko 2007-06-18 11:49:29 UTC
> 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.  

Comment 16 Ralf Corsepius 2007-06-18 12:12:45 UTC
(In reply to comment #15)
> > 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.
Let me put it this way: I find this style of reviews WAY too verbose to be
useful. Submitters and reviewers are drowning in irrelevant (and partially
bogus) comments.

I for one only see 2 remaining issues:
- GPL -> LGPL
- Bogus permissions on source files.
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

=> Add chmod -x src/*
to %prep

All the rest is stylishness, not fixing anything.


Comment 17 Adam M. Dutko 2007-06-18 12:46:37 UTC
(In reply to comment #16)
> (In reply to comment #15)
> > > 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.
> Let me put it this way: I find this style of reviews WAY too verbose to be
> useful. Submitters and reviewers are drowning in irrelevant (and partially
> bogus) comments.

    I'm learning and appreciate the critique.  In the future I will try to be
more specific, and instead of listing out all of the checks I did, only list the
ones that need changing; is that the style you are hinting at?  :-)

> I for one only see 2 remaining issues:
> - GPL -> LGPL

     Check.

> - Bogus permissions on source files.
> 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


      Again, I'm not getting this with rpmlint.  Can you let me know what
version of rpmlint you have please? and what version of Fedora/RHEL you're using
to build?  Thanks.

> => Add chmod -x src/*
> to %prep

      Check.
 
> All the rest is stylishness, not fixing anything.

    Ok.  :-)  Some of the points were raised for personal edification.  :-)  I
will ask those in a different forum instead of through this review.  The reason
I asked them here in the first place was so everyone on this list could learn too.

Comment 18 Mamoru TASAKA 2007-06-18 13:39:58 UTC
(In reply to comment #15)
> >       *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 ~]$ 
Well, when you rebuild srpm (by rpmbuild or mock), you should also
have debuginfo rpm. For this package, there should be
astyle-debuginfo rpm. Also, we have to check rpmlint error for
src.rpm and the _installed_ rpm.

> >       *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.  
This is for the *newest* (i.e. top) entry of the %changelog.

> 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.
Well, please try "rpm -q --changelog coreutils", for example.

BTW as this is NEEDSPONSOR blocker, will someone going to sponsor
the reviewer, Ralf, Kevin (and me)?

Comment 19 Adam M. Dutko 2007-06-18 14:03:55 UTC
(In reply to comment #18)
> (In reply to comment #15)
> > >       *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 ~]$ 
> Well, when you rebuild srpm (by rpmbuild or mock), you should also
> have debuginfo rpm. For this package, there should be
> astyle-debuginfo rpm. Also, we have to check rpmlint error for
> src.rpm and the _installed_ rpm.

    So I need to run rpmlint not only on the primary rpm, but also on the debug
rpms and etc.  Thank you for clarifying.  :-)

> > >       *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.  
> This is for the *newest* (i.e. top) entry of the %changelog.

     Good to know.  I was under the wrong impression that versioning needed to
be consistent with the pkg: I stand corrected.  Thank you.


> > 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.
> Well, please try "rpm -q --changelog coreutils", for example.

     Good to know.  Thank you.
 
> BTW as this is NEEDSPONSOR blocker, will someone going to sponsor
> the reviewer, Ralf, Kevin (and me)?


Comment 20 Ralf Corsepius 2007-06-18 16:38:57 UTC
(In reply to comment #19)
> > BTW as this is NEEDSPONSOR blocker, will someone going to sponsor
> > the reviewer, Ralf, Kevin (and me)?
Erm, ... the submitter needs to be sponsored, not the reviewer :)

Mary, do you have any other packages of yours to be reviewed?


Comment 21 Mamoru TASAKA 2007-06-18 16:58:22 UTC
(In reply to comment #20)
> (In reply to comment #19)
> > > BTW as this is NEEDSPONSOR blocker, will someone going to sponsor
> > > the reviewer, Ralf, Kevin (and me)?
> Erm, ... the submitter needs to be sponsored, not the reviewer :)

Oops...

Comment 22 Mary Ellen Foster 2007-06-18 17:42:08 UTC
(In reply to comment #20)
> Mary, do you have any other packages of yours to be reviewed?

Yes, and it's a bit bigger: I've also submitted Ice (object middleware). The 
review bug is here:
    https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=234612

I'm slightly drowning in all of the recent comments, but I'll try to get a new 
astyle package out today or tomorrow. :)

Comment 23 Adam M. Dutko 2007-06-20 02:25:34 UTC
OK.  I have a new spec, new debug,regular and source rpms up at:

http://littlehat.homelinux.org:8000/FEDORA/RPMS/astyle/

I figured it was the least I could do for lobbing all the info onto this bug at
once.  Sorry Mary.  :-/  

I implemented all of Ralf's suggestions, but did not remove the html page Mamoru
requested.  If that's a definite I will, just let me know.

rpmlint passed on the SRPM,i386-RPM and DEBUG rpm.

Comment 24 Mamoru TASAKA 2007-06-27 16:14:44 UTC
Well, as no one is reviewing, assigning to me...

Comment 25 Mamoru TASAKA 2007-06-28 17:52:03 UTC
* SourceURL
  - As this is sourceforge source, please follow:
    the section "Sourceforge.net" of
    http://fedoraproject.org/wiki/Packaging/SourceURL

* Newest version
  - It seems 1.21 is released on Jun 21... So please update.

Comment 26 Adam M. Dutko 2007-06-29 14:46:14 UTC
>* SourceURL
>  - As this is sourceforge source, please follow:
>    the section "Sourceforge.net" of
>    http://fedoraproject.org/wiki/Packaging/SourceURL

Source0: http://dl.sourceforge.net/%{name}/%{name}_%{version}_linux.tar.gz

will change to 
Source0: http://downloads.sourceforge.net/%{name}/%{name}_%{version}_linux.tar.gz

>* Newest version
>  - It seems 1.21 is released on Jun 21... So please update.

Will grab and rebuild after changing spec file by Monday evening unless Mary
does it before I do.

Comment 27 Adam M. Dutko 2007-07-03 02:26:55 UTC
(In reply to comment #25)
> * SourceURL
>   - As this is sourceforge source, please follow:
>     the section "Sourceforge.net" of
>     http://fedoraproject.org/wiki/Packaging/SourceURL

Fixed in the latest spec.

> * Newest version
>   - It seems 1.21 is released on Jun 21... So please update.

Fixed in the latest spec and build.

Please see...  http://littlehat.homelinux.org:8000/FEDORA/RPMS/astyle/current/


Comment 28 Mamoru TASAKA 2007-07-03 17:56:56 UTC
By the way, who will become the maintainer of this package?
Adam, are you working together with Mary?

Comment 29 Adam M. Dutko 2007-07-03 20:42:16 UTC
I haven't heard from Mary.  If she is unable to maintain the package I will. Can
you please chime in Mary and let us know either way?  Thanks. 

Comment 30 Adam Monsen 2007-07-03 21:59:21 UTC
I've also not heard from Mary. I don't have a formal working relationship with
Mary, I just wanted an updated astyle RPM package so I helped out a little. I'm
happy to be a 3rd string maintainer. :)

Comment 31 Mamoru TASAKA 2007-07-04 14:09:47 UTC
Mary, how do you think about maintainership?
Also, I want you to update bug 234612 .

Comment 32 Mary Ellen Foster 2007-07-04 14:19:18 UTC
I'd be happy for Adam to take over astyle. I'll reply on the Ice review shortly.
I'm sorry, I've been travelling to conferences for the last two weeks; I'm back
now and have more time to devote to this stuff again.

Comment 33 Adam M. Dutko 2007-07-04 14:33:11 UTC
Mamoru, how shall we all proceed?

Comment 34 Mamoru TASAKA 2007-07-04 14:40:03 UTC
Well, I must clarify who will maintain this package.


Monsen and Dutko, do you want to maintain this package?
And will you also want to maintain this package, Mary?

Comment 35 Adam M. Dutko 2007-07-04 14:49:00 UTC
I'm willing to either co-maintain with Foster and Monsen or be primary, or be
secondary; it's really up to what they'd prefer.  Again, I'm OK with any scenario.

Comment 36 Mamoru TASAKA 2007-07-05 10:37:38 UTC
OKay. Now I approve this package.

--------------------------------------------------
  This package (astyle) is APPROVED by me
--------------------------------------------------

Dukto, please become the maintainer of this package.
Monsen and Mary, if you also want to maintain this package,
let us know on this bug ticket.

Once removing NEEDSPONSOR blocker

Comment 37 Adam M. Dutko 2007-07-05 14:38:12 UTC
(In reply to comment #36)
> OKay. Now I approve this package.
> 
> --------------------------------------------------
>   This package (astyle) is APPROVED by me
> --------------------------------------------------
> 
> Dukto, please become the maintainer of this package.
> Monsen and Mary, if you also want to maintain this package,
> let us know on this bug ticket.
> 

I will build for FC-6, F-7 and devel and also register for CVS inclusion this
evening.  Then once verified and built, push through Bodhi.
> Once removing NEEDSPONSOR blocker



Comment 38 Adam M. Dutko 2007-07-06 00:15:47 UTC
New Package CVS Request
=======================
Package Name: astyle
Short Description: Source code formatter for C-like programming languages
Owners: gnome
Branches: FC6 F7
InitialCC: mtasaka.u-tokyo.ac.jp mefoster haircut

Comment 39 Kevin Fenzi 2007-07-06 00:41:12 UTC
cvs done.

Comment 40 Adam M. Dutko 2007-07-06 02:21:49 UTC
FC-6 PLAGUE BUILD DONE:
http://buildsys.fedoraproject.org/build-status/job.psp?uid=34797

FC-7 KOJI BUILD FAILED: 
BuildError: package astyle not in list for tag dist-fc7-updates-candidate

I think it needs to be promoted to fc7-updates-candidate?

F-8 KOJI BUILD FAILED:

[a@buildbox devel]$ make build
astyle.spec not tagged with tag astyle-1_21-5_fc8
make: *** [build-check] Error 1

I tried to remove from cvs, then readd and even when I do make tag build, it
says the tag already exists...  I don't see how to remove the tag after it's
been created.  I'm sleepy so please forgive me if I'm overlooking something
simple.  Any ideas?

Comment 41 Mamoru TASAKA 2007-07-06 02:39:48 UTC
You may have to wait for about one or two hours...

Note: I live in Japan and now it is 11:40 AM.

Comment 42 Mamoru TASAKA 2007-07-06 03:06:49 UTC
Perhaps now you can rebuild astyle on koji.

Comment 43 Adam M. Dutko 2007-07-06 11:59:48 UTC
(In reply to comment #42)
> Perhaps now you can rebuild astyle on koji.

FC-7 KOJI BUILD SUCCEEDED:
http://koji.fedoraproject.org/koji/buildinfo?buildID=10510

F-8 KOJI BUILD FAILED:
[a@buildbox devel]$ make build
astyle.spec not tagged with tag astyle-1_21-5_fc8
make: *** [build-check] Error 1

Not sure what I need to do with the F-8 build failure. I'm thinking I might need
to delete the directory from CVS and then recreate it?

I've pushed astyle in bodhi:

Release:  	Fedora 7
Status: 	pending
Type: 	enhancement
Bugs: 	207896 - Review Request: astyle - Source code formatter
Requested: 	push
Pushed: 	False
Mail Sent: 	False
...
Submitted: 	2007-07-06 04:58:40
Build Logs: http://koji.fedoraproject.org/packages/astyle/1.21/5.fc7/data/logs

The last thing to fix is the devel build issue.  Any ideas?


Comment 44 Jeroen van Meeuwen 2007-07-06 12:07:50 UTC
Hi Adam,

Did you import the src or tarball in the CVS branch devel?

Comment 45 Mamoru TASAKA 2007-07-06 12:14:22 UTC
http://cvs.fedora.redhat.com/viewcvs/rpms/astyle/devel/?root=extras&only_with_tag=astyle-1_21-5_fc8
shows that astyle.spec is not tagged.

The easiest way to fix this is to bump release number and retag.

Comment 46 Adam M. Dutko 2007-07-06 14:13:04 UTC
Will attempt to fix when I get home this evening.  Thank you for the suggestions.

Comment 47 Fedora Update System 2007-07-06 18:10:53 UTC
astyle-1.21-5.fc7 has been pushed to the Fedora 7 testing repository.  If problems still persist, please make note of it in this bug report.

Comment 48 Adam M. Dutko 2007-07-07 17:28:45 UTC
Build for F-8 branch completed successfully:  

http://koji.fedoraproject.org/koji/buildinfo?buildID=10605

All branches built, and F-7 in testing repository.  DONE.

Comment 49 Fedora Update System 2007-07-11 15:19:54 UTC
astyle-1.21-5.fc7 has been pushed to the Fedora 7 stable repository.  If problems still persist, please make note of it in this bug report.