Bug 226295 (php-pear)

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.

I don't see why this would be particularly useful; the PEAR install-pear-*.phar
bootstraps itself without needing existing PEAR classes already.

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.

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.

Joe: please let me know if you plan to upgrade to 1.5.0 before I do a formal
review.  Thanks.

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.

==== 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.

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.

Quick question:  Why do we need to use a .phar archive?

(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.

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.

- 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.

(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.

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.

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

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 .

(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.

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.)

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)

(In reply to comment #18)
> 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:

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.

(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.

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.

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.

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.

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.

To recap: just waiting on availability of the 1.5.0 .phar from upstream; Tim are
 you able to push that out?

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.

I think the phar has been updated to 1.5.0 now, please verify.

ping?

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

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.

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)

==== 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).

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.

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.

Joe, whats up here?  I need pear 1.6.0 in devel ...
Whats going on with you?  

PING.

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

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?

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.

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)

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)

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

@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 ?

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.

Done
http://pkgs.fedoraproject.org/gitweb/?p=php-pear.git;a=commitdiff;h=ad7c6d7b7871c27c3b8a2b07a9afc9f4be4cf609

Excellent, thanks!  APPROVED, closing.