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 Review | Assignee: | Tom "spot" Callaway <tcallawa> |
| Status: | CLOSED NEXTRELEASE | QA Contact: | David Lawrence <dkl> |
| Severity: | medium | Docs Contact: | |
| Priority: | medium | ||
| Version: | rawhide | CC: | 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
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 |