Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=226295
Summary: Merge Review: php-pear Product: Fedora Extras Version: devel Platform: All OS/Version: Linux Status: NEW Severity: normal Priority: normal Component: Package Review AssignedTo: nobody@fedoraproject.org ReportedBy: nobody@fedoraproject.org QAContact: fedora-package-review@redhat.com CC: jorton@redhat.com
Fedora Merge Review: php-pear
http://cvs.fedora.redhat.com/viewcvs/devel/php-pear/ Initial Owner: jorton@redhat.com
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Merge Review: php-pear
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=226295
------- Additional Comments From chris.stone@gmail.com 2007-01-31 16:57 EST ------- Would it make sense to put the pear classes this package needs into different SRPMS? Such as php-pear-Archive-Tar, php-pear-Console-Getargs etc. Then put a circular dependency on them? This would allow upgrades to the classes without actually updating pear itself.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Merge Review: php-pear
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=226295
------- Additional Comments From jorton@redhat.com 2007-01-31 17:10 EST ------- I don't see why this would be particularly useful; the PEAR install-pear-*.phar bootstraps itself without needing existing PEAR classes already.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Merge Review: php-pear
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=226295
------- Additional Comments From chris.stone@gmail.com 2007-01-31 17:28 EST ------- The only advantage is that you get to update the Archive-Tar, Console-Getargs, etc packages without having to update php-pear. However there is the extra burden of supporting 4-5 additional packages, but I would be willing to maintain the additional packages myself.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Merge Review: php-pear
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=226295
------- Additional Comments From chris.stone@gmail.com 2007-02-01 11:18 EST ------- I gave a quick one minute perusal of the spec file, one thing I noticed is you have %{buildroot} and $RPM_BUILD_ROOT mixed which is a blocker for the extras guidelines (guess it is confusing or something).
The buildroot also needs to be fixed, fesco decided to use the one and only true buildroot which is: %{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n)
Ive never seen BuildArchitecture tag before, I've always just seen "BuildArch", guess thats okay though, just interesting, im not sure what the official tag is or how rpm works, but I guess this works.
The standard license tag should just be "PHP License" or "PHP License 3.0", to match rpmlint list of valid licenses.
Should the %configs have a (noreplace) flag??
And here is the really really important one:
pear 1.5.0 is out, and it fixes a bug I need to do a proper upgrade path for php-pear-PHPUnit.
This is by no means an official review, just a few quick comments after looking at the spec for one minute.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Merge Review: php-pear
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=226295
chris.stone@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- AssignedTo|nobody@fedoraproject.org |chris.stone@gmail.com Flag| |fedora-review?
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Merge Review: php-pear
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=226295
chris.stone@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |ASSIGNED
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Merge Review: php-pear Alias: php-pear
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=226295
chris.stone@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Alias| |php-pear
------- Additional Comments From chris.stone@gmail.com 2007-02-02 15:52 EST ------- Joe: please let me know if you plan to upgrade to 1.5.0 before I do a formal review. Thanks.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Merge Review: php-pear Alias: php-pear
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=226295
chris.stone@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- AssignedTo|chris.stone@gmail.com |jorton@redhat.com
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Merge Review: php-pear Alias: php-pear
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=226295
------- Additional Comments From jorton@redhat.com 2007-02-05 10:18 EST ------- The upgrade to 1.5.0 is not important to the review of the packaging of php-pear - can we keep these issues seperate? I will try to get to that sometime this week.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Merge Review: php-pear Alias: php-pear
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=226295
------- Additional Comments From chris.stone@gmail.com 2007-02-05 12:57 EST ------- ==== REVIEW CHECKLIST ==== X rpmlint php-pear-1.4.11-3.noarch.rpm:
W: php-pear non-standard-group System
Trivial: change to "Development/Languages"
W: php-pear invalid-license The PHP License v3.0
Trivial: change to "PHP License 3.0"
W: php-pear conffile-without-noreplace-flag /etc/pear.conf W: php-pear conffile-without-noreplace-flag /etc/rpm/macros.pear
Trivial: Add noreplace flag (or a comment explainging your *good* reason for not adding a noreplace flag)
W: php-pear hidden-file-or-dir /usr/share/pear/.registry/.channel.__uri W: php-pear hidden-file-or-dir /usr/share/pear/.registry/.channel.__uri W: php-pear hidden-file-or-dir /usr/share/pear/.depdblock E: php-pear zero-length /usr/share/pear/.depdblock W: php-pear hidden-file-or-dir /usr/share/pear/.depdb W: php-pear hidden-file-or-dir /usr/share/pear/.lock E: php-pear zero-length /usr/share/pear/.lock W: php-pear hidden-file-or-dir /usr/share/pear/.filemap W: php-pear hidden-file-or-dir /usr/share/pear/.channels W: php-pear hidden-file-or-dir /usr/share/pear/.channels W: php-pear hidden-file-or-dir /usr/share/pear/.pkgxml W: php-pear hidden-file-or-dir /usr/share/pear/.pkgxml W: php-pear hidden-file-or-dir /usr/share/pear/.registry/.channel.pecl.php.net W: php-pear hidden-file-or-dir /usr/share/pear/.registry/.channel.pecl.php.net W: php-pear hidden-file-or-dir /usr/share/pear/.channels/.alias W: php-pear hidden-file-or-dir /usr/share/pear/.channels/.alias W: php-pear hidden-file-or-dir /usr/share/pear/.registry W: php-pear hidden-file-or-dir /usr/share/pear/.registry
Safe to ignore, except for the 0 length ones, do these really need to be in the package?
$ rpmlint php-pear-1.4.11-3.src.rpm W: php-pear non-standard-group System
Trivial fix (see above)
W: php-pear invalid-license The PHP License v3.0
Trivial fix (see above)
W: php-pear setup-not-quiet
Trivial: Add -q to %setup
E: php-pear use-of-RPM_SOURCE_DIR
Never seen this used before, are you using this in place of .?
W: php-pear no-%build-section
Trivial: Add empty build section with a comment saying it is not needed
W: php-pear patch-not-applied Patch1: php-pear-1.4.8-package.patch W: php-pear patch-not-applied Patch0: php-pear-1.4.8-template.patch
Patches that are not applied should be removed, or commented out with an explanation as to why there are still there.
- Package named according to package naming guidelines - spec file name matches %{name} - package meets packaging guidelines (buildroot fixed) - licensed with open source compatible license X license does not match actual license - license included in %doc - spec written in American english - spec file legible X cannot verify upstream sources match, I cannot find upstream source
==== MUST FIX ==== - Fix trivial rpmlint errors and warnings - According to http://pear.php.net/package/PEAR this is licensed under the PHP License 2.02, not 3.0 (please clarify with upstream and/or fix spec) - SOURCE0 must have full URL to find source files
I have to stop the review here because I cannot find the upstream source file you use, please provide a full URL to sources!
Indeed, the upgrade to 1.5.0 *is* important as there should be fewer patches in the spec, and possible build/install changes, and open bugs will be closed.
Please make above mentioned changes and upgrade to 1.5.0 before I continue review.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Merge Review: php-pear Alias: php-pear
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=226295
------- Additional Comments From jorton@redhat.com 2007-02-05 14:02 EST ------- Thanks for the review!
Fixed in -4: - upstream do not provide a permanent archive URL for each version of the .phar installer, so there is no URL to provide (yes, I know this sucks). I'll add a comment to this effect - bogus Group - pear.conf marked noreplace; /etc/rpm/macros.* should never be noreplace (and that should be documented by standard not per spec file)
Won't fix: - passing -q to %setup has no effect when -T is also used - providing an noop %build adds no value - no idea what the issue with $RPM_SOURCE_DIR is, this has been used in spec files forever - the patches are applied using %{PATCHn} syntax - the license specified in every PEAR class file is indeed v3 not v2.02. There is no accepted policy for the License tag in Fedora yet; there is no point tweaking this on a whim until there is, rpmlint is not definitive on that front
Again, the upgrade to the latest upstream is not relevant to the review of the current packaging and need not block the review process. Of course the packaging may change with each new upstream releases, that is always going to be true.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Merge Review: php-pear Alias: php-pear
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=226295
------- Additional Comments From chris.stone@gmail.com 2007-02-05 14:09 EST ------- Quick question: Why do we need to use a .phar archive?
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Merge Review: php-pear Alias: php-pear
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=226295
------- Additional Comments From chris.stone@gmail.com 2007-02-05 14:45 EST ------- (In reply to comment #8)
Thanks for the review!
You're welcome, but we are only about 1/2 way done ;-)
Fixed in -4:
- upstream do not provide a permanent archive URL for each version of the .phar
installer, so there is no URL to provide (yes, I know this sucks). I'll add a comment to this effect
See comment #9, I need to know why using a .phar is required instead of using the .tgz
- bogus Group
Thanks
- pear.conf marked noreplace; /etc/rpm/macros.* should never be noreplace (and
that should be documented by standard not per spec file)
Thanks. Please add a comment, something like: # macros.* should be replaced on updates Remember, you are not the only one who reads the spec file, and should you get hit by a bus, the next php-pear maintainer will appreciate the extra documentation.
Won't fix:
- passing -q to %setup has no effect when -T is also used
Filed a bug against rpmlint (bug #227389). Thanks for pointing this out.
- providing an noop %build adds no value
Yet, rpm has "unpredictable results if not added". You can email Ville Skytta if you need further clarification on this, he has real world examples where this has caused problems. Better to play safe than sorry. *ALL* pear packages contain empty %build sections, php-pear should not be an exception. Again, this is something that takes 5 seconds to add, and causes no harm and helps documentation in the spec for other users. Please add.
- no idea what the issue with $RPM_SOURCE_DIR is, this has been used in spec
files forever
You should install source files using %{SOURCEx} notation, please change your for loop to something like this: install -pm 755 %{SOURCE10} $RPM_BUILD_ROOT%{_bindir}/pear install -pm 755 %{SOURCE11} $RPM_BUILD_ROOT%{_bindir}/pecl install -pm 755 %{SOURCE12} $RPM_BUILD_ROOT%{_bindir}/peardev
In addition, your reference to macros.pear should use %{SOURCE13}
- the patches are applied using %{PATCHn} syntax
My mistake. You are right in this case, pear packages are installed funky and patches have to be done this way.
- the license specified in every PEAR class file is indeed v3 not v2.02. There
is no accepted policy for the License tag in Fedora yet; there is no point tweaking this on a whim until there is, rpmlint is not definitive on that front
Okay, can you give me some kind of proof that _all_ pear classes are automatically licensed with PHP License 3.0? I know of many pear classes that use BSD for example.
The upstream home page actually has a link to the license file which is a version 2.02 license.
I simply cannot approve this until this matter is cleared up. The licenses **MUST** match upstream. If upstream's home page is pointing to the wrong license, then please attach to this bug report an e-mail from upstream saying their home page is incorrect.
Please also remove the "The" and "v" in the License tag. Why not atleast make the licenses consistent to help people who run shell scripts and such parsing license tags? A license of "PHP License 3.0" is no different than "The PHP License v3.0", however the first case is consistent with all other packages licensed with a PHP license.
Again, the upgrade to the latest upstream is not relevant to the review of the current packaging and need not block the review process. Of course the packaging may change with each new upstream releases, that is always going to be true.
Right, so what is the point in reviewing a package that is going to change significantly immediately after it is reviewed? It makes far greater sense to make the significant changes _before_ the review.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Merge Review: php-pear Alias: php-pear
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=226295
------- Additional Comments From chris.stone@gmail.com 2007-02-05 17:45 EST ------- Okay, I've dug a little deeper into this .phar issue, and this is the solution I propose:
Add a -pear subpackage to the php package, and do not use --without-pear flag when compiling php.
This should allow us to have the pear installer without a need to bootstrap.
In addition, we can package the pear classes in seperate spec files using the original .tgz files for each class and can update them independently. And we can also use the standard pear templates to package these classes.
Please let me know if there is a problem with this alternative, I think this will be a cleaner solution.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Merge Review: php-pear Alias: php-pear
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=226295
------- Additional Comments From jorton@redhat.com 2007-02-06 04:21 EST ------- - the source code files distributed in PEAR carry a copyright notice with the v3 license. Those copyright notices alone define how the source code is licensed, and the License tag on the package should reflect that and only that.
- the .phar archive is the only way found to bootstrap PEAR
- it is desirable to have PEAR build from a separate source RPM; it has an independent upstream (and hence release cycle) to PHP, and we can ship updates to this small self-contained part without revving the entire PHP package (which is large, fragile, and needs lots of QA for every update). There is insufficient motivation here to change that.
- whatever package gets reviewed is subject to change as new upstream releases come out. 1.4.11 or 1.5.0 or 1.6.0. 1.4.11 is what's in Fedora at the moment and what's up for review.
- the License tag, presence or lack of %build, $RPM_SOURCE_DIR vs %{SOURCEn} - changing these is a bikeshed painting exercise unless and until specified by a Fedora packaging guideline.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Merge Review: php-pear Alias: php-pear
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=226295
------- Additional Comments From chris.stone@gmail.com 2007-02-06 12:09 EST ------- (In reply to comment #12)
- the source code files distributed in PEAR carry a copyright notice with the v3
license. Those copyright notices alone define how the source code is licensed, and the License tag on the package should reflect that and only that.
Indeed the source files specify the 3.0 license and this carries more weight than the license linked to on the web site. Thank you for clarifying this.
- the .phar archive is the only way found to bootstrap PEAR
Indeed, however the point is we do not need to bootstrap PEAR.
- it is desirable to have PEAR build from a separate source RPM; it has an
independent upstream (and hence release cycle) to PHP, and we can ship updates to this small self-contained part without revving the entire PHP package (which is large, fragile, and needs lots of QA for every update). There is insufficient motivation here to change that.
Okay, now you are contradicting yourself. There are several points I should make in reards to this statement:
1) It is desirable to have the PEAR class separate from php, (NOT the pear installer). Therefore we should have a php-pear-PEAR rpm that contains ONLY the PEAR class. The PEAR *class* is what is updated frequently, and the pear installer does *NOT* get updated frequently.
2) It makes perfect sense to include the pear INSTALLER with the php class. This has several very important benefits. First it allows us to package the classes individually using the standard default template spec files. It allows us to use the standard .tgz files for the classes. This allows us better auditing and allows us better freedom for updates to the classes. And all the classes will be packaged in seperate SRPMS for better consistency and just makes sense.
3) There is HUGE motivation to change this. I do not understand why you do not see it.
4) The bootstrapping method uses a source file which changes at every single release, there is no way for someone to download an old version of the bootstrap code.
5) The bootstrap code bunches several pear classes together in a large lump sum which actually makes it more difficult to make incremental updates which contradicts the reasoning you mention above.
6) PEAR 1.5.0 adds a gtk and web interface, by using the bootstrapping method you again bring in all these extra packages (some of which are already packaged seperately) into a single lump sum. This even makes it more desirable to NOT use the bootstrapping method and to simply add the installer in the php package.
7) with PEAR 1.5.0 there are already sepearate packages classes which PEAR now depends on therefore making the bootstrapping method conflict and clash with existing packages.
8) The bottom line is that this *has* to be done. We can no longer use the bootstrap method because it is conflicting with too many existing packages and it is simply causes a big mess and many problems. It has to be done this way. If you need help I will be glad to help you.
- whatever package gets reviewed is subject to change as new upstream releases
come out. 1.4.11 or 1.5.0 or 1.6.0. 1.4.11 is what's in Fedora at the moment and what's up for review.
Sir, I am getting sick and tired of explaining this to you and you are trying my patience!
Pear 1.5.0 is going to bring on many signifacnt changes, it adds php-gtk, and a web installer among other things. This means it is even more important to break these packages up into sepearate packages and to include the pear installer in the php package like I suggested. These changes are significant enough for me to demand that they be done before the formal review.
We are going to be working together with this and major changes to how php and php-pear are packaged are going to have to be made. I am not here to make your life difficult, I am here to try and assist you on the best way to package this. I hope that you understand the bootstrap method is just simply no longer going to work, especially now with pear 1.5.0.
- the License tag, presence or lack of %build, $RPM_SOURCE_DIR vs %{SOURCEn} -
changing these is a bikeshed painting exercise unless and until specified by a Fedora packaging guideline.
Mr Orton, I have already brought this up with fedora packaging members and several the suggestions I have made come straight from the packaging committee! If you want to bring this up to them again personally, I can do that.
So instead of spending 10 seconds making a simple benign change, you want to be stubborn and escalate this to the packaging committee and waste their time with this nonsense? We can do that if you want, but I strongly urge you to put your ego aside and just make the simple benign trivial changes.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Merge Review: php-pear Alias: php-pear
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=226295
jorton@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |rpm@timj.co.uk
------- Additional Comments From jorton@redhat.com 2007-02-06 13:06 EST ------- I can't follow most of that. The PEAR class *is* the PEAR installer: they are one and the same thing. All the PHP tarball does to "install PEAR" is to run a .phar exactly like php-pear.spec does.
The only .phar available is 1.5.0RC2, and the .tgz for PEAR seems to call itself 1.5.0RC3 in the package.xml - CC added for timj; Tim, are you in a position to fix this upstream?
php-pear.spec seems to work fine with zero lines of packaging changes with that installer anyway, so what is the big deal with the upgrade? The non-CLI Frontends should be packaged separately from php-pear; I don't see how that is relevant.
I have no philosophical objection to moving the PEAR packages bundled with the .phar into subpackages, I'm just not convinced it's technically worth it; XML_RPC at least probably is.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Merge Review: php-pear Alias: php-pear
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=226295
------- Additional Comments From rpm@timj.co.uk 2007-02-06 13:18 EST ------- I will respond a bit more later but for now some background for Chris and other interested parties re: bootstrapping/separation:
- bug #173810 - http://www.zend.com/lists/pear-dev/200604/msg00153.html
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Merge Review: php-pear Alias: php-pear
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=226295
rpm@timj.co.uk changed:
What |Removed |Added ---------------------------------------------------------------------------- External Bug| |http://bugs.php.net/show_bug Reference| |.cgi?id=10038
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Merge Review: php-pear Alias: php-pear
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=226295
rpm@timj.co.uk changed:
What |Removed |Added ---------------------------------------------------------------------------- External Bug| |http://pear.php.net/bugs//sh Reference| |ow_bug.cgi?id=10038 External Bug|http://bugs.php.net/show_bug%7C Reference|.cgi?id=10038 |
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Merge Review: php-pear Alias: php-pear
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=226295
------- Additional Comments From rpm@timj.co.uk 2007-02-06 13:51 EST ------- The upstream license is definitively PHP License 3.0. Every source file has that. The license link on the package home page is bogus and is a bug in the web UI of PEAR - filed upstream : bug http://pear.php.net/bugs/10038 .
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Merge Review: php-pear Alias: php-pear
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=226295
------- Additional Comments From chris.stone@gmail.com 2007-02-06 14:14 EST ------- (In reply to comment #14)
I can't follow most of that. The PEAR class *is* the PEAR installer: they are one and the same thing. All the PHP tarball does to "install PEAR" is to run a .phar exactly like php-pear.spec does.
I had build the php package after removing the --without-pear option and it only installed a /usr/bin/pear command (nothing under /usr/share/pear).
This led me to believe it was different than the .phar install.
I guess we are back to square one then.
Basically now it is just bootstrapping from php, or from php-pear package. Atleast with php we have a standard official tarball to compare against.
So what is the plan with 1.5.0 then? Are you planning on including the web and/or gtk installer or are these optional? According to the web site they are not optional.
I now understand your reasonging for the php-pear package and agree with you now (even though I think it sucks, but that is pear's fault not ours).
This being said, there are _still_ major changes for the 1.5.0 package and these should be done before I do the review. I don't want to hear any B.S. about having to review what is currently in CVS.
I also want to verify that the .phar does not bundle packages that already exist such as Net-UserAgent-Detect package which is now required for the web installer. I'm hoping this wont be a problem if the web installer is optional and not included in the .phar.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Merge Review: php-pear Alias: php-pear
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=226295
------- Additional Comments From rpm@timj.co.uk 2007-02-06 14:39 EST ------- OK some comments from me (numbering not related to numbering in above bugs):
1. Chris, I'm not sure what your proposal re: splitting the "PEAR class" from the "PEAR installer" is. Are you referring specifically to the file PEAR.php? If so, whilst it's true that:
a) looking at CVS this hasn't changed for a while b) the PEAR and PEAR_Error classes contained within it are often required by other PEAR classes,
I'm not sure that it makes any sense to split this one file off and do something separate with it. The distinction is not made upstream, so we shouldn't make it. (If you think there's justification upstream, please file a bug there). In any case, the PEAR installer requires the PEAR class, so we wouldn't gain anything useful that I can see. The PEAR command line installer is an intrinsic part of the PEAR package and uses most of the files from it; it is not a distinct "binary".
2) I disagree with your assertion that the PEAR installer is not updated frequently. Most of the "PEAR package" is related to the PEAR installer. For example, PEAR 1.5.0 fixes some critical bugs related to installing certain packages and the entire point of bug #173810 was to split this away from the PHP release cycle so that we can upgrade the PEAR installer in Fedora without having to wait for the next upstream PHP release.
3) I agree with the principle of splitting sub-packages out, particularly XML_RPC. However, this requires that we get a better bootstrapping method. I do not consider reverting to the PHP-release-lockin a better method. The best example of an alternative method which supports sub-packages without locking PEAR to PHP releases is over at PLD (similar method used by Mandriva):
http://cvs.pld-linux.org/cgi-bin/cvsweb.cgi/SPECS/php-pear-PEAR.spec
This was discussed last year. Joe objected (I can't find a reference; it may have been in an offline discussion) to this method on the basis that it requires too much intimate knowledge of PEAR. I think that point is a fair one, although personally I think the above (PLD) method may be the lesser of two evils, which has the bonus of eliminating the PHAR problem. However, since Joe has been the maintainer of the php-pear package I have deferred to his judgement. That said, I do think now is an appropriate time to revisit this issue.
Re: the PHAR bootstrapping: CS> The bootstrapping method uses a source file which changes at every single CS> release, there is no way for someone to download an old version of the CS> bootstrap code.
This does indeed suck but is a problem upstream which, if fixed, would negate this point. However my personal preferred bootstrap method would be the PLD one, which would also eliminate this problem, since that is based on the standard released PEAR tarball (i.e. PEAR-x.y.z.tgz) and therefore benefits from all the normal PEAR package release mechanism (archived versions, changelog, no special treatment required to keep it up to date) with the added bonuses that - it is readable with standard tools - it backports more easily to platforms < PHP 5.1 (for example RHEL4 - yes, I know RHEL4 probably isn't going to update PEAR, but there are people like me running EL derivatives with backported versions of key packages like PHP/PEAR etc.)
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Merge Review: php-pear Alias: php-pear
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=226295
------- Additional Comments From rpm@timj.co.uk 2007-02-06 14:49 EST ------- Chris, can you cite a source for the web + GTK installer not being optional with PEAR 1.5.0? I have PEAR 1.5.0 installed (via "pear upgrade", not via RPM) on a machine here and I don't have GTK or web installers.
You are perhaps being confused by the wording on http://pear.php.net/package/PEAR/download/1.5.0 which says:
" Dependencies: ... - PEAR Package: PEAR_Frontend_Web - PEAR Package: PEAR_Frontend_Gtk "
However, these are only *suggestions*. They are marked as "optional" in the package.xml file in the tarball and are not *required*. (The PEAR website should make this clearer; I will pursue that upstream)
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Merge Review: php-pear Alias: php-pear
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=226295
------- Additional Comments From chris.stone@gmail.com 2007-02-06 14:57 EST ------- (In reply to comment #18)
OK some comments from me (numbering not related to numbering in above bugs):
- Chris, I'm not sure what your proposal re: splitting the "PEAR class" from
the "PEAR installer" is. Are you referring specifically to the file PEAR.php? If so, whilst it's true that:
As I mentioned in comment #17, I compiled php after removing the --without-pear option and it only added a /usr/bin/pear command, it did not add anything in /usr/share/pear
This lead me to believe the pear installer from php was different than the pear class.
It was a misunderstanding on my part and all my arguments for putting pear in the php spec file are no longer valid.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Merge Review: php-pear Alias: php-pear
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=226295
------- Additional Comments From chris.stone@gmail.com 2007-02-06 14:59 EST ------- (In reply to comment #19)
Chris, can you cite a source for the web + GTK installer not being optional with PEAR 1.5.0? I have PEAR 1.5.0 installed (via "pear upgrade", not via RPM) on a machine here and I don't have GTK or web installers.
You are perhaps being confused by the wording on http://pear.php.net/package/PEAR/download/1.5.0 which says:
Yes, I was basing it off this wording, all other pear sites indication optional packages with "(optional)". Even PEAR itself says XML_RPM is optinal but does not mention this for the gtk or web installer parts.
I'm glad to know that they are not actually required. This simplifies things tremendously.
However due to the number of bugs being fixed and patches no longer required, I am still going to insist on an updated 1.5.0 package before I complete the review.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Merge Review: php-pear Alias: php-pear
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=226295
------- Additional Comments From rpm@timj.co.uk 2007-02-06 17:04 EST ------- In reply to comment #21)
Yes, there is some weird stuff going on upstream - optional deps are not being marked as such. Upstream bug filed: http://pear.php.net/bugs/bug.php?id=10040
If I can summarise where we are with this review, I think the following 4 issues are outstanding at this point:
- $RPM_SOURCE_DIR vs %{SOURCEx}. Joe, I take your point about painting the bikesheds although the latter form is clearer to me too as it ties up the SourceX files in the list with what's actually going on in the spec - License tag. This is trivial stuff. There is a DRAFT proposal by Jason T regarding general cleanups here: http://www.fedoraproject.org/wiki/PackagingDrafts/LicenseTag . Personally I don't think this is a blocker either way, at least in the absence of an approved guideline from the committee one way or another. - empty build section. This seems pointless to me but has been done to death on fe-list and my understanding of the conclusion is that empty build sections are stupid but necessary to avoid some kind of obscure bug - bootstrapping: I think this is the key technical issue this review should focus on. I think we're all agreed that splitting off subpackages is a good concept, to aid modularity and allow individual upgrades. Chris has offered to do any extra work required, so that's cool. However with the current (PHAR) based bootstrapping, the XML_RPC, Console_Getopt and Archive_Tar subpackages are pretty much required as part of PEAR. Switching to an alternative packaging methodology (e.g. PLD) is one possible solution to this.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Merge Review: php-pear Alias: php-pear
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=226295
------- Additional Comments From chris.stone@gmail.com 2007-02-06 17:54 EST ------- That is basically a summary of the review so far. However, I only got about 1/4 the way through the review checklist. I was unable to verify the upstream sources. This is another reason I wish to wait for 1.5.0 before finishing the review checklist. It will allow me to actually verify the upstream sources.
I'm okay with the bootstrapping how it is done now, however, the PLD technique sounds interesting and should be investigated. I think it would be beneficial to have pear packages seperated, ie php-pear and php-pear-PEAR, php-pear-Archive-Tar, etc. That being said, I'm not going to block this review on implementing this feature.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Merge Review: php-pear Alias: php-pear
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=226295
------- Additional Comments From rpm@timj.co.uk 2007-02-06 18:20 EST ------- I'm happy to spend the time getting the PLD method working (including splitting off Archive_Tar, XML_RPC and Console_Getopt) in Fedora style, but only if we can jointly agree in principle that that's a good thing, and I know Joe was against it before, which is why we need to discuss that. There is an interesting circular dependency there too (PEAR <-> Archive_Tar) which could be fun with mock.
I can't readily see how php-pear and php-pear-PEAR could/would be separated, or what that would mean. Remember too (further to my earlier comments) that the "PEAR installer" isn't just the CLI frontend: it's also a PHP library backend for integration with other software - calls direct to the installer's back-end API *are* used by other packages (for example PEAR_Command_Packaging, or the not-yet-RPM-packaged s9y). That said, the current php-pear package does Provide php-pear(PEAR), so if such a thing did become meaningful in future, we already have the mechanism to do it in a reasonably BC way.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Merge Review: php-pear Alias: php-pear
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=226295
------- Additional Comments From jorton@redhat.com 2007-02-07 11:44 EST ------- The PLD PEAR stuff is half hidden in obfuscated RPM macros shipped in the rpm package (!?), it took me a while to try and read through what they actually do again.
It is a lot of code to achieve what we do in one PHP invocation running the .phar. They have to fake up a PEAR environment and configuration, unpack the tgz, do a fake --nodeps install, then copy that environment out to the buildroot.
I can't see how they get a lot of stuff right:
- they create .filemap via "touch" so presumably the files list in the PEAR database omits the PEAR package itself - they don't relocate any of the installed PEAR database files - the pear.conf they install is AFAICT the fake one; we use the fully-populated one created by PEAR itself
The whole thing looks overcomplicated and fragile. Bootstrapping from the .phar is simple and low-maintenance; it's the only method for bootstrapping PEAR actually supported by upstream to boot (whether it comes via go-pear.org or with the PHP tarball).
The only thing which sucks about using the .phar is upstream's lack of release archive, which really makes no difference to the packaging. So my choice definitely remains with using the .phar.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Merge Review: php-pear Alias: php-pear
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=226295
------- Additional Comments From jorton@redhat.com 2007-02-08 06:11 EST ------- To recap: just waiting on availability of the 1.5.0 .phar from upstream; Tim are you able to push that out?
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Merge Review: php-pear Alias: php-pear
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=226295
------- Additional Comments From rpm@timj.co.uk 2007-02-08 18:43 EST ------- I can't do that at the moment I'm afraid, however if you file a bug upstream I should imagine it will be fixed fairly quickly.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Merge Review: php-pear Alias: php-pear
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=226295
------- Additional Comments From chris.stone@gmail.com 2007-02-13 18:14 EST ------- I think the phar has been updated to 1.5.0 now, please verify.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Merge Review: php-pear Alias: php-pear
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=226295
chris.stone@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |NEEDINFO Flag| |needinfo?(jorton@redhat.com)
------- Additional Comments From chris.stone@gmail.com 2007-02-15 14:28 EST ------- ping?
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Merge Review: php-pear Alias: php-pear
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=226295
Fedora@FamilleCollet.com changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |Fedora@FamilleCollet.com
------- Additional Comments From Fedora@FamilleCollet.com 2007-02-17 01:47 EST ------- Hi, some remarks and questions
I saw php-pear-1.5.0 in rawhide.
Provides should be update to version actualy provided by .phar : Provides: php-pear(Console_Getopt) = 1.2.1 Provides: php-pear(Archive_Tar) = 1.3.2 Provides: php-pear(Structures_Graph) = 1.0.2
patch0 could be remove as "pear makerm" is obsolete and remove (remplace by "pear make-rpm-spec" from PEAR_Command_Packaging) even if template.spec still present (no response from "find /usr/share/pear/ -name *.php -exec grep "template.spec" {} ; -print")
Big question : is it ok to provide a default pear.conf that point to non user writable directory, and to have 2 uses for the same directory : download_dir = /var/cache/php-pear cache_dir = /var/cache/php-pear This are not useful for most user (first by apache). Changing permissions on this directory is not a solution as files created in this are only 644.
Regards
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Merge Review: php-pear Alias: php-pear
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=226295
jorton@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|NEEDINFO |ASSIGNED Flag|needinfo?(jorton@redhat.com)|
------- Additional Comments From jorton@redhat.com 2007-02-19 14:01 EST ------- Thanks Remi - I've updated as you indicate.
w.r.t download_dir + cache_dir - is there actually an issue here? I can't see one: e.g. "pear install" is not going to work for non-root users anyway.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Merge Review: php-pear Alias: php-pear
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=226295
------- Additional Comments From rpm@timj.co.uk 2007-02-19 17:17 EST ------- Actually, "pear install" is very useful for non-root users to deploy localised PEAR installs or installs or applications which use the PEAR installer, in combination with options specifying e.g. php_dir etc. That said, in that case they will probably have a customised pearrc which can/could/would override cache_dir and download_dir.
I suppose what would be coolest is some run-time substitution of a "current user" part in download_dir and cache_dir, but that's almost certainly overcomplicated.
I don't think it's harmful having cache_dir = download_dir.
**Highly** recommended reading for all those interested in this bug is ISBN 1904811191 ("The PEAR Installer Manifesto: Revolutionizing PHP Application Development and Deployment" by the upstream lead Greg Beaver)
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Merge Review: php-pear Alias: php-pear
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=226295
------- Additional Comments From chris.stone@gmail.com 2007-02-21 14:44 EST ------- ==== REVIEW CHECKLIST ====
W: php-pear invalid-license The PHP License v3.0 W: php-pear invalid-license The PHP License v3.0
Trivial fix.
W: php-pear conffile-without-noreplace-flag /etc/rpm/macros.pear
okay.
W: php-pear hidden-file-or-dir /usr/share/pear/.registry/.channel.__uri W: php-pear hidden-file-or-dir /usr/share/pear/.registry/.channel.__uri W: php-pear hidden-file-or-dir /usr/share/pear/.depdblock E: php-pear zero-length /usr/share/pear/.depdblock W: php-pear hidden-file-or-dir /usr/share/pear/.depdb W: php-pear hidden-file-or-dir /usr/share/pear/.lock E: php-pear zero-length /usr/share/pear/.lock W: php-pear hidden-file-or-dir /usr/share/pear/.filemap W: php-pear hidden-file-or-dir /usr/share/pear/.channels W: php-pear hidden-file-or-dir /usr/share/pear/.channels W: php-pear hidden-file-or-dir /usr/share/pear/.pkgxml W: php-pear hidden-file-or-dir /usr/share/pear/.pkgxml W: php-pear hidden-file-or-dir /usr/share/pear/.registry/.channel.pecl.php.net W: php-pear hidden-file-or-dir /usr/share/pear/.registry/.channel.pecl.php.net W: php-pear hidden-file-or-dir /usr/share/pear/.channels/.alias W: php-pear hidden-file-or-dir /usr/share/pear/.channels/.alias E: php-pear zero-length /usr/share/pear/test/Structures_Graph/tests/README W: php-pear hidden-file-or-dir /usr/share/pear/.registry W: php-pear hidden-file-or-dir /usr/share/pear/.registry
these are okay
E: php-pear non-executable-script /usr/share/pear/test/Structures_Graph/tests/all-tests.php 0644 E: php-pear non-executable-script /usr/share/pear/doc/Structures_Graph/docs/generate.sh 0644 E: php-pear non-executable-script /usr/share/pear/data/Structures_Graph/genpackage.xml.pl 0644 E: php-pear non-executable-script /usr/share/pear/data/Structures_Graph/publish.sh 0644 E: php-pear non-executable-script /usr/share/pear/data/Structures_Graph/package.sh 0644
Trivial fix
W: php-pear setup-not-quiet
okay
E: php-pear use-of-RPM_SOURCE_DIR
Trivial fix
W: php-pear no-%build-section
Must fix. Trivial
- package named according to package naming guidelines - spec file name matches %{name} - package meets packaging guidelines - licensed with open source compatible license - license matches actual license - license file included in %doc - spec written in American english - spec file legible - sources match upstream d7ec831fe6439fae783b56bd8499ee17 install-pear-nozlib.phar - package successfully compiles and builds on FC-6 x86_64 - all build depenedencies listed in BR - no locales - no shared library files - not relocatable - package owns all directories it creates - directories it does not created owned by filesystem or rpm - no duplicates in %files - file permissions set properly - contains proper %clean - macro usage consistent - contains code - no large documentation - files in %doc do not affect runtime - no header files or static libraries - no pkgconfig files - no libraries with suffixes - no devel subpackage requried - no libtool archives - not a GUI app - package does not own files or directories owned by other packages
==== MUST FIX ==== - MUST add empty %build section. I simply cannot approve this with a straight face when every single other pear package is requried to have this. Either we go through all the trouble to fix the pear template, or you simply add an empty %build. - Fix non-executable-script rpmlint warnings - Change Source0 to: http://pear.php.net/install-pear-nozlib.phar or else add this URL in a comment above Source0
==== SHOULD FIX ==== - Change license tag to "PHP License" or "PHP Licnese 3.0" - Remove RPM_SOURCE_DIR from spec file. This is a community project, not a Joe Orton project, and consistent thinking is the hobgoblin of a feeble mind (or something like that).
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Merge Review: php-pear Alias: php-pear
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=226295
------- Additional Comments From rpm@timj.co.uk 2007-03-06 04:51 EST ------- The BuildRequire on php should be php-cli. Otherwise when the php package is modified to finally drop the "Requires: php-cli" (can we do this for FC7?), the php-pear build will break as it is trying to use the CLI version of PHP.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Merge Review: php-pear Alias: php-pear
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=226295
------- Additional Comments From jorton@redhat.com 2007-03-06 09:36 EST ------- The -3 release adds the redundant %build and changes the BR to php-cli as Tim suggests.
- I haven't looked at the new exec-script warnings in detail, at least some of those files look like they shouldn't be packaged. - There's been a comment regarding the source-of-the-Source for a rev or too. - The f-m@ flamewar demonstrated a clear lack of consensus in the community w.r.t $RPM_SOURCE_DIR. - License tag changes are waiting for an agreed standard on License tags.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Merge Review: php-pear Alias: php-pear
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=226295
bugzilla@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Severity|normal |medium Priority|normal |medium Product|Fedora Extras |Fedora
------- Additional Comments From chris.stone@gmail.com 2007-07-03 00:13 EST ------- Joe, whats up here? I need pear 1.6.0 in devel ... Whats going on with you?
PING.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Merge Review: php-pear Alias: php-pear
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=226295
------- Additional Comments From fedora@famillecollet.com 2007-07-03 01:22 EST ------- Some comments (i use latest pear for a while).
1/ install-pear-nozlib.phar only available for 1.5.4 (i'm used to create my own for my need)
2/ It seems there is a problem when packaging extensions using PEAR 1.6.1 :
rpmbuild fails with an error : download directory "/var/cache/php-pear" is not writeable. Change download_dir config variable to a writeable dir
So we need to investigate this before requiring devel update. For my latest spec, i used : %{__pear} -d download_dir=/tmp install --nodeps --packagingroot ...
Regards
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Merge Review: php-pear Alias: php-pear
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=226295
------- Additional Comments From jorton@redhat.com 2007-07-03 05:00 EST ------- Let's use bug 246588 to track the 1.6.x update; Remi can you add your comments on the problems there? To be clear, this is a problem with PEAR packages not the 1.6.x php-pear source RPM build itself?
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=226295
Christopher Stone chris.stone@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Depends on| |468255 Flag|fedora-review? |
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=226295
Christopher Stone chris.stone@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |NEW
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=226295
Christopher Stone chris.stone@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- AssignedTo|jorton@redhat.com |nobody@fedoraproject.org
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=226295
Bug 226295 depends on bug 468255, which changed state.
Bug 468255 Summary: doc files are not in %doc https://bugzilla.redhat.com/show_bug.cgi?id=468255
What |Old Value |New Value ---------------------------------------------------------------------------- Status|NEW |CLOSED Resolution| |WONTFIX Resolution|WONTFIX | Status|CLOSED |ASSIGNED
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=226295
Jon Ciesla limburgher@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |ASSIGNED CC| |limburgher@gmail.com AssignedTo|nobody@fedoraproject.org |limburgher@gmail.com Flag| |fedora-review?
Bug 226295 depends on bug 468255, which changed state.
Bug 468255 Summary: doc files are not in %doc https://bugzilla.redhat.com/show_bug.cgi?id=468255
What |Old Value |New Value ---------------------------------------------------------------------------- Resolution| |WONTFIX Status|ASSIGNED |CLOSED
--- Comment #39 from Jon Ciesla limburgher@gmail.com 2012-04-03 13:53:13 EDT --- Fresh review:
Good:
- rpmlint checks return:
php-pear.spec: W: patch-not-applied Patch0: php-pear-1.9.4-restcache.patch A patch is included in your package but was not applied. Refer to the patches documentation to see what's wrong.
Fix.
php-pear.noarch: W: self-obsoletion php-pear-XML-Util <= 1.2.1 obsoletes php-pear-XML-Util = 1.2.1-5.fc18 The package obsoletes itself. This is known to cause errors in various tools and should thus be avoided, usually by using appropriately versioned Obsoletes and/or Provides and avoiding unversioned ones.
Fix.
php-pear.noarch: E: incorrect-fsf-address /usr/share/doc/php-pear-1.9.4/LICENSE-Structures_Graph php-pear.noarch: E: incorrect-fsf-address /usr/share/pear/data/Structures_Graph/LICENSE The Free Software Foundation address in this file seems to be outdated or misspelled. Ask upstream to update the address, or if this is a license file, possibly the entire file with a new copy available from the FSF.
Trivial fix.
Same hidden, zerolength and config-not-noreplace as above, still ok.
php-pear.noarch: W: file-not-utf8 /usr/share/doc/pear/XML_Util/examples/example.php The character encoding of this file is not UTF-8. Consider converting it in the specfile's %prep section for example using iconv(1).
Fix.
- package meets naming guidelines - package meets packaging guidelines - license ( BSD and PHP and LGPLv2+ ) OK, text in %doc, matches source - spec file legible, in am. english - source matches upstream - package compiles on devel (x86_64) - no missing BR - no unnecessary BR - no locales - not relocatable - owns all directories that it creates - no duplicate files - permissions ok - %clean ok - macro use consistent - code, not content - no need for -docs - nothing in %doc affects runtime - no need for .desktop file
This is really pretty good, just the self-obsoletion, patch, license address and encoding. Let me know if you want me to commit any of these fixes.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=226295
--- Comment #40 from Remi Collet fedora@famillecollet.com 2012-04-04 01:21:51 EDT --- Jon, can you attach the proposed fixes to this bug reports ?
About patch-not-applied, patch is applied, but in %install (yes, this is uggly) Moving it to %prep is possible, of course (but patched file will need to be pushed in buildroot after install, which use the tarball, not the uncompressed files)
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=226295
--- Comment #41 from Remi Collet fedora@famillecollet.com 2012-04-04 01:40:17 EDT --- About License files.
I think we should never alter the upstream provided file, and as rpmlint say "Ask upstream to ..." Done: https://pear.php.net/bugs/bug.php?id=19367
License files are installed twice, ex: /usr/share/doc/php-pear-1.9.4/LICENSE-Structures_Graph (rpm standard, but for file not installed by upstream) /usr/share/pear/data/Structures_Graph/LICENSE (pear standard)
We should probably keep only 1 copy (the pear standard, as all the other pear packages, according to PHP Guidelines)
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=226295
--- Comment #42 from Remi Collet fedora@famillecollet.com 2012-04-04 01:49:37 EDT --- Arg.. reading my previous post, it seems LICENSE is mark as "data" instead of "doc" (quite usual error in pear) => comment added to the previous BUG.
About LICENSE-XML_RPC, as file is not provided by upstream, and according to Guidelines, we don't have to add it, but request Upstream to do it Done: https://pear.php.net/bugs/bug.php?id=19368
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=226295
--- Comment #43 from Remi Collet fedora@famillecollet.com 2012-04-04 01:53:49 EDT --- @Joe, about the php-pear-1.9.4-restcache.patch (first applied in RHEL) do you have a link to an upstream bug we could add, as comment, in the spec, and track for future version ?
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=226295
--- Comment #44 from Jon Ciesla limburgher@gmail.com 2012-04-04 10:41:34 EDT --- I think all that needs to be done now is around line 55:
Obsoletes: php-pear-XML-Util <= %{xmlutil} Provides: php-pear-XML-Util = %{xmlutil}-%{release}
I think you just need to change it to:
Obsoletes: php-pear-XML-Util < %{xmlutil}-%{release} Provides: php-pear-XML-Util = %{xmlutil}-%{release}
Unless you can drop it altogether.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=226295
--- Comment #45 from Remi Collet fedora@famillecollet.com 2012-04-04 11:48:09 EDT --- Done http://pkgs.fedoraproject.org/gitweb/?p=php-pear.git;a=commitdiff;h=ad7c6d7b...
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=226295
Jon Ciesla limburgher@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |CLOSED Resolution| |NOTABUG Flag|fedora-review? |fedora-review+ Last Closed| |2012-04-04 12:01:57
--- Comment #46 from Jon Ciesla limburgher@gmail.com 2012-04-04 12:01:57 EDT --- Excellent, thanks! APPROVED, closing.
package-review@lists.fedoraproject.org
-
Red Hat Bugzilla