Bug 1413434 - Review Request: php-gettext-languages - Generate gettext language lists with plural rules
Summary: Review Request: php-gettext-languages - Generate gettext language lists with ...
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Remi Collet
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks: 1414216
TreeView+ depends on / blocked
 
Reported: 2017-01-16 00:55 UTC by Randy Barlow
Modified: 2017-02-22 17:51 UTC (History)
3 users (show)

Fixed In Version: php-gettext-languages-2.1.3-3.fc26
Doc Type: If docs needed, set a value
Doc Text:
Clone Of:
Environment:
Last Closed: 2017-02-22 17:23:39 UTC
Type: ---
fedora: fedora-review+


Attachments (Terms of Use)
phpci.log (8.60 KB, text/plain)
2017-02-03 10:49 UTC, Remi Collet
no flags Details
review.txt (6.81 KB, text/plain)
2017-02-03 10:50 UTC, Remi Collet
no flags Details

Description Randy Barlow 2017-01-16 00:55:56 UTC
Spec URL: https://bowlofeggs.fedorapeople.org/php-gettext-languages.spec
SRPM URL: https://bowlofeggs.fedorapeople.org/php-gettext-languages-2.1.3-1.fc26.src.rpm
Description: A library that can generate gettext language lists automatically
generated from CLDR data.
Fedora Account System Username: bowlofeggs

Comment 1 Remi Collet 2017-01-18 17:14:53 UTC
Quick notes:

- you've been hit by the ".gitattributes" issue. Upstream remove test suite from the tarball, so you have to use a git snapshot (see various packages, such as the php-zendframework-zend-* ones for example). Running test suite is a must when possible (and allow Koschei monitoring)

- better to add an autoloader (using fedora/autoloader) as composer.json state this library is PSR64 compliant. Will help you:

   - to run the test suite
   - to use this lib. from other package

Comment 2 Randy Barlow 2017-01-22 03:11:56 UTC
Hello Remi!

The upstream project already has an autoloader.php for the installed package, and they had a tests/bootstrap.php that was close to what we needed but not quite the same. I ended up using two sed statements to modify the tests/bootstrap.php and bin/export.php so that they require_once with the installed path. What do you think about that approach?

Also, I realized that it seems that they may expect that bin/export.{php,sh} to get distributed with the package and I have not done that so far because the name "export.php" sounds too general to go into /usr/bin. Do you think we should distribute that executable? If so, should I name it something more specific (gettext-languages-export.sh, cldr-to-gettext-plural-rules-export.sh?) The tests rely on this executable running first to generate some data file (see tests/bootstrap.php), which is why I used that sed statement, but if we want to distribute it I'll need to sed a copy of it instead (and sed the one we install separately).

Here's what I have so far, without distributing that executable:

Spec URL: https://bowlofeggs.fedorapeople.org/php-gettext-languages.spec
SRPM URL: https://bowlofeggs.fedorapeople.org/php-gettext-languages-2.1.3-2.fc26.src.rpm

Comment 3 Remi Collet 2017-01-22 06:35:44 UTC
> The upstream project already has an autoloader.php

Indeed, I miss it.

> Also, I realized that it seems that they may expect that bin/export.{php,sh} to get distributed with the package 

Indeed (reading the README)

Notice: this means that the comopser.json is incomplete, should have this listed in the "bin" section (ex, see https://github.com/sebastianbergmann/phpunit/blob/5.7/composer.json#L61).

=> could be submitted upstream

The .sh is uneeded, (the .php with a propoer shebang is enough)

=> using "#!/usr/bin/env php" can be submitted upstream.

(Fedora Guidelines, recommends #!/usr/bin/php, but using env is also ok, if you require php-cli, at least this allow the command to work with SCL... even if this is more correct outside Fedora).

> Do you think we should distribute that executable? 

yes

> If so, should I name it something more specific (gettext-languages-export.sh, cldr-to-gettext-plural-rules-export.sh?) 

Yes. "%{name}-export" seems ok (no suffix)

> but if we want to distribute it I'll need to sed a copy of it instead (and sed the one we install separately).

Indeed. 
Patch it in %prep for installation
Patch it again in %check.

From php-nikic-php-parser

   sed -e 's:%{php_home}:%{buildroot}%{php_home}:' \
      bin/php-parse > bin/php-parse-test

Comment 4 Randy Barlow 2017-02-01 21:35:25 UTC
Hello Remi! It was wonderful to meet you at DevConf.cz!

(In reply to Remi Collet from comment #3)
> Notice: this means that the comopser.json is incomplete, should have this
> listed in the "bin" section (ex, see
> https://github.com/sebastianbergmann/phpunit/blob/5.7/composer.json#L61).
> 
> => could be submitted upstream

https://github.com/mlocati/cldr-to-gettext-plural-rules/pull/12

> => using "#!/usr/bin/env php" can be submitted upstream.

https://github.com/mlocati/cldr-to-gettext-plural-rules/pull/13

Upstream had a question about whether the shebang will be printed in the output - if you know the answer it could be helpful to chime in.

> > Do you think we should distribute that executable? 
> 
> yes

Done!


The new code is here:

Spec URL: https://bowlofeggs.fedorapeople.org/php-gettext-languages.spec
SRPM URL: https://bowlofeggs.fedorapeople.org/php-gettext-languages-2.1.3-3.fc26.src.rpm

Comment 5 Randy Barlow 2017-02-01 21:38:23 UTC
Oh, one more thing: fedora-review is upset that php-gettext-gettext and this package both own /usr/share/php/Gettext. I don't know a way around that without creating a php-gettext-common package that just owns that directory, or by making php-gettext-languages own it (which just seems weird, but maybe that's OK?) This seems like a problem that might be common with PHP packages - is there a common solution, or is it something that is just ignored?

Comment 6 Remi Collet 2017-02-03 10:20:11 UTC
As php-gettext-gettext requires php-gettext-languages, on the latest have to own this directory.

Comment 7 Remi Collet 2017-02-03 10:49:58 UTC
Created attachment 1247398 [details]
phpci.log

     Note: phpCompatInfo version 5.0.4 DB version 1.17.0 built Jan 24 2017
     09:56:36 CET static analyze results in

Comment 8 Remi Collet 2017-02-03 10:50:28 UTC
Created attachment 1247399 [details]
review.txt

Generated by fedora-review 0.6.1 (f03e4e7) last change: 2016-05-02
Command line :/usr/bin/fedora-review -b 1413434
Buildroot used: fedora-rawhide-x86_64
Active plugins: Generic, PHP, Shell-api

Comment 9 Remi Collet 2017-02-03 10:51:30 UTC
1 minor easy fix:

[!]: Rpmlint is run on all installed packages.
        php-gettext-languages.src:44: W: rpm-buildroot-usage %prep sed -i "s:require_once.*:require_once '%{buildroot}/%{_datadir}/php/Gettext/Languages/autoloa
der.php';:" tests/bootstrap.php
        easy fix => move this in %check


No blocker

[x]: Package complies to the Packaging Guidelines

=== APPROVED ===

Comment 10 Gwyn Ciesla 2017-02-03 15:04:41 UTC
Package request has been approved: https://admin.fedoraproject.org/pkgdb/package/rpms/php-gettext-languages

Comment 11 Fedora Update System 2017-02-13 04:16:35 UTC
php-gettext-languages-2.1.3-3.fc24 has been submitted as an update to Fedora 24. https://bodhi.fedoraproject.org/updates/FEDORA-2017-aca77236c7

Comment 12 Fedora Update System 2017-02-13 04:16:42 UTC
php-gettext-languages-2.1.3-3.fc25 has been submitted as an update to Fedora 25. https://bodhi.fedoraproject.org/updates/FEDORA-2017-46016c3a74

Comment 13 Fedora Update System 2017-02-13 23:49:55 UTC
php-gettext-languages-2.1.3-4.fc24 has been pushed to the Fedora 24 testing repository. If problems still persist, please make note of it in this bug report.
See https://fedoraproject.org/wiki/QA:Updates_Testing for
instructions on how to install test updates.
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2017-aca77236c7

Comment 14 Fedora Update System 2017-02-14 00:53:53 UTC
php-gettext-languages-2.1.3-4.fc25 has been pushed to the Fedora 25 testing repository. If problems still persist, please make note of it in this bug report.
See https://fedoraproject.org/wiki/QA:Updates_Testing for
instructions on how to install test updates.
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2017-46016c3a74

Comment 15 Fedora Update System 2017-02-22 17:23:39 UTC
php-gettext-languages-2.1.3-4.fc25 has been pushed to the Fedora 25 stable repository. If problems still persist, please make note of it in this bug report.

Comment 16 Fedora Update System 2017-02-22 17:51:15 UTC
php-gettext-languages-2.1.3-4.fc24 has been pushed to the Fedora 24 stable repository. If problems still persist, please make note of it in this bug report.


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