Fedora Merge Review: perl-XML-Grove http://cvs.fedora.redhat.com/viewcvs/devel/perl-XML-Grove/ Initial Owner: rnorwood
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
any updates here?
Do we really care to have this package in Fedora?
any updates?
ping?
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.
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?
I note that rnoorwood doesn't own this package. CC'ing the current owner; hopefully we can get this merge review cleared up.
Created attachment 312341 [details] test grove.t is working With 'use utf8' are tests passing.
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)
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.
(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.
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.
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.
I reset the review flag. But to my surprise Parag doesn't seems to be in CC. I am more and more confused.
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. ;-)
(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 ;-)
^ I don't mind if you reassing the review to you.
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/
(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.
(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.
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.
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?
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.
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.
(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.
(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?
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.
(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.
(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.
(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.
(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...
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 ;-)
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.
(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.
OK, I incorporated Paul's changes. Thanks.
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.
(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