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-1.fc16.src.rpm Description: Locale component provides fallback code to handle cases when the intl extension is missing. Additionally it extends the implementation of a native Locale (http://php.net/manual/en/class.locale.php) class with several handy methods. Replacement for the following functions and classes is provided: * intl_is_failure * intl_get_error_code * intl_get_error_message * Collator * IntlDateFormatter * Locale * NumberFormatter Stub implementation only supports the en locale.
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.