Bug 226285 - (perl-XML-Grove) Merge Review: perl-XML-Grove
Merge Review: perl-XML-Grove
Status: CLOSED CURRENTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Patrice Dumas
Fedora Extras Quality Assurance
: Reopened
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2007-01-31 15:38 EST by Nobody's working on this, feel free to take it
Modified: 2008-11-14 09:16 EST (History)
10 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2008-11-14 09:16:46 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
pertusus: fedora‑review+


Attachments (Terms of Use)
test grove.t is working (387 bytes, patch)
2008-07-22 09:04 EDT, Marcela Mašláňová
no flags Details | Diff
Patch to remove bogus deps and provides (1.11 KB, patch)
2008-10-15 10:45 EDT, Paul Howarth
no flags Details | Diff

  None (edit)
Description Nobody's working on this, feel free to take it 2007-01-31 15:38:32 EST
Fedora Merge Review: perl-XML-Grove

http://cvs.fedora.redhat.com/viewcvs/devel/perl-XML-Grove/
Initial Owner: rnorwood@redhat.com
Comment 1 Parag AN(पराग) 2007-10-22 03:24:16 EDT
1)you may like to use disttag 
2)rpmlint only complained ->
perl-XML-Grove.noarch: E: invalid-version 0.46alpha
The version string must not contain the pre, alpha, beta or rc suffixes
because when the final version will be out, you will have to use an Epoch tag
to make the package upgradable. Instead put it in the release tag like
0.alpha8.1.
==> You need to follow solution for this error.

perl-XML-Grove.noarch: W: invalid-license Artistic
==> Use GPL+ or Artistic 

perl-XML-Grove.noarch: E: useless-explicit-provides perl(XML::Grove)
This package provides 2 times the same capacity. It should only provide it
once.
==> In this case I think you need to remove unwanted provides. check
http://fedoraproject.org/wiki/Packaging/Perl

perl-XML-Grove.src: W: mixed-use-of-spaces-and-tabs (spaces: line 1, tab: line 15)
The specfile mixes use of spaces and tabs for indentation, which is a
cosmetic annoyance.  Use either spaces or tabs for indentation, not both.
==> Use "sed -i -e 's|\t|  |g' perl-XML-Grove"

3) make test is failing in mock build as
PERL_DL_NONLAZY=1 /usr/bin/perl "-MExtUtils::Command::MM" "-e" "test_harness(0,
'blib/lib', 'blib/arch')" t/*.t
t/factory....ok
t/grove......FAILED tests 3-4
        Failed 2/5 tests, 60.00% okay
Failed Test Stat Wstat Total Fail  Failed  List of Failed
-------------------------------------------------------------------------------
t/grove.t                  5    2  40.00%  3-4
Failed 1/2 test scripts, 50.00% okay. 2/10 subtests failed, 80.00% okay.
make: *** [test_dynamic] Error 255
+ :
+ exit 0
Comment 2 Parag AN(पराग) 2007-11-19 01:31:58 EST
any updates here?
Comment 3 Parag AN(पराग) 2007-12-04 09:03:46 EST
Do we really care to have this package in Fedora?
Comment 4 Parag AN(पराग) 2007-12-17 12:55:02 EST
any updates?
Comment 5 Parag AN(पराग) 2008-01-03 11:20:48 EST
ping?
Comment 6 Robin Norwood 2008-01-03 11:38:18 EST
Sorry for the lack of updates.  I haven't figured out why the test is failing
yet.  It looks like a unicode encoding issue.  I'll try to take a look at it soon.
Comment 7 Parag AN(पराग) 2008-01-30 03:43:12 EST
strange I checked grove.t test and got confused why its failing. 
Anyway this package I guess have problem of tests from its initial entry in
Fedora. I don't want to block review and wait here till you resolve this.

Will you please update other issues commented as in comment #1?


Comment 8 Jason Tibbitts 2008-06-18 13:53:13 EDT
I note that rnoorwood doesn't own this package.  CC'ing the current owner;
hopefully we can get this merge review cleared up.
Comment 9 Marcela Mašláňová 2008-07-22 09:04:54 EDT
Created attachment 312341 [details]
test grove.t is working

With 'use utf8' are tests passing.
Comment 10 Stepan Kasal 2008-09-29 10:34:17 EDT
I looked at the rpmlint complaints in comment #1.  I think the non-standard Version: tag should stay, and I added an explaining comment to the spec file.
I resolved the tab vs. spaces nonsense.

But the following rpmlint complaints are still to be resolved:

perl-XML-Grove.noarch: E: useless-explicit-provides perl(XML::Grove)
This package provides 2 times the same capacity. It should only provide it
once.

perl-XML-Grove.noarch: W: doc-file-dependency /usr/share/doc/perl-XML-Grove-0.46alpha/examples/my-html.pl perl(XML::Parser::PerlSAX)
An included file marked as %doc creates a possible additional dependency in
the package.  Usually, this is not wanted and may be caused by eg. example
scripts with executable bits set included in the package's documentation.

(the latter is repeated twice)
Comment 11 Marcela Mašláňová 2008-09-30 04:10:22 EDT
check release numbers here:
https://fedoraproject.org/wiki/Packaging/NamingGuidelines#NonNumericRelease

- package meets naming guidelines OK
- package meets packaging guidelines OK
- license (same as Perl) OK, text in %doc, matches source OK
- spec file legible, in am. english OK
- source matches upstream FAIL -> source at upstream is in tar.gz format
- package compiles on devel (x86) OK
- no missing BR OK
- no unnecessary BR FAIL
- not relocatable OK
- owns all directories that it creates OK
- no duplicate files OK
- permissions ok OK
- %clean ok OK
- macro use consistent OK
- no need for -docs OK
- nothing in %doc affects runtime OK
- no need for .desktop file OK

Please fix the source and useless dependency.
Comment 12 Stepan Kasal 2008-10-14 06:56:43 EDT
(In reply to comment #11)
> https://fedoraproject.org/wiki/Packaging/NamingGuidelines#NonNumericRelease

As mentioned in comment #10, I'm convinced the non-standard version tag should stay in this case.

> - source matches upstream FAIL -> source at upstream is in tar.gz format

Fixed in release 34.

> - no unnecessary BR FAIL
> [...]
> Please fix the source and useless dependency.

??? BuildRequires: perl-libxml-perl perl(ExtUtils::MakeMaker)
I see no problem here.

(In reply to comment #10)
> perl-XML-Grove.noarch: E: useless-explicit-provides perl(XML::Grove)
> This package provides 2 times the same capacity. It should only provide it
> once.

No explicit provides.
Both "perl(XML::Grove)" and "perl(XML::Grove) = 0.46" are generated.
It might be a bug in rpmlint or the dependency generator, but not in this package.

> perl-XML-Grove.noarch: W: doc-file-dependency
> /usr/share/doc/perl-XML-Grove-0.46alpha/examples/my-html.pl
> perl(XML::Parser::PerlSAX)
> An included file marked as %doc creates a possible additional dependency in
> the package.  Usually, this is not wanted and may be caused by eg. example
> scripts with executable bits set included in the package's documentation.
> 
> (the latter is repeated twice)

Again, these are bogus.  The *.pl files in doc do not have their executable bit set, and the require "perl(XML::Parser::PerlSAX)" is justified by other *.pm files in the package.

To sum up, the review passed.

Several issues about rpmlint and/or the dependency generator are due to be filed.
Comment 13 Patrice Dumas 2008-10-14 07:12:12 EDT
I am quite confused by the status of this review. Who is the maintainer
and who is the reviewer?

Also it is not right to close it without letting Parag say something.

About the version, the comment should pasted here so that everybody
can see what reasononing is used. I found:

###### rpmlint error: E: invalid-version 0.46alpha
# We use the version tag "0.46alpha" -- the traditional version number of
# this module since Sep 1999, immutable through the ages, no matter what the
# current Package Naming Guidelines say.
 
which is a very poor argumentation in my opinion.
Comment 14 Patrice Dumas 2008-10-14 07:15:42 EDT
I had a quick look at the pkgdb and it looks like Stepan is the
maintainer so he definitively cannot grant the review. Marcela
seems to be co-maintainer, so this applies too. 

There are also dozens of co-maintainers that are dubiously 
interested in the packae, but it is not a big deal.
Comment 15 Patrice Dumas 2008-10-14 07:17:25 EDT
I reset the review flag. But to my surprise Parag doesn't seems to
be in CC. I am more and more confused.
Comment 16 Stepan Kasal 2008-10-14 08:40:13 EDT
Hello Patrice,

(In reply to comment #13)
> About the version, the comment should pasted here so that everybody
> can see what reasononing is used.

You are right, sorry.

> ###### rpmlint error: E: invalid-version 0.46alpha
> # We use the version tag "0.46alpha" -- the traditional version number of
> # this module since Sep 1999, immutable through the ages, no matter what the
> # current Package Naming Guidelines say.
> 
> which is a very poor argumentation in my opinion.

How would you solve this?  Would you change evr to 1:0.46-0.1.alpha.fc10 ?

Does this really justify the epoch increase?
Wouldn't that confuse people:
  "oh, 0.46 finally released!"
  "Oh, wait, ... Dash!, those RH &*!% always advertise different version then they deliver!"

But I agree to change evr that way if you do the review for me.  ;-)
Comment 17 Patrice Dumas 2008-10-14 08:56:43 EDT
(In reply to comment #16)
> Hello Patrice,
> 

> How would you solve this?  Would you change evr to 1:0.46-0.1.alpha.fc10 ?
> 
> Does this really justify the epoch increase?
> Wouldn't that confuse people:
>   "oh, 0.46 finally released!"
>   "Oh, wait, ... Dash!, those RH &*!% always advertise different version then
> they deliver!"
> 
> But I agree to change evr that way if you do the review for me.  ;-)

I am indeed ready to do the review; But I really dislike epochs 
too. So I think that, even though it is against the guidelines, given
that the package seems to be very slowly moving, and with the possibility
that upstream skip one version to go straight to 0.47, for 
example I think that this version is right. So I propose something along:

# the version is against the guidelines. However adherence to
# the guideline would imply using an epoch, which is very inconvenient.
# Given that this package is very slowly moving, and hoping that
# upstream skip one version to go straight to 0.47, it seems better
# not to use an epoch. If 0.46 is ever released, the epoch way would have
# to be used, but we are better avoiding that if possible.

That seems to me to be a better argumentation ;-)
Comment 18 Marcela Mašláňová 2008-10-14 09:54:59 EDT
^ I don't mind if you reassing the review to you.
Comment 19 Stepan Kasal 2008-10-14 11:57:24 EDT
Hello Pat,

(In reply to comment #17)
> I am indeed ready to do the review;

great!  Thanks a lot.  Marcela (the current asisignee) agrees, so I'm re-assigning the bug to you.

> But I really dislike epochs too.

yep.

> # the version is against the guidelines. However adherence to
> # the guideline would imply using an epoch, which is very inconvenient.
> # Given that this package is very slowly moving, and hoping that
> # upstream skip one version to go straight to 0.47, it seems better
> # not to use an epoch. If 0.46 is ever released, the epoch way would have
> # to be used, but we are better avoiding that if possible.
> 
> That seems to me to be a better argumentation ;-)

Very nice and clear, thanks.  What about the following small change, though?

s/this package is very slowly moving/this package's development is stalled since Sep 1999/
Comment 20 Patrice Dumas 2008-10-14 12:04:26 EDT
(In reply to comment #19)

> Very nice and clear, thanks.  What about the following small change, though?
> 
> s/this package is very slowly moving/this package's development is stalled
> since Sep 1999/

No problem.
Comment 21 Stepan Kasal 2008-10-14 12:16:28 EDT
(In reply to comment #15)
> But to my surprise Parag doesn't seems to be in CC.

Here is what I see in the history:
Parag was willing to do the review, but after half year of no reactions he gave up and assigned the review back to nobody.  At that time he might has added himself to cc but he hasn't; it seems natural at that situation and I'm not sure if he were happy if I added him to cc now.

I hope he won't mind if we finish the review without dragging him in.
Comment 22 Patrice Dumas 2008-10-14 12:19:10 EDT
No problem, we just should be sure to let the review opened something 
like a day after the approval such that he has time to put a comment.
Comment 23 Stepan Kasal 2008-10-14 14:21:59 EDT
So I committed your version of the comment, removed || : from %check and verified that a scratch build finishes.

Could you please look whether you see any unresolved problems?
Comment 24 Patrice Dumas 2008-10-14 16:54:22 EDT
There are spurious provides coming from examples:

Provides: perl(MyHTML) perl(MyVisitor)

rpmlint says:
known issue:
perl-XML-Grove.src: E: invalid-version 0.46alpha
perl-XML-Grove.noarch: E: invalid-version 0.46alpha

As said above both "perl(XML::Grove)" and "perl(XML::Grove) = 0.46" are 
generated. If this is problematic, like in that case th estring is never taken into account this should be acted upon, otherwise this may be ignored.
perl-XML-Grove.noarch: E: useless-explicit-provides perl(XML::Grove)

This is already a package dependency, so no problem:
perl-XML-Grove.noarch: W: doc-file-dependency /usr/share/doc/perl-XML-Grove-0.46alpha/examples/my-html.pl perl(XML::Parser::PerlSAX)
perl-XML-Grove.noarch: W: doc-file-dependency /usr/share/doc/perl-XML-Grove-0.46alpha/examples/visitor.pl perl(XML::Parser::PerlSAX)

ls -l *gz*
-rw-rw-r-- 1 dumas dumas 27336 sept. 10  1999 XML-Grove-0.46alpha.tar.gz
-rw-rw-r-- 1 dumas dumas 27336 oct. 14 12:13 XML-Grove-0.46alpha.tar.gz-cvs
tarball timestamp was not kept. It is too late now, but please do it
for the next time.

match upstream
48bee70ae412bd6cf8ef302b6c68e24e  XML-Grove-0.46alpha.tar.gz


In summary, please remove the spurious provides and I will approve the package.
Comment 25 Parag AN(पराग) 2008-10-14 23:52:16 EDT
Ok. I will like to say only that I don't mind if anyone add me in CC to any package review. I will be happy to help them. But as you all see what happened here after I requested updated SRPM many times. So I gave up with conclusion no one cares here so why should I keep myself in CC also? Let me work on other reviews.

As I am working on some other things please allow me to stay away from this review. I know Patrice will help this package to review.

Thanks all.
Comment 26 Stepan Kasal 2008-10-15 09:13:04 EDT
(In reply to comment #24)
> ls -l *gz*
> -rw-rw-r-- 1 dumas dumas 27336 sept. 10  1999 XML-Grove-0.46alpha.tar.gz
> -rw-rw-r-- 1 dumas dumas 27336 oct. 14 12:13 XML-Grove-0.46alpha.tar.gz-cvs
> tarball timestamp was not kept. It is too late now, but please do it
> for the next time.

Weird.  My own copy of the file had the right time stamp when I issued
   make new-source FILES=XML-Grove-0.46alpha.tar.gz

But it seems the upload process has crippled the time stamp.

2008-10-14 12:13:51 is the access time of the file on my disk, but I do not know if it is the time of download from upstream or the time of upload to fedora cvs.
I think that this upload failed because I had not the ssh key in my ssh keyring at the moment.  But in any case, it looks like a bug that "make upload" left things in an inconsistent state.
Comment 27 Stepan Kasal 2008-10-15 09:26:09 EDT
(In reply to comment #26)
I said:
> I think that this upload failed because I had not the ssh key in my ssh
> keyring at the moment.

This does not seem likely, the ssh key is used later, for "cvs update".

> But in any case, it looks like a bug that "make upload" left
> things in an inconsistent state.

Indeed, it looks like a bug in update.cgi; don't you, by any chance, know against what component should the bug be filed?
Comment 28 Patrice Dumas 2008-10-15 09:49:28 EDT
I am missing something, isn't the file in the lookaside cache since a long time now? Subsequente make upload won't change the timestamp. In fact I guess that it was automatically imported, or something like that, during the marge from the old Red Haat internal buildsystem.

Anyway it isn't important, just fix the Provides and we're done.
Comment 29 Stepan Kasal 2008-10-15 09:59:54 EDT
(In reply to comment #24)
> There are spurious provides coming from examples:
> 
> Provides: perl(MyHTML) perl(MyVisitor)

indeed.  So it seems the messages were almost right:

> perl-XML-Grove.noarch: W: doc-file-dependency
> /usr/share/doc/perl-XML-Grove-0.46alpha/examples/my-html.pl
> perl(XML::Parser::PerlSAX)

> perl-XML-Grove.noarch: W: doc-file-dependency
> /usr/share/doc/perl-XML-Grove-0.46alpha/examples/visitor.pl
> perl(XML::Parser::PerlSAX)

Had it mentioned perl(MyHTML) and perl(MyVisitor), respectively, the messages would have named the problem.

But the "rpmlint -i" hint says that it helps to clear the exec bit of the files.
This is not true, the bit is cleared, yet the dependency generator brings in the wrong provides.

Should I fight against the dependency generator somehow? gzipping the examples, rot13-encoding, renaming them, clearing the #! line or whatever... ???

Or is it enough to file a bug aginst the buggy dependency generator?

To sum up, I still apply for the approval of the package in its current state in the cvs.
Comment 30 Stepan Kasal 2008-10-15 10:06:07 EDT
(In reply to comment #28)
> I am missing something, isn't the file in the lookaside cache since a long time
> now?

No, it isn't; the file in cvs has yesterday's time stamp:

> -rw-rw-r-- 1 dumas dumas 27336 oct. 14 12:13 XML-Grove-0.46alpha.tar.gz-cvs

and I imported that file yesterday.  And I did the import yesterday, using the originial tar.gz, dowloaded from upstream, with the right 1999 time stamp.
But the upload command (upload.cgi, called from "make new-source") screwed the time stamp.  I do not know why.

I agree with you that there is no known way to fix the time stamp now.

I think that the bug in upload.cgi which caused that, should be reported.
Comment 31 Patrice Dumas 2008-10-15 10:09:00 EDT
(In reply to comment #30)
> (In reply to comment #28)
> > I am missing something, isn't the file in the lookaside cache since a long time
> > now?
> 
> No, it isn't; the file in cvs has yesterday's time stamp:

I forgot about the .tar.gz vs .tar.bz2.

> I think that the bug in upload.cgi which caused that, should be reported.

I guess that the best location would be the trac of the infrastructure team.
Comment 32 Patrice Dumas 2008-10-15 10:16:24 EDT
(In reply to comment #29)
> (In reply to comment #24)
> > There are spurious provides coming from examples:
> > 
> > Provides: perl(MyHTML) perl(MyVisitor)
> 
> indeed.  So it seems the messages were almost right:
> 
> > perl-XML-Grove.noarch: W: doc-file-dependency
> > /usr/share/doc/perl-XML-Grove-0.46alpha/examples/my-html.pl
> > perl(XML::Parser::PerlSAX)
> 
> > perl-XML-Grove.noarch: W: doc-file-dependency
> > /usr/share/doc/perl-XML-Grove-0.46alpha/examples/visitor.pl
> > perl(XML::Parser::PerlSAX)

No, unless I am wrong it is about Requires, not about Provides, and I thing that it is harmless since those Requires are Provided by the package or are also
a Requires for the package.
 
> Had it mentioned perl(MyHTML) and perl(MyVisitor), respectively, the messages
> would have named the problem.
> 
> But the "rpmlint -i" hint says that it helps to clear the exec bit of the
> files.
> This is not true, the bit is cleared, yet the dependency generator brings in
> the wrong provides.

The perl dependencies generator doesn't take into account the exec
bit since .pm are not executables in general.

> Should I fight against the dependency generator somehow? gzipping the examples,
> rot13-encoding, renaming them, clearing the #! line or whatever... ???

I don't think so.

> Or is it enough to file a bug aginst the buggy dependency generator?

The bug I see is that rpmlint should say that perl(MyHTML) and perl(MyVisitor) are doc file Provides dependencies, I don't see a bug in the dependency generator.

And the rpmlint explanation is untrue, but rpmlint cannot know about all
dependency generators, especially those that are customized.
 
> To sum up, I still apply for the approval of the package in its current state
> in the cvs.

The perl(MyHTML) and perl(MyVisitor) provides should be removed. They 
are bogus provides, even though rpmlint doesn't find them...
Comment 33 Paul Howarth 2008-10-15 10:45:22 EDT
Created attachment 320442 [details]
Patch to remove bogus deps and provides

Here's a simple patch that removes the bogus provides for perl(MyHTML) and perl(Visitor), the unversioned perl(XML::Grove) provide (leaving the versioned one), and the doc file dependencies that rpmlint complains about.

It also switches the position of the "-depth" option in a "find" command to the start of the command, a style thing that I just can't seem to help myself fixing wherever I come across it ;-)
Comment 34 Patrice Dumas 2008-10-15 10:54:26 EDT
I don't think that the perl(XML::Parser::PerlSAX) requires should be 
removed, it is really required (unless I am wrong), not only by a doc 
file.
Comment 35 Paul Howarth 2008-10-15 11:04:56 EDT
(In reply to comment #34)
> I don't think that the perl(XML::Parser::PerlSAX) requires should be 
> removed, it is really required (unless I am wrong), not only by a doc 
> file.

Hmm, you're right, it's used by XML::Grove and XML::Grove::Subst. I wonder why rpmlint had it down as a doc file dependency then?

So the requires filter isn't needed at all then, which simplifies things.
Comment 36 Stepan Kasal 2008-10-15 12:28:20 EDT
OK, I incorporated Paul's changes.  Thanks.
Comment 37 Patrice Dumas 2008-10-15 16:20:09 EDT
Everything is right now. APPROVED.

One minor thing, I think that
/usr/lib/perl5/vendor_perl/5.10.0/XML/DOM-ecmascript.pod
should be labelled as %doc, but also that it should be automatic.
Comment 38 Stepan Kasal 2008-10-16 14:58:04 EDT
(In reply to comment #37)
> Everything is right now. APPROVED.

THANKS

> One minor thing, I think that
> /usr/lib/perl5/vendor_perl/5.10.0/XML/DOM-ecmascript.pod
> should be labelled as %doc, but also that it should be automatic.

done in -36.fc10

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