Updated to just-released 4.96.

Spec URL: http://home.comcast.net/~ckweyl/perl-POE-Component-IRC.spec
SRPM URL: http://home.comcast.net/~ckweyl/perl-POE-Component-IRC-4.96-0.fc5.src.rpm

BuildArch:      noarch

=> 
...
%{__perl} Makefile.PL INSTALLDIRS=vendor OPTIMIZE="%{optflags}"
...
find %{buildroot} -type f -name '*.bs' -a -size 0 -exec rm -f {} ';'
...
=> remove this find and the OPTIMIZE

Bits snipped to comply with the soon to release spec template.

Spec URL: http://home.comcast.net/~ckweyl/perl-POE-Component-IRC.spec
SRPM URL: http://home.comcast.net/~ckweyl/perl-POE-Component-IRC-4.96-1.fc5.src.rpm

For some reason I had to remove the conditional bits from %check in order to get
this to build or I'd get this:

RPM build errors:
    /builddir/build/SPECS/perl-POE-Component-IRC.spec:58: parseExpressionBoolean
returns -1

and the the entire file list would be ignored, causing the build to fail.  Not
sure what's up here; I thought your syntax was correct.  Anyway, it's simple
remove it to get through the buildsys, or investigate some way to make it work
and it shouldn't hold up the package.

Review:
* source files match upstream:
   e57f2fbcce0aecf07062b8bf83b7bd96  POE-Component-IRC-4.96.tar.gz
* package meets naming and packaging guidelines.
* specfile is properly named, is cleanly written and uses macros consistently.
* dist tag is present.
* build root is correct.
* license field matches the actual license.
* license is open source-compatible.  License text not included upstream.
* latest version is being packaged.
* BuildRequires are proper (BR: perl is unnecessary)
* Compiler flags are appropriate (none for noarch package)
* %clean is present.
X package builds in mock (development, x86_64) (needs above fix)
* rpmlint is silent.
* noarch package; no debuginfo.
* final provides and requires are sane:
   perl(POE::Component::IRC) = 4.96
   perl(POE::Component::IRC::Common) = 4.86
   perl(POE::Component::IRC::Constants)
   perl(POE::Component::IRC::Pipeline) = 0.01
   perl(POE::Component::IRC::Plugin) = 0.08
   perl(POE::Component::IRC::Plugin::BotAddressed)
   perl(POE::Component::IRC::Plugin::BotTraffic)
   perl(POE::Component::IRC::Plugin::CTCP) = 1.0
   perl(POE::Component::IRC::Plugin::Connector)
   perl(POE::Component::IRC::Plugin::Console)
   perl(POE::Component::IRC::Plugin::ISupport) = 0.52
   perl(POE::Component::IRC::Plugin::NickReclaim) = 1.0
   perl(POE::Component::IRC::Plugin::PlugMan)
   perl(POE::Component::IRC::Plugin::Proxy)
   perl(POE::Component::IRC::Plugin::Whois) = 0.02
   perl(POE::Component::IRC::Projects) = 0.01
   perl(POE::Component::IRC::Qnet) = 1.3
   perl(POE::Component::IRC::Qnet::State) = 1.5
   perl(POE::Component::IRC::State) = 1.9
   perl(POE::Component::IRC::Test::Harness) = 0.4
   perl(POE::Component::IRC::Test::Plugin)
   perl(POE::Filter::CTCP)
   perl(POE::Filter::IRC)
   perl(POE::Filter::IRC::Compat) = 1.0
   perl-POE-Component-IRC = 4.96-1.fc6
  =
   perl >= 0:5.006
   perl(:MODULE_COMPAT_5.8.8)
   perl(Carp)
   perl(Data::Dumper)
   perl(Exporter)
   perl(File::Basename)
   perl(POE)
   perl(POE::Component::IRC) >= 4.5
   perl(POE::Component::IRC::Common)
   perl(POE::Component::IRC::Constants)
   perl(POE::Component::IRC::Pipeline)
   perl(POE::Component::IRC::Plugin)
   perl(POE::Component::IRC::Plugin::ISupport)
   perl(POE::Component::IRC::Plugin::Whois)
   perl(POE::Filter::CTCP)
   perl(POE::Filter::IRC)
   perl(POE::Filter::IRC::Compat)
   perl(POE::Filter::IRCD)
   perl(POSIX)
   perl(Socket)
   perl(Symbol)
   perl(base)
   perl(constant)
   perl(strict)
   perl(vars)
   perl(warnings)
* %check is present but tests are necessarily disabled.
* no shared libraries are present.
* package is not relocatable.
* owns the directories it creates.
* doesn't own any directories it shouldn't.
* no duplicates in %files.
* file permissions are appropriate.
* no scriptlets present.
* code, not content.
* documentation is small, so no -docs subpackage is necessary.
* %docs are not necessary for the proper functioning of the package.
* no headers.
* no pkgconfig files.
* no libtool .la droppings.
* not a GUI app.

APPROVED

You guys might want to learn about rpmbuild's --with/--without options.

This provides much easier means to enable conditional parts of a spec than using
brute force hacks like this __with_network_tests.



--- perl-POE-Component-IRC.spec 2006-07-21 05:08:25.000000000 +0200
+++ perl-POE-Component-IRC.spec.hacked  2006-07-21 10:25:01.000000000 +0200
@@ -1,8 +1,7 @@
 # Note:  The tests for this perl dist. are disabled by default, as they
 # require network access and would thus fail in the buildsys' mock
 # environments.  To build locally while enabling tests, either:
-#   rpmbuild ... --define '__with_network_tests 1' ...
-#   define __with_network_tests 1 in your ~/.rpmmacros
+#   rpmbuild ... --with network_tests

 Name:           perl-POE-Component-IRC
 Version:        4.96
@@ -55,9 +54,7 @@

 %check
 # tests require network access, disabled by default
-%if %{?__with_network_tests}
-make test
-%endif
+%{?_with_network_tests:make test}


 %clean

Seems quite reasonable to me, but I've not seen it before.  Can you provide a
reference to some documentation?

It's a nice idea, but doesn't seem to play nice with "make noarch" in the sandbox.  

Actually, it looks like the only way to get an additional flag in to rpm with
the CVS makefile framework is to define it in one's ~/.rpmmacros.

I'd be happy to be shown wrong on this one :)

(In reply to comment #7)
> It's a nice idea, but doesn't seem to play nice with "make noarch" in the
> sandbox.  
What doesn't work?

This approach is being used by probably 100's of packages in FE and does work.

> Actually, it looks like the only way to get an additional flag in to rpm with
> the CVS makefile framework is to define it in one's ~/.rpmmacros.

Are you trying set such per-package flags globally in ~/.rpmmacros?

I think the point is that you can't specify --with arguments when doing testing
from within a normal CVS checkout with "make build" or "make mockbuild".  Which
indeed seems to be correct, but not really something that troubles me.  It
should be possible to add a bit to the Makefile to have "make build" pass the
flags to rpm; doing it in mock seems to be right out.

Chris's solution allows you to edit your .rpmmacros to set the flags and still
use "make build".  Seems reasonably unsavory to me as well, especially since it
broke my build.  I'd be surprised if it makes it through the buildsystem.  I
realize I wasn't very clear at all when I reviewed the package; my intention was
to ensure that you removed that bit or came up with an alternate that would
actually work in mock before you checked it in.  As it is, I don't think the
package will make it through the buildsys.

Of course I meant "make noarch" or "make x86_64" or somesuch instead of "make
build".

(In reply to comment #9)
> I think the point is that you can't specify --with arguments when doing testing
> from within a normal CVS checkout with "make build" or "make mockbuild".  Which
> indeed seems to be correct, but not really something that troubles me.  It
> should be possible to add a bit to the Makefile to have "make build" pass the
> flags to rpm; doing it in mock seems to be right out.

Yah.  The intention was never to pass it into mock (or the buildsys for that
matter), but just to make it easy on one's own box, without monkeying with the
toolset.  Also, setting it in ~/.rpmmacros didn't seem to be too bad a
compromise to me, as there are a number of other packages that I maintain that
could benefit from the same "conditional testing" -- and I'll almost always want
to run those tests for local builds.

> Chris's solution allows you to edit your .rpmmacros to set the flags and still
> use "make build".  Seems reasonably unsavory to me as well, especially since it
> broke my build.  I'd be surprised if it makes it through the buildsystem.  I
> realize I wasn't very clear at all when I reviewed the package; my intention was
> to ensure that you removed that bit or came up with an alternate that would
> actually work in mock before you checked it in.  As it is, I don't think the
> package will make it through the buildsys.

Just to be clear, it was never going to be submitted to the buildsys as is. 
(Much as none have ever been submitted with a release of "0".)

And, as it stands, a happy marriage of the two works...  Leave
__with_network_tests defined in ~/.rpmmacros or at the command line (or
undefined, for that matter), and change the conditional test to:

%check
%{?__with_network_tests: make test}

This allows the build to succeed, with or without __with_network_tests defined
(the original intent, as it were).

(In reply to comment #11)
> And, as it stands, a happy marriage of the two works...  Leave
> __with_network_tests defined in ~/.rpmmacros or at the command line (or
> undefined, for that matter), and change the conditional test to:
> 
> %check
> %{?__with_network_tests: make test}
> 
> This allows the build to succeed, with or without __with_network_tests defined
> (the original intent, as it were).

I guess I need to be more direct:
- Your approach renders building non-deterministic.
- Your approach trashes ~/.rpmmacros, which can affect subsequent builts.
- Your approach is an ugly hack.

IMO, you are out-smarting yourself.

Now I'm really confused.  He's currently using exactly what you told him to use
(modulo one underscore, which is probably a typo) and you're still complaining.
 So what are you proposing that he use now?

Surely you don't pretend to tell him what he's allowed to put in his personal
.rpmmacros file.

(In reply to comment #13)
> Now I'm really confused.  He's currently using exactly what you told him to use
Does he? 

It's not how I read
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=198882#11

> (modulo one underscore, which is probably a typo) and you're still complaining.

The "__" is what makes the difference.

_with_xxx defines are automatically/implicitly set by --with xxx

__with_xxx is a custom define.



>  So what are you proposing that he use now?
MUST: _with_xxx 

This implies supporting --with and renders all the custom 
"--define '__with_xxx 1' superflous.

May-be the problem is understanding the meaning of --with/--without.

Basically all --with/--without does, is to implicitly 
%define _with_xxx 1
rsp.
%define _without_xxx 1

i.e. the "_with" (one underscore) vs. "__with" (two underscores) is the
essential point here.

Then why on earth didn't you just say that?  Comment 12 says nothing about
underscores.  You said "IMO, you are out-smarting yourself" when you could have
simply said "I think you have an extra underscore in your macro definition". 
It's not at all obvious, given that:

1) It's often not easy to see the separation between successive underscores with
certain fonts.  Vera, at least, doesn't seem to show any space between them.

2) You hadn't responded to the request for a reference to any documentation
(comment 6), so some of us were still in the dark until your helpful explanation
in comment 14.  It's unfortunate that this feature seems to be undocumented,
expecially since you want to make it mandatory according to your recent addition
to the packaging committee schedule.  Hopefully you will consider adding your
explanation to a wiki page somewhere.

At last, but not least... closure for this review :)

+Import to CVS
+Add to owners.list
+Bump release, build for devel
+devel build succeeds
+Request branching
+Close bug

Thanks again!

Package Change Request
======================
Package Name: perl-POE-Component-IRC
New Branches: el6
Owners: averi psabata
InitialCC: perl-sig

Git done (by process-git-requests).