Bug 823056 - Review Request: php-symfony2-Locale - Symfony2 Locale Component
Review Request: php-symfony2-Locale - Symfony2 Locale Component
Status: CLOSED ERRATA
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
unspecified Severity medium
: ---
: ---
Assigned To: Remi Collet
Fedora Extras Quality Assurance
:
Depends On: php-channel-symfony2
Blocks: 823071
  Show dependency treegraph
 
Reported: 2012-05-18 17:38 EDT by Shawn Iwinski
Modified: 2012-07-14 14:33 EDT (History)
3 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2012-07-10 12:28:26 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
fedora: fedora‑review+
limburgher: fedora‑cvs+


Attachments (Terms of Use)
php-symfony2-Locale-review.txt (5.87 KB, text/plain)
2012-06-24 02:22 EDT, Remi Collet
no flags Details

  None (edit)
Description Shawn Iwinski 2012-05-18 17:38:46 EDT
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.
Comment 1 Shawn Iwinski 2012-05-20 14:00:45 EDT
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
Comment 4 Shawn Iwinski 2012-05-31 13:29:22 EDT
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
Comment 5 Remi Collet 2012-06-10 05:36:54 EDT
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" />
Comment 6 Remi Collet 2012-06-10 05:48:00 EDT
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, ...)
Comment 7 Shawn Iwinski 2012-06-10 18:56:57 EDT
(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?
Comment 8 Remi Collet 2012-06-11 01:03:55 EDT
(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.
Comment 9 Shawn Iwinski 2012-06-23 11:15:31 EDT
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
Comment 10 Remi Collet 2012-06-24 02:22:46 EDT
Created attachment 593970 [details]
php-symfony2-Locale-review.txt

Generated by fedora-review 0.1.3
Comment 11 Remi Collet 2012-06-24 02:23:34 EDT
No issue detected.

=== APPROVED ===
Comment 12 Shawn Iwinski 2012-06-24 23:11:08 EDT
New Package SCM Request
=======================
Package Name: php-symfony2-Locale
Short Description: Symfony2 Locale Component
Owners: siwinski
Branches: f16 f17 el6
InitialCC:
Comment 13 Gwyn Ciesla 2012-06-26 10:38:33 EDT
Git done (by process-git-requests).
Comment 14 Fedora Update System 2012-06-28 01:37:15 EDT
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
Comment 15 Fedora Update System 2012-06-28 01:37:27 EDT
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
Comment 16 Fedora Update System 2012-06-28 01:37:37 EDT
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
Comment 17 Fedora Update System 2012-06-28 12:09:05 EDT
php-symfony2-Locale-2.0.15-3.el6 has been pushed to the Fedora EPEL 6 testing repository.
Comment 18 Fedora Update System 2012-07-10 12:28:26 EDT
php-symfony2-Locale-2.0.15-3.fc17 has been pushed to the Fedora 17 stable repository.
Comment 19 Fedora Update System 2012-07-10 12:30:31 EDT
php-symfony2-Locale-2.0.15-3.fc16 has been pushed to the Fedora 16 stable repository.
Comment 20 Fedora Update System 2012-07-14 14:33:04 EDT
php-symfony2-Locale-2.0.15-3.el6 has been pushed to the Fedora EPEL 6 stable repository.

Note You need to log in before you can comment on or make changes to this bug.