Bug 183028 - Review Request: perl-Spiffy
Summary: Review Request: perl-Spiffy
Keywords:
Status: CLOSED NEXTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Jason Tibbitts
QA Contact: Fedora Package Reviews List
URL:
Whiteboard:
Depends On:
Blocks: FE-ACCEPT 183038 183039 183054
TreeView+ depends on / blocked
 
Reported: 2006-02-25 16:37 UTC by Steven Pritchard
Modified: 2011-01-19 09:20 UTC (History)
2 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2006-03-03 00:40:28 UTC
Type: ---
Embargoed:


Attachments (Terms of Use)

Description Steven Pritchard 2006-02-25 16:37:55 UTC
Spec Name or Url: http://ftp.kspei.com/pub/steve/rpms/perl-Spiffy/perl-Spiffy.spec
SRPM Name or Url: http://ftp.kspei.com/pub/steve/rpms/perl-Spiffy-0.24-1.src.rpm
Description:
"Spiffy" is a framework and methodology for doing object oriented (OO)
programming in Perl. Spiffy combines the best parts of Exporter.pm,
base.pm, mixin.pm and SUPER.pm into one magic foundation class. It attempts
to fix all the nits and warts of traditional Perl OO, in a clean,
straightforward and (perhaps someday) standard way.

Spiffy is required by Spoon and IO::All, which are both required by Kwiki.

Comment 1 Jason Tibbitts 2006-02-25 22:58:28 UTC
Man, there's a whole pile of these, but so far the ones I've looked at look
great.    What autogenerated these specs?  I'll try to get through them all tonight.

Anyway, one problem and one question:

BuildRequires: perl >= 1:5.6.1 should go; it will always be satisfied for Extras
packages thus is prohibited by the packaging guidelines.

Why the Provides: perl(mixin)?  I note that RPM does not find that provide, but
I'm wondering why not.

Comment 2 Steven Pritchard 2006-02-26 00:57:11 UTC
> What autogenerated these specs?

cpanspec (http://cpanspec.sf.net/ or install it from Extras).

While I didn't (and probably should) mention it in the changelog, I had to do a
little minor cleanup to most of the specs it generated, but it's usually just a
matter of verifying the license and fixing the description.

> I'll try to get through them all tonight.

Thank you.  :-)

> Anyway, one problem and one question:
> 
> BuildRequires: perl >= 1:5.6.1 should go; it will always be satisfied for
> Extras packages thus is prohibited by the packaging guidelines.

True.  That's probably something picked up by cpanspec.  I'll change that.

> Why the Provides: perl(mixin)?  I note that RPM does not find that provide,
> but I'm wondering why not.

ISTR that rpm picked up a Requires, but not the Provides, so I added the
Provides.  The Requires seems to get picked up in this whole stack, so I don't
think filtering it out would work.

# rpm -q --whatrequires 'perl(mixin)'
perl-IO-All-0.33-1.fc4
perl-Kwiki-0.38-1.fc4
perl-Kwiki-UserPreferences-0.13-1.fc4
perl-Kwiki-Search-0.12-1.fc4
perl-Kwiki-Revisions-0.15-1.fc4
perl-Kwiki-RecentChanges-0.13-1.fc4
perl-Kwiki-NewPage-0.12-1.fc4
perl-Kwiki-UserName-0.14-1.fc4


Comment 3 Steven Pritchard 2006-02-26 01:04:25 UTC
It turns out there's a newer version.

http://ftp.kspei.com/pub/steve/rpms/perl-Spiffy-0.30-1.src.rpm

* Sat Feb 25 2006 Steven Pritchard <steve> 0.30-1
- Update to 0.30.
- Drop explicit perl BR.


Comment 4 Jason Tibbitts 2006-02-26 05:36:19 UTC
OK, the package looks great.  I've thought about the Provides: perl(mixin) a
bit; just to explain it for posterity:

Various modules using this one will make use of the syntax:

use mixin 'package';

which will cause Perl's dependency generator to decide that the package requires
perl(mixin).  Since this package (perl-Spiffy) is the package that enables that
syntax even though RPM's automatic Provides: generation doesn't detect it, it
makes sense to add Provides: perl(mixin) here.

The bad:
Source matches upstream.  However, I can't fetch the source from the provided
URL.  I needed to use
http://search.cpan.org/CPAN/authors/id/I/IN/INGY/Spiffy-0.30.tar.gz instead.

The Summary: is problematic.  How about "A framework for doing object orinted
programming in Perl" instead?

The good:
Package builds and passes tests in mock (development, i386 and x86_64).
rpmlint is silent.
The specfile is clean and well written.
The license is appropriate, matches upstream and is included as %doc.

I'll approve if you fix the Source0: URL and the Summary:.

Comment 5 Jason Tibbitts 2006-02-26 05:39:18 UTC
I meant to add "and you comment the necessity for the Provides: perl(mixin)."

Comment 6 Ralf Corsepius 2006-02-26 06:35:14 UTC
I disagree on your rationale on "Provides: mixin". IMO, this package should NOT
provide it, instead packages applying "use mixin 'package'" should be adapted to
this kind of usage.



Comment 7 Jason Tibbitts 2006-02-26 15:11:36 UTC
Why am I not surprised that you would second guess another of my reviews. 
Frankly, I understand your trepidation but unless you're willing to provide a
reasonable workaround I will still approve this package.  I don't think that
overriding dependency generation for a dozen dependent packages is reasonable.

Comment 8 Jason Tibbitts 2006-02-26 18:21:07 UTC
I will add that it looks to me as if the base Fedora Perl package does the same
thing.  And it makes sense: a package does "use mixin 'something'", so it needs
the package that supplies perl(mixin), which happens to be Spiffy.  Only RPM's
automatic dependency generation doesn't understand that Spiffy provides
perl(mixin), so we have to instruct it.  I suppose the dependency generator
doesn't understand because the Spiffy module doesn't have a file named
"mixin.pm", nor does it have a "package mixin" statement.  It's pretty esoteric
code that looks like it inserts things into the symbol table.

So I think it's perfectly reasonable to have this package provide perl(mixin),
because in reality it does provide perl(mixin).

Comment 9 Ralf Corsepius 2006-02-27 06:39:02 UTC
(In reply to comment #8)
> I will add that it looks to me as if the base Fedora Perl package does the same
> thing.  And it makes sense: a package does "use mixin 'something'", so it needs
> the package that supplies perl(mixin), 

perl(mixin)  would correspond to mixin.pm.

perl-Spiffy doesn't provide this, as can be easily demonstrated:
perl -e 'use mixin'

=> Letting the rpm "Provide: perl(mixin)" is a bug.

> So I think it's perfectly reasonable to have this package provide perl(mixin),
> because in reality it does provide perl(mixin).
Cf. above. it doesn't. It's packages using perl(Spiffy) which apply magic to
access the Spiffy::mixin method as "mixin"
=> It's those applications which have to "Requires: perl(Spiffy)" to get this
working. If rpm generates bogus "Requires: perl(mixin)" for those applications,
these have to filter them out.

Comment 10 Paul Howarth 2006-02-27 10:12:37 UTC
(In reply to comment #9)
> => It's those applications which have to "Requires: perl(Spiffy)" to get this
> working. If rpm generates bogus "Requires: perl(mixin)" for those applications,
> these have to filter them out.

I agree with Ralf, and have a suggestion for how to do this:

Have the perl-Spiffy package (which is presumably a buildreq of the problematic
packages) provide a custom perl "requires" script (perhaps as simple as an
expansion of "%{__perl_requires} "$@" | sed -e 's/^perl(mixin)/perl(Spiffy)/'")
and ship it with the perl-Spiffy package as /usr/lib/rpm/perl-Spiffy.req. Then,
in each dependent package, simply add the line:

%define __perl_requires /usr/lib/rpm/perl-Spiffy.req

That's not too hard, is it?

Comment 11 Jason Tibbitts 2006-02-27 14:13:22 UTC
I still disagree, because it is quite possible for a package to provide
functionality different from its name.  But fortunately Paul is actually
constructive in his objections, though I feel that adding that hack to every
downstream package is far worse then actually specifying what Spiffy provides.  

If Steve is willing to add hacks to all of the downstream packages then I won't
object.

Comment 12 Ralf Corsepius 2006-02-27 14:25:38 UTC
(In reply to comment #11)
> I still disagree, because it is quite possible for a package to provide
> functionality different from its name.

Consider the case of adding annother package implementing "perl(mixin)"?

> If Steve is willing to add hacks to all of the downstream packages then
> I won't object.

You guys are putting the cart before the horse. I can't avoid VETO'ing against
this package if the Provides: mixin won't be removed.


Comment 13 Paul Howarth 2006-02-27 14:29:04 UTC
(In reply to comment #11)
> I still disagree, because it is quite possible for a package to provide
> functionality different from its name.  But fortunately Paul is actually
> constructive in his objections, though I feel that adding that hack to every
> downstream package is far worse then actually specifying what Spiffy provides.  

Whilst it's fine for a package to provide functionality different from its name,
the problem here is that the package is saying it provides something that it
doesn't, namely a perl module called "mixin". Pretending that it provides this
package means that it's not possible for, at some future date, a new perl module
of that name to be packaged properly without clashing with this one.


Comment 14 Jason Tibbitts 2006-02-27 14:51:39 UTC
Mid-air collission - I'm replying to Ralf here.

Consider the case of adding another package implementing "webserver".  So we
have two packages providing the same thing.  The "mixin" functionality you get
depends on which package you 'use'.  You could never use that package and this
one in the same Perl program because they would conflict at the language level.
 What perl packages provide and which conflict with each other doesn't simply
map onto RPM dependencies.

Anyway, the issue is whether Steve is willing to add the Requires: filtering
kindly provided by Paul to the packages that require this one, so that the
Provides: here can be removed.  It's simpler to ask here than in all of the
downstream bugzilla tickets.  If he's not, then we try to work out another solution.

BTW, please quote the place in the review policy where the process of vetoing
reviews is laid out.  I would like to know how it works, but I can't find it.

Comment 15 Ralf Corsepius 2006-02-27 14:59:03 UTC
(In reply to comment #14)
> Mid-air collission - I'm replying to Ralf here.
> 
> Consider the case of adding another package implementing "webserver". 
Yes, ... RH's responsibility, bad design on their part.

However, there is a substantial difference between "webserver" and "perl(mixin)"

"webserver" is a virtual package/property, perl(mixin) is a 1:1 correspondence
to mixin.pm. 

> So we
> have two packages providing the same thing.  The "mixin" functionality you get
> depends on which package you 'use'.
Exactly.

>  You could never use that package and this
> one in the same Perl program because they would conflict at the language level.
>  What perl packages provide and which conflict with each other doesn't simply
> map onto RPM dependencies.
It does. perl(xxx) is the file dependency on xxx.pm with the %vendor*/%perl*
cruft removed.
 
> BTW, please quote the place in the review policy where the process of vetoing
> reviews is laid out.  I would like to know how it works, but I can't find it.
FE doesn't have a policy of vetoing.



Comment 16 Jason Tibbitts 2006-02-27 16:01:41 UTC
> It does. perl(xxx) is the file dependency on xxx.pm with the %vendor*/%perl*
> cruft removed.

Now that's a clean, well defined statement that I can live with.  But is it
actually written down as policy anywhere?  Judging from things like "webserver",
I have always taken Requires:/Provides: to be based on functionality except when
specifying paths or library names.  If perl(blah) is different then things are
certainly simpler, but this really has to be written down somewhere that someone
like me can find.  Otherwise I'm just seeing policy being made up on the spot
and frankly it looks rather capricious.  (See also the veto thing.)

In any case, this is just banter until we hear from Steve.  So, Steve: I think
the Provides: perl(mixin) is the only issue of contention, so if you drop that
and fixe the Source0: URL and the summary then I'll approve this and move on to
the rest of your submissions.  If you don't want to drop the Provides: and
filter out the dependency downstream, then say so and we'll try to figure
something else out.

Comment 17 Steven Pritchard 2006-02-27 16:29:23 UTC
What if I just add a mixin.pm that does nothing but "use Spiffy;" (or maybe "die
'use Spiffy; first';")?  I could document the entire packaging problem there,
and I think if I do it right, it wouldn't affect proper use of Spiffy at all.

I *really* don't want to force some hack on every package that uses Spiffy,
which would probably mean every Kwiki package (potentially 130 of them at the
moment).

Comment 18 Jason Tibbitts 2006-02-27 17:31:14 UTC
To go down that road, you could just use:

package mixin;
1;

(You need the package line for RPM to pick up the Provides.)  I built a quick
package using this and it does work.  It probably satisfies the letter of the
requirement, but I'm sure there will be objections as it's far nastier than just
a single Provides: line and is obvious trickery which purists will hate.  On the
plus side, is prevents conflicts with any hypothetical mixin.pm that may appear
in the future.

Barring that, perhaps we could get the dependency filter into the Perl template?
 130 packages needing it certainly argues for general accessibility.

Comment 19 Steven Pritchard 2006-02-27 18:16:05 UTC
http://ftp.kspei.com/pub/steve/rpms/perl-Spiffy-0.30-2.src.rpm

* Mon Feb 27 2006 Steven Pritchard <steve> 0.30-2
- Drop explicit Provides: mixin.
- Add dummy mixin.pm.
- Improve Summary.
- Fix Source0.


Comment 20 Ralf Corsepius 2006-02-27 18:21:47 UTC
(In reply to comment #19)
> http://ftp.kspei.com/pub/steve/rpms/perl-Spiffy-0.30-2.src.rpm
> 
> * Mon Feb 27 2006 Steven Pritchard <steve> 0.30-2
> - Drop explicit Provides: mixin.
> - Add dummy mixin.pm.
</sigh> With all due respect, what have you been drinking? One crazyness
replacing the next?




Comment 21 Jason Tibbitts 2006-02-27 19:52:12 UTC
Look, he's trying to come up with a solution that doesn't involve hacking 130
downstream packages.  If you don't want to be constructive then please just
don't post.

Steve, I'm working on filtering the requires and it's really simple; you just
need six lines in %prep.  Looking through what other packages in Core and Extras
do, I see that this is really very common:

cat << EOF > %{name}-req
#!/bin/sh
%{__perl_requires} $* | sed -e '/perl(mixin)/d'
EOF
%define __perl_requires %{_builddir}/%{name}-%{version}/%{name}-req
chmod +x %{__perl_requires}

(You can also reference an external script as %{SOURCE99} if you find emitting
it from the spec distasteful.)

Finally, I was hacking on the packages and noticed that Spiffy also provides
perl(DB), which I think is broken.  (It overrides the super() method in the DB
package, and so it has a "package DB" statement which RPM turns into Provides:
perl(DB).)  This has the capacity to break other things, so Spiffy is definitely
going to need a __perl_provides override as well:

cat <<XXX > %{name}-prov
#!/bin/sh
%{__perl_provides} $* | sed -e '/perl(DB)/d'
XXX
%define __perl_provides %{_builddir}/Spiffy-%{version}/%{name}-prov
chmod +x %{__perl_provides}

With this, things are working cleanly.

I know it's a pain to have to do this in all of the modules, but I think this is
going to be the only way to move forward.  I will try to get these overrides
into the Perl template and I suggest that they get into cpanspec as well.

Comment 22 Steven Pritchard 2006-02-27 20:16:41 UTC
I missed the perl(DB) thing.  That's fixed in -3.

http://ftp.kspei.com/pub/steve/rpms/perl-Spiffy-0.30-3.src.rpm

I still *really* don't like the idea of modifying (potentially) nearly every
Kwiki package to filter out the perl(mixin) thing, especially since there's no
easy way to tell if the dependency will get picked up ahead of time.

Comment 23 Steven Pritchard 2006-02-27 22:10:11 UTC
Given that there is a real mixin.pm, I'm checking now to see if Spiffy is a
drop-in replacement for it.  If it is, we *should* be able to include a fake
mixin.pm that just does "use Spiffy".  Otherwise, I guess I have to filter the
perl(mixin) dependency.

Comment 24 Steven Pritchard 2006-02-27 22:54:39 UTC
OK, I've dropped the bogus mixin.pm.

http://ftp.kspei.com/pub/steve/rpms/perl-Spiffy-0.30-4.src.rpm

Comment 25 Jason Tibbitts 2006-02-28 01:54:58 UTC
I think we're set.  Approved.

BTW, while talking with Ville I found that for correctness you need to backwhack
the first XXX.  My screwup.  I don't know if it breaks anything, but you might
wnat to hit it before checking in.  The current version of this stuff is at
http://fedoraproject.org/wiki/Packaging/Perl

Comment 26 Steven Pritchard 2006-02-28 15:52:24 UTC
> BTW, while talking with Ville I found that for correctness you need to
> backwhack the first XXX.  My screwup.  I don't know if it breaks anything,
> but you might wnat to hit it before checking in.

I wonder what the reason is for that.  It seems to build fine as-is.

Comment 27 Ville Skyttä 2006-02-28 19:49:20 UTC
Yes, because the perl.prov and perl.req and friends scriptles don't currently
take any arguments but just read the filelist from stdin.  Backslash protects
the $* so it won't be expanded too early when emitting the script.

Comment 28 Steven Pritchard 2006-02-28 20:42:22 UTC
Ah, so to be more obvious to a perl person, would this be acceptable?

cat <<'END' > %{name}-prov
#!/bin/sh
%{__perl_provides} "$@" | sed -e '/^perl(DB)$/d'
END
%define __perl_provides %{_builddir}/Spiffy-%{version}/%{name}-prov
chmod +x %{__perl_provides}

Comment 29 Steven Pritchard 2006-02-28 20:53:07 UTC
Actually, since anything using Module::Signature doesn't like for anything to be
in the build directory that isn't in MANIFEST, I'm going with this (unless there
are any objections):

cat <<'END' > %{_sourcedir}/%{name}-prov
#!/bin/sh
%{__perl_provides} "$@" | sed -e '/^perl(DB)$/d'
END
%define __perl_provides %{_sourcedir}/%{name}-prov
chmod +x %{__perl_provides}


Comment 30 Ville Skyttä 2006-02-28 21:00:46 UTC
That will leave trash behind in sourcedir.  And for a number of reasons, all
Module::Signature checks should be disabled anyway (the exact way how to do that
varies between packages).  More info:
http://koti.welho.com/vskytta/packagers-handbook/packagers-handbook.html#guidelines-perl-cpansign


Comment 31 Ville Skyttä 2006-03-03 07:17:07 UTC
The sourcedir-trashing filter implementation was left in this package and also
applied in perl-IO-All.   I wonder why is that?

Comment 32 Steven Pritchard 2006-03-03 15:51:14 UTC
I haven't seen (or come up with) another solution that I'm completely happy with
yet, but I've been watching the "Filtering requires/provides" thread on
fedora-perl-devel-list.  When that's resolved, I'll fix this and the other
packages I submitted, and I'll add code to cpanspec to match.

Comment 33 Mathieu Bridon 2011-01-19 09:20:49 UTC
This package is in EPEL4, but not in EPEL5 and 6. Is there any reason for that?

I would like to see this package in EPEL6, and I am willing to help co-maintain it.


Note You need to log in before you can comment on or make changes to this bug.