Note: This is a public test instance of Red Hat Bugzilla. The data contained within is a snapshot of the live data so any changes you make will not be reflected in the production Bugzilla. Email is disabled so feel free to test any aspect of the site that you want. File any problems you find or give feedback at bugzilla.redhat.com.

Bug 170131

Summary: Review Request: php-extras - Additional PHP modules from the standard PHP distribution
Product: [Fedora] Fedora Reporter: Dmitry Butskoy <dmitry>
Component: Package ReviewAssignee: Tom "spot" Callaway <tcallawa>
Status: CLOSED NEXTRELEASE QA Contact: David Lawrence <dkl>
Severity: medium Docs Contact:
Priority: medium    
Version: rawhideCC: developer, fedora-extras-list, gauret, herrold, matthias, rpm
Target Milestone: ---   
Target Release: ---   
Hardware: All   
OS: Linux   
URL: http://www.php.net
Whiteboard:
Fixed In Version: Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2006-03-27 10:31:46 UTC Type: ---
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: --- Target Upstream Version:
Embargoed:
Bug Depends On:    
Bug Blocks: 163779    

Description Dmitry Butskoy 2005-10-07 16:56:14 UTC
Spec Url: http://dmitry.butskoy.name/php-extras/php-extras.spec
SRPM Url: http://dmitry.butskoy.name/php-extras/php-extras-4.3.11-0.src.rpm

Description:
PHP is an HTML-embedded scripting language.
This package contains various additional modules for PHP, which
have not been included in the basic PHP package for Fedora Core.


Additional info:

There was already a mail list discussion prior to this package appearing here: https://www.redhat.com/archives/fedora-extras-list/2005-October/msg00212.html

Currently I've made this for FC3 (php4) only. Therefore release is "0" :)

Note, that only appropriate modules' subdirs are used from the main php tarbal (no any executables or apache module are built). Therefore BR: php-devel, etc.

A list of modules to include is not ended. And it should be different for FC3+php4 and FC4+php5 .

I'm not sure about "dbase" module licensing.

etc...etc...etc... :)

Comment 1 Dmitry Butskoy 2005-10-10 15:24:17 UTC
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.


Comment 2 Dmitry Butskoy 2005-10-14 13:35:48 UTC
*** Bug 169544 has been marked as a duplicate of this bug. ***

Comment 3 Matthias Saou 2005-10-14 15:38:36 UTC
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.

Comment 4 Dmitry Butskoy 2005-10-17 17:31:37 UTC
> - 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!

Comment 5 Dmitry Butskoy 2005-10-27 13:34:54 UTC
ping -b somebody! :)

Comment 6 Dmitry Butskoy 2005-11-14 14:27:32 UTC
ping -f Matthias || ping -b Somebody  :)

Comment 7 Matthias Saou 2005-11-14 14:45:00 UTC
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 ;-)

Comment 8 Dmitry Butskoy 2005-11-14 15:32:38 UTC
> 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


Comment 9 Dmitry Butskoy 2005-11-22 17:54:00 UTC
ping Matthias :)

Comment 10 Dmitry Butskoy 2005-12-05 14:51:06 UTC
ping -f Matthias :|

Comment 11 Dmitry Butskoy 2005-12-17 19:18:17 UTC
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




Comment 12 Aurelien Bompard 2005-12-22 15:16:46 UTC
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)

Comment 13 Dmitry Butskoy 2005-12-22 15:55:10 UTC
> 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

Comment 14 Aurelien Bompard 2005-12-23 09:51:43 UTC
* 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.

Comment 15 Dmitry Butskoy 2005-12-23 12:37:11 UTC
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?


Comment 16 Dmitry Butskoy 2006-01-23 12:10:19 UTC
ping?  :)

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

Comment 17 Rahul Sundaram 2006-01-23 15:25:37 UTC
(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. 


Comment 18 Michael Schwendt 2006-02-27 13:41:35 UTC
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 

Comment 19 Tim Jackson 2006-02-27 15:45:32 UTC
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.

Comment 20 Michael Schwendt 2006-02-27 16:18:00 UTC
That thread confirms that the current dependency is too strict.


Comment 21 Tim Jackson 2006-02-27 16:50:58 UTC
I have filed a request for the API Provide as bug #183227

Comment 22 Rex Dieter 2006-02-27 17:14:07 UTC
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.

Comment 23 Dmitry Butskoy 2006-02-27 17:28:24 UTC
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...

Comment 24 Michael Schwendt 2006-02-27 19:12:07 UTC
> 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.


Comment 25 Dmitry Butskoy 2006-02-28 12:46:03 UTC
> 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)?

Comment 26 Dmitry Butskoy 2006-02-28 16:15:28 UTC
- 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

Comment 28 Tim Jackson 2006-02-28 17:19:13 UTC
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 :)

Comment 29 Dmitry Butskoy 2006-02-28 17:43:19 UTC
Well,
FC4: php = %{version}
FC5 and later: php-api = <integer>

Is it OK?

Comment 31 Dmitry Butskoy 2006-03-14 15:29:06 UTC
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.) ...

Comment 32 Tom "spot" Callaway 2006-03-24 20:20:23 UTC
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.
 

Comment 33 Dmitry Butskoy 2006-03-27 10:31:46 UTC
Thanks a lot for the review, Tom!

Comment 34 Gwyn Ciesla 2007-08-31 20:53:02 UTC
CVS admin for ownership/EPEL branched done in bug 249577:
https://bugzilla.redhat.com/show_bug.cgi?id=249577

Comment 35 Dmitry Butskoy 2007-09-19 13:06:59 UTC
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 .




Comment 36 Kevin Fenzi 2007-09-19 17:10:44 UTC
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.

Comment 37 Dmitry Butskoy 2010-12-20 14:29:49 UTC
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.

Comment 38 Kevin Fenzi 2010-12-21 05:57:52 UTC
Git done