Add PHP5 support (i.e., ready for FC4 and devel).

PHP5 & FC4/devel:
SRPM: http://dmitry.butskoy.name/php-extras/php-extras-5.0.4-0.src.rpm
SPEC: http://dmitry.butskoy.name/php-extras/php-extras.spec.fc4

PHP4 & FC3:
SRPM: http://dmitry.butskoy.name/php-extras/php-extras-4.3.11-0.src.rpm
SPEC: http://dmitry.butskoy.name/php-extras/php-extras.spec.fc3

.spec files are mostly identical, just changed in default modules lists.

You can omit defaults if specify "--without default --with <module_name> " etc.

*** Bug 169544 has been marked as a duplicate of this bug. ***

The spec file is way too messy :
- The conditionals (_with_*/_without_*) are used in a wrong way since modules
cannot be enabled or disabled on the rpmbuild command line
- Setting the php extension dir to /var/tmp if the php-config execution to get
it fails is a bad idea (as it will lead to a really broken package)
- Same goes for the version defaulting to "Unknown"... eventually, just make the
build fail instead, as a correct php version will be mandatory
- Using %{sumary} in the %description should be avoided, since nothing in the
spec file syntax says if %{summary} references the last set one or the main
one... so if it works, it's pure luck and might break some day
- Add the sub-package headers (from %package to %description, included) don't
need to be wrapped in %if/%endif as they will simply be ignored if they don't
have a matching %files section
- The CFLAGS export seems unneeded, unless -fno-strict-aliasing is mandatory, in
which case it probably should be considered a bug
- The readline LIBS hack should probably only be set temporarily in the for loop
when the current module it the readline one in order to avoid any nasty side
effects (other modules could get linked to ncurses?)
- The %check section is ugly, probably needs cleaning up, and could at least use
some explicit comments about what is being done and why
- The per .so file %attr(755,root,root) is completely redundant with the above
"install -m755" + %defattr(-,root,root) and should be removed for a better
readability

This all needs to be improved/fixed. Please let me know if you want me to take a
stab at it.

> - The conditionals (_with_*/_without_*) are used in a wrong way since modules
> cannot be enabled or disabled on the rpmbuild command line
Improved, now should work

> - Setting the php extension dir to /var/tmp if the php-config execution to get
> it fails is a bad idea (as it will lead to a really broken package)
> - Same goes for the version ...
Just removed "else" things. If no php-config present, user will be stopped by
"empty Vesrion tag" error.

> Using %{summary} ...
changed to explicit text.

> - Add the sub-package headers (from %package to %description, included) don't
> need to be wrapped in %if/%endif as they will simply be ignored if they don't
> have a matching %files section
If I remove "%if/%endif" wraps, all existing BuildRequires will take effect,
which I prefer to avoid.

> - The CFLAGS export seems unneeded
Just preserve the main php package behaviour here (both php4 and php5 use it).

> - The readline LIBS hack ...
removed, a patch added instead.

> - The %check section is ugly, probably needs cleaning up, ...
Thanks! I've spent two days to find the most compact working variant... :)
> use some explicit comments about what is being done and why
surely, add some comments

> - The per .so file %attr(755,root,root)
removed

SPEC/SRPM updated (the same urls as above).


> Please let me know if you want me to take a stab at it.

Matthias,

I'll be happy if you wanna review this!

ping -b somebody! :)

ping -f Matthias || ping -b Somebody  :)

Sorry for the lag :-)
I've taken a look at the current fc4 spec file. It looks much better!

Questions :
- Why to you force -Wall -fno-strict-aliasing? Do things break without?
- Why do you export EXTENSION_DIR for the build? Is it really needed?
- Isn't --enable-shared --disable-static the default after phpize?

All the rest is quite clean, well commented now, and pretty nifty too ;-)

> ping -f Matthias || ping -b Somebody  :)
I always knew, that "ping -f" is very effective!  :)

> Why to you force -Wall -fno-strict-aliasing? Do things break without?
These things are present in the original php package for Fedora Core. I don't
know explicitly why, but IMHO I should repeat them too to avoid possibly unknown
issues.

> Why do you export EXTENSION_DIR for the build? Is it really needed?
Yes, it is unneeded.

> Isn't --enable-shared --disable-static the default after phpize?
Yep, remove extra args.

New SRPM & SPEC:

PHP5 & FC4/devel:
http://dmitry.butskoy.name/php-extras/php-extras-5.0.4-1.src.rpm
http://dmitry.butskoy.name/php-extras/php-extras.spec.fc4

PHP4 & FC3:
http://dmitry.butskoy.name/php-extras/php-extras-4.3.11-1.src.rpm
http://dmitry.butskoy.name/php-extras/php-extras.spec.fc3

ping Matthias :)

ping -f Matthias :|

There is php-5.1.1 in FC5/devel now, which no more have "fam" module and built
now with system sqlite (>= 3.0.0).
- don't build "fam" module by default for FC4 (as it will disappear in FC5)
- drop "sqlite" for FC5/devel, as it use bundled sqlite v2, but newest php now
use system's sqlite v3 (for the pdo-sqlite module), the using of both versions
leads to incompatibilities.
- don't build "sqlite" for FC4 by default (as it will be not present in FC5).

(Thanks to Tom 'spot' Callaway for the testing under rawhide).


New SPECS/SRPMS:

FC3:
http://dmitry.butskoy.name/php-extras/php-extras.spec.fc3
http://dmitry.butskoy.name/php-extras/php-extras-4.3.11-2.src.rpm

FC4:
http://dmitry.butskoy.name/php-extras/php-extras.spec.fc4
http://dmitry.butskoy.name/php-extras/php-extras-5.0.4-2.src.rpm

FC5:
http://dmitry.butskoy.name/php-extras/php-extras.spec.fc5
http://dmitry.butskoy.name/php-extras/php-extras-5.1.1-1.src.rpm

Needs work:
* Source php-5.0.4.tar.gz is different from upstream
* Specfile should be in the format %{name}.spec
  (wiki: PackageReviewGuidelines)
* Build failed in mock with this error "No Spec file found in srpm:
php-extras-5.0.4-2.src.rpm" (because of previous issue)

> Source php-5.0.4.tar.gz is different from upstream
OOOPS! I just take it from FC4's php...

Both initial and updated php-5.0.4 in FC4 have the same tarball, which differs
from the upstream's tarball. (Perhaps FC4 uses some two-days-prerelease...).

> Specfile should be in the format %{name}.spec
Sorry, just a typo.


New SPECS/SRPMS:

FC3:
http://dmitry.butskoy.name/php-extras/php-extras.spec.fc3
http://dmitry.butskoy.name/php-extras/php-extras-4.3.11-2.src.rpm

FC4:
http://dmitry.butskoy.name/php-extras/php-extras.spec.fc4
http://dmitry.butskoy.name/php-extras/php-extras-5.0.4-3.src.rpm

FC5:
http://dmitry.butskoy.name/php-extras/php-extras.spec.fc5
http://dmitry.butskoy.name/php-extras/php-extras-5.1.1-1.src.rpm

* rpmlint complains:
binary-or-shlib-defines-rpath /usr/lib/php/modules/mcrypt.so ['/usr/lib']
binary-or-shlib-defines-rpath /usr/lib/php/modules/mhash.so ['/usr/lib']
binary-or-shlib-defines-rpath /usr/lib/php/modules/readline.so ['/usr/lib']
binary-or-shlib-defines-rpath /usr/lib/php/modules/tidy.so ['/usr/lib']
* The packages should contain the text of the license (wiki:
PackageReviewGuidelines)

Concerning the different sources, I've gunzipped the one shipped by fedora an
upstreams, and both tarballs give the same md5sum, so I guess Fedora's one has
just been re-tarred.

Well,

The main goal of this package is to add some standard PHP-tarball's modules
which are not compiled by default in FC. The added modules should look as if
they were compiled in the main PHP package (i.e. as if the FC php's maintainers
have decided to enable it as yet another main php's subpackage). Therefore these
extras packages should first follow the FC's php behaviour (considering it as a
precedent), and only then the recommended FE rules (if it is not conflicts with
the main php behavior).

> The packages should contain the text of the license
IMHO it is included in the main php. Php's subpackages (php-ldap, php-pgsql
etc.) do not include it.

> binary-or-shlib-defines-rpath /usr/lib/php/modules/.... ['/usr/lib']
Strange... My rpmlint-0.71-1 say nothing about it. And there is no any
"/usr/lib" substrings in the binary files...

What version of rpmlint did you use? 
When you build the main php package the same way as php-extras, are there
similar reports about the main subpackages?

ping?  :)

FC3 has died now, hoping it will simplify the review...

(In reply to comment #16)
> ping?  :)
> 
> FC3 has died now, hoping it will simplify the review...
For FC packages they are now being maintained in legacy. For Extras, it is upto
the package maintainer. 

The "Requires: php = %{version}" in every module package is not good.
It causes upgrade problems between FC's "php" and every module package.
Querying the rpms, all contain just DSOs, no versioned paths, and
nothing obvious which would justify a dependency on a specific minor
version of PHP. In case you meant to use "Requires: php >= %{version}"
instead, then prefer that. Alternatively, drop the version completely,
since you know PHP in the FC repo is big enough.

RPATHs will need a close look to make sure they really never end up in
the binaries. With Rawhide they don't currently, but in the build log
they are still present, e.g.:
  -rpath /home/qa/tmp/rpm/BUILD/php-5.1.1/ext/dbase/modules 

About the Requires, this thread I raised on pecl-dev may be of relevance:
http://marc.theaimsgroup.com/?t=113745441400005

I *think* what needs to happen is for the core PHP package to have a:

Provides: php(API) = XXXXXXXX

(or similar)

and for PECL modules, php-extras etc to have:

Requires: php(API) = XXXXXXXX

This will need jorton@redhat to help by adding the Provide to the Core php package.

That thread confirms that the current dependency is too strict.

I have filed a request for the API Provide as bug #183227

IMO, until Provides: php(API) = XXXXXXXX is adopted by the core php
package/maintainer, I think it not unreasonable to be more safe (to guarantee
modules' use against the same php version it was built) by using the more
restrictive
Requires: php = %{version}
than to be (potentially) sorry and discover breakage when/if php(core) is upgraded.

About "Requires: php = %{version}" :

There is also one big reason for this. These are modules which have THE SAME
source as the Core php package (i.e., common tarball). It would be strange if
the Core php package will be upgraded, but php-extras still use another,
obsoleted source...

Initially, I even wanted to make "Requires: php = %{version}-%{release}", but as
different "php Core" releases do not reflect the "php-extras" source part
(patches change nothing in that area), I've considered that "php = %{version}"
seems to be enough...

> These are modules which have THE SAME source as the Core php package
> (i.e., common tarball). It would be strange if the Core php package
> will be upgraded, but php-extras still use another, obsoleted source...

Unconvincing. Surely you would follow with a minor update in Extras,
too, wouldn't you?

IMO, the version in this dependency introduces breakage at the
depsolver level, whereas no API/ABI breakage is found in the php
module implementation itself.

> Unconvincing. Surely you would follow with a minor update in Extras,
> too, wouldn't you?
Sure! It is not too often, and it is not too difficult...


But you are right -- it is possible that there is a new Core package version,
but Extras yet not, and Extras package's dependencies prevent the updating of
the Core package...

Perhaps:
"Requires: php >= 5.1.1, php < 5.2"
is a good solution (at least while there is no "php(ABI)" feature)?

- apply "libtool-rpath-workaround" to avoid passing -rpath option to the linker.
It is well-known hack, easyly found anywhere in Inet.
Actually, I never see any rpathes in the binaries generated by me, but comment
#14 says that it seems to be needed.

- remove old and add new readline patch for newest php 5.1.2 (avoid building
with libedit instead of libreadline).

- "Requires: php = %{version}" -- not any changes yet, IMO it can be saved "as
is" until some php(ABI) feature appears in the future...


New SPECS/SRPMS:

FC3 (I know that it was EOL'ed :)) :
http://dmitry.butskoy.name/php-extras/php-extras.spec.fc3
http://dmitry.butskoy.name/php-extras/php-extras-4.3.11-3.src.rpm

FC4:
http://dmitry.butskoy.name/php-extras/php-extras.spec.fc4
http://dmitry.butskoy.name/php-extras/php-extras-5.0.4-3.src.rpm

FC5:
http://dmitry.butskoy.name/php-extras/php-extras.spec.fc5
http://dmitry.butskoy.name/php-extras/php-extras-5.1.1-1.src.rpm

Sorry for the typo:

FC5:
http://dmitry.butskoy.name/php-extras/php-extras.spec.fc5
http://dmitry.butskoy.name/php-extras/php-extras-5.1.2-1.src.rpm

Hey, Joe Orton's done the php-api Provide upstream in the Core php package. See
bug #183227. Awesome. Assuming that makes it into FC5 final, hopefully that ends
the discussion about versioning :)

Well,
FC4: php = %{version}
FC5 and later: php-api = <integer>

Is it OK?

- Now FC5 has:
Requires: php >= %{version}, php-api = %{apiver}


New SPECS/SRPMS:

FC3 (I know that it was EOL'ed :)) :
http://dmitry.butskoy.name/php-extras/php-extras.spec.fc3
http://dmitry.butskoy.name/php-extras/php-extras-4.3.11-3.src.rpm

FC4:
http://dmitry.butskoy.name/php-extras/php-extras.spec.fc4
http://dmitry.butskoy.name/php-extras/php-extras-5.0.4-3.src.rpm

FC5:
http://dmitry.butskoy.name/php-extras/php-extras.spec.fc5
http://dmitry.butskoy.name/php-extras/php-extras-5.1.2-2.src.rpm

Maybe it is better to name the source package as "php-modules" instead of
"php-extras" ? (Anyway, binary packages will have its own names: "php-mcrypt",
"php-tidy" etc.) ...

Good:

- rpmlint checks return:

W: php-dbase invalid-license The PHP License
W: php-dbase no-documentation
W: php-readline invalid-license The PHP License
W: php-readline no-documentation
W: php-mcrypt invalid-license The PHP License
W: php-mcrypt no-documentation
W: php-mhash invalid-license The PHP License
W: php-mhash no-documentation
W: php-tidy invalid-license The PHP License
W: php-tidy no-documentation


- package meets naming guidelines
- package meets packaging guidelines
- license (PHP License) OK, matches source
- spec file legible, in am. english
- source matches upstream
- package compiles on FC-5 (x86)
- 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
- no need for .desktop file

Good work Dmitry, sorry it took so long to get this reviewed.
 

Thanks a lot for the review, Tom!

CVS admin for ownership/EPEL branched done in bug 249577:
https://bugzilla.redhat.com/show_bug.cgi?id=249577

Since the latest Fedora 7 update of the main PHP package,
"php" and "php-extras" are merged into common "php".

Hence:
- remove "devel" branch from CVS
- remove "F-7" branch from CVS
- remove php-extras source rpm from the "development" repo
(There is no need to remove any binary rpms, as they just updated by the
same-name rpms produced by the main PHP).

"php-extras" is still needed for FC-6, and for EPEL .

Please follow the package end of life procedure at: 
http://fedoraproject.org/wiki/PackageMaintainers/PackageEndOfLife

Feel free to reset the fedora-cvs flag if you need further cvsadmin attention.

Package Change Request
======================
Package Name: php-extras
New Branches: el6
Owners: buc

People have informed that they want php-mcrypt and others in EPEL6, which are not provided by the main PHP package in RHEL6 . Hence the php-extras is still needed for EPEL.

Git done