Bug 823056
Summary: | Review Request: php-symfony2-Locale - Symfony2 Locale Component | ||||||
---|---|---|---|---|---|---|---|
Product: | [Fedora] Fedora | Reporter: | Shawn Iwinski <shawn> | ||||
Component: | Package Review | Assignee: | Remi Collet <fedora> | ||||
Status: | CLOSED ERRATA | QA Contact: | Fedora Extras Quality Assurance <extras-qa> | ||||
Severity: | medium | Docs Contact: | |||||
Priority: | unspecified | ||||||
Version: | rawhide | CC: | fedora, notting, package-review | ||||
Target Milestone: | --- | Flags: | fedora:
fedora-review+
gwync: fedora-cvs+ |
||||
Target Release: | --- | ||||||
Hardware: | All | ||||||
OS: | Linux | ||||||
Whiteboard: | |||||||
Fixed In Version: | Doc Type: | Bug Fix | |||||
Doc Text: | Story Points: | --- | |||||
Clone Of: | Environment: | ||||||
Last Closed: | 2012-07-10 16:28:26 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: | 814994 | ||||||
Bug Blocks: | 823071 | ||||||
Attachments: |
|
Description
Shawn Iwinski
2012-05-18 21:38:46 UTC
Updates per comments in bug 823043 - Removed BuildRoot - Changed php require to php-common - Added the following requires based on phpci results: php-ctype, php-date, php-pcre - Removed %defattr from %files section SPEC URL: http://people.redhat.com/siwinski/rpmbuild/SPECS/php-symfony2-Locale.spec SRPM URL: http://people.redhat.com/siwinski/rpmbuild/SRPMS/php-symfony2-Locale-2.0.14-2.fc16.src.rpm Update per https://bugzilla.redhat.com/show_bug.cgi?id=823041#c5 - Moved documentation to correct location SPEC URL: http://people.redhat.com/siwinski/rpmbuild/SPECS/php-symfony2-Locale.spec SRPM URL: http://people.redhat.com/siwinski/rpmbuild/SRPMS/php-symfony2-Locale-2.0.14-3.fc16.src.rpm Update per https://bugzilla.redhat.com/show_bug.cgi?id=823071#c5 - Added missing php-intl require SPEC URL: http://people.redhat.com/siwinski/rpmbuild/SPECS/php-symfony2-Locale.spec SRPM URL: http://people.redhat.com/siwinski/rpmbuild/SRPMS/php-symfony2-Locale-2.0.14-4.fc16.src.rpm Updated to upstream version 2.0.15 & updates per bug #817303 - Removed "BuildRequires: php-pear >= 1:1.4.9-1.2" - Updated %prep section - Removed cleaning buildroot from %install section - Removed documentation move from %install section (fixed upstream) - Removed %clean section - Updated %doc in %files section SPEC URL: http://people.redhat.com/siwinski/rpmbuild/SPECS/php-symfony2-Locale.spec SRPM URL: http://people.redhat.com/siwinski/rpmbuild/SRPMS/php-symfony2-Locale-2.0.15-1.fc16.src.rpm This package seems broken. Until version 2.0.14 all ".res" file were installed as "php" Since version 2.0.15 they are installed as "doc" In code (Symfony/Component/Locale/Locale.php) the have $bundle = \ResourceBundle::create($locale, __DIR__.'/Resources/data/region'); So, file will not be found. This, of course, need to be reported upstream and, at least fixed in package.xml for the review All this "locale" %files should be tag with "%lang(xx)" ---- Out of the scope of this review: I personally think this files should be installed as "data" but this will requires some changes in the code () For example (taken from Date_Holidays) $data_dir = "@DATA-DIR@"; if ($data_dir == '@'.'DATA-DIR'.'@') { // run from source dir $data_dir = dirname(__FILE__)."xxxxxxx"; } else { // run from install dir $data_dir .= "xxxxxx"; } And, in package.xml, when needed <tasks:replace from="@DATA-DIR@" to="data_dir" type="pear-config" /> Notice : You could have detect this regression when updating your package to 2.0.15 A good practice when you update a package : run "rpmdiff" between the old and the new version and analyze the changes For PEAR package, I'm also used to run a diff between the old and the new package.xml (dependency changes, new/deleted file, license change, ...) (In reply to comment #5) > This package seems broken. > > Until version 2.0.14 all ".res" file were installed as "php" > Since version 2.0.15 they are installed as "doc" > > In code (Symfony/Component/Locale/Locale.php) the have > $bundle = \ResourceBundle::create($locale, __DIR__.'/Resources/data/region'); > > So, file will not be found. > > This, of course, need to be reported upstream and, at least fixed in > package.xml for the review Since these fixes upstream will require a new release (probably 2.0.16), may I just fix the package.xml in the spec file? > All this "locale" %files should be tag with "%lang(xx)" I will work on this. > ---- > Out of the scope of this review: > > I personally think this files should be installed as "data" but this will > requires some changes in the code () > > For example (taken from Date_Holidays) > $data_dir = "@DATA-DIR@"; > if ($data_dir == '@'.'DATA-DIR'.'@') { > // run from source dir > $data_dir = dirname(__FILE__)."xxxxxxx"; > } else { > // run from install dir > $data_dir .= "xxxxxx"; > } > > And, in package.xml, when needed > <tasks:replace from="@DATA-DIR@" to="data_dir" type="pear-config" /> I will work with upstream on this. Several packages could use this approach. I'm pretty sure this approach would break their unit tests though, but I will work with them. Speaking of unit tests, upstream does not include their unit tests in their PEAR packages (only in their SCM repo). Is that common? (In reply to comment #7) > Since these fixes upstream will require a new release (probably 2.0.16), may > I just fix the package.xml in the spec file? Yes, it will allow us to proceed to the review > I will work with upstream on this. Several packages could use this approach. Great > I'm pretty sure this approach would break their unit tests though, Should not. The code sample pasted from Date_Holidays allow to run from source or from installation and is used by lot of pear extensions (pear project run an jenkins server for QA) > Speaking of unit tests, upstream does not include their unit tests in their > PEAR packages (only in their SCM repo). Is that common? Yes. I think you can post a upstream RFE to add test suite in the tarball (role="test") Then, it will become a "must" to run this test suite during %check. I have add such %check in lot of packages, you can look at for example (and usefull tips). Of course I can help you on this feature. This big change will introduce some complexity (more BR) but also more QA. Updates per comment #5: - Fix package.xml for *.res files issue - Added %lang directive flags for *.res files - Modified %files because of separate *.res file listings SPEC URL: http://siwinski.fedorapeople.org/rpmbuild/SPECS/php-symfony2-Locale.spec SRPM URL: http://siwinski.fedorapeople.org/rpmbuild/SRPMS/php-symfony2-Locale-2.0.15-3.fc17.src.rpm Created attachment 593970 [details]
php-symfony2-Locale-review.txt
Generated by fedora-review 0.1.3
No issue detected. === APPROVED === New Package SCM Request ======================= Package Name: php-symfony2-Locale Short Description: Symfony2 Locale Component Owners: siwinski Branches: f16 f17 el6 InitialCC: Git done (by process-git-requests). php-symfony2-Locale-2.0.15-3.fc17 has been submitted as an update for Fedora 17. https://admin.fedoraproject.org/updates/php-symfony2-Locale-2.0.15-3.fc17 php-symfony2-Locale-2.0.15-3.fc16 has been submitted as an update for Fedora 16. https://admin.fedoraproject.org/updates/php-symfony2-Locale-2.0.15-3.fc16 php-symfony2-Locale-2.0.15-3.el6 has been submitted as an update for Fedora EPEL 6. https://admin.fedoraproject.org/updates/php-symfony2-Locale-2.0.15-3.el6 php-symfony2-Locale-2.0.15-3.el6 has been pushed to the Fedora EPEL 6 testing repository. php-symfony2-Locale-2.0.15-3.fc17 has been pushed to the Fedora 17 stable repository. php-symfony2-Locale-2.0.15-3.fc16 has been pushed to the Fedora 16 stable repository. php-symfony2-Locale-2.0.15-3.el6 has been pushed to the Fedora EPEL 6 stable repository. |