Bug 999313 - Review Request: php-pecl-zip - A ZIP archive management extension
Review Request: php-pecl-zip - A ZIP archive management extension
Status: CLOSED CURRENTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Gwyn Ciesla
Fedora Extras Quality Assurance
https://fedoraproject.org/wiki/Common...
: CommonBugs
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2013-08-21 02:55 EDT by Remi Collet
Modified: 2013-11-12 13:48 EST (History)
4 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2013-09-06 10:39:45 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)

  None (edit)
Description Remi Collet 2013-08-21 02:55:30 EDT
Spec URL: https://raw.github.com/remicollet/remirepo/daf1eb5c9b21fcc75d66de95c4008deff66464c8/php/pecl/php-pecl-zip/php-pecl-zip.spec
SRPM URL: http://rpms.famillecollet.com/SRPMS/php-pecl-zip-1.12.1-2.remi.src.rpm
Description: 
Zip is an extension to create and read zip files.

Fedora Account System Username: remi

Target : fedora >= 20 which have libzip 0.11.1 and zip extension dropped from main php package.
Comment 2 Gwyn Ciesla 2013-08-22 09:04:15 EDT
Good:

- rpmlint checks return:

Lots of spurious executable perms and non-utf8 errors.

php-pecl-zip.x86_64: W: unable-to-read-zip /usr/share/doc/php-pecl-zip-1.12.1/examples/test.zip: Bad magic number for central directory

This one is expected.

php-pecl-zip.x86_64: I: enchant-dictionary-not-found fr
A dictionary for the Enchant spell checking library is not available for the
language given in the info message.  Spell checking will proceed with
rpmlint's built-in implementation for localized tags in this language. For
better spell checking results in this language, install the appropriate
dictionary that Enchant will use for this language, often for example
hunspell-* or aspell-*.


- package meets naming guidelines
- package meets packaging guidelines
! license ( PHP ) OK, text in %doc, matches source I also see BSD.
- spec file legible, in am. english
- source matches upstream
- package compiles on devel (x86_64)
! no missing BR     mock build on rawhide fails, missing zlib.h.
- 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
- nothing in %doc affects runtime
- no need for .desktop file 

So it's just the logic around zlib-devel, executable perms, utf8, and verify the license tag.
Comment 3 Remi Collet 2013-08-22 10:13:55 EDT
Thanks for taking the review.

- Your right zlib.h is included from lib/zipint.h which is used even using system libzip (uggly thing, I now).

- License is really PHP (for the extension). I clarify the License tag when bundled libzip is used (even if not in our target f>=20). I will probably clear the backport stuff later.

Changes: https://github.com/remicollet/remirepo/commit/0bf66f7fc83f0c56082836008980ba5ea825a334

Spec: https://raw.github.com/remicollet/remirepo/0bf66f7fc83f0c56082836008980ba5ea825a334/php/pecl/php-pecl-zip/php-pecl-zip.spec

Srpm: http://rpms.famillecollet.com/SRPMS/php-pecl-zip-1.12.1-3.remi.src.rpm

F20 koji scratch build: http://koji.fedoraproject.org/koji/taskinfo?taskID=5841526
Comment 4 Remi Collet 2013-08-22 10:15:01 EDT
P.S. : I don't think file-not-utf8 make sense for zip files...
Comment 6 Gwyn Ciesla 2013-08-22 10:41:34 EDT
Looks better, but I'm still getting spurious executable perms.  You may want to fix them with find/xargs, so you get them all.
Comment 8 Gwyn Ciesla 2013-08-22 11:19:09 EDT
Now it works on f19 but fails in rawhide.

chmod: cannot access 'lib/*.c': No such file or directory

This is why I suggested find. :)
Comment 10 Gwyn Ciesla 2013-08-22 11:45:02 EDT
Much better.  Yes, there's a lot of that going around. :)

APPROVED.

So I just replace php-zip with php-pecl-zip in the wordpress Requires and I should be good?
Comment 11 Remi Collet 2013-08-22 11:48:44 EDT
Thanks for the quick review (and for your patience...)


New Package SCM Request
=======================
Package Name: php-pecl-zip
Short Description: A ZIP archive management extension
Owners: remi
Branches: f20
InitialCC:
Comment 12 Gwyn Ciesla 2013-08-22 11:52:29 EDT
Unretired, please take ownership in pkgdb and submit Package Change Request
for f20 branch.
Comment 13 Remi Collet 2013-08-23 01:47:17 EDT
Package Change Request
======================
Package Name: php-pecl-zip
New Branches: f20
Owners: remi
InitialCC:
Comment 14 Gwyn Ciesla 2013-08-23 08:40:33 EDT
Git done (by process-git-requests).
Comment 15 Gwyn Ciesla 2013-09-06 10:39:45 EDT
This is built.
Comment 16 Peter Bowey 2013-10-18 08:29:35 EDT
Hi Remi Collet,

I have not understood the 'logic flow' to replace PHP's previous 'zip extension', other than your comment "As PHP only work with libzip 0.10 (use lot of private stuff), the zip extension is become unmaintainable, so have been dropped from
php."

As a Fedora 20 (devel) user, coder and programmer, I arrive at a 'issue' to interpret how the new Fedora PHP 5.5.4/5 RPM sources have placed a drop on including the original ZIP support and arrived at a kludge [patchy] PECL include?

Now I find references to a 'new' PECL build of "php-pecl-zip" at http://koji.fedoraproject.org/koji/packageinfo?packageID=3452

I can see the logic of how Fedora 20 uses 'libzip 0.11' and not libzip 0.10', however, as fully successful 'build' based on http://koji.fedoraproject.org/koji/buildinfo?buildID=471936 [php-5.5.5-1.fc20.src.rpm] does simply remove the PHP tree use of the original supplied ZIP support.

Using this 'successful' PHP build, I simply see that PHP zip support is now disabled! That is not good logic, without 'knowledge' or shared information on builds!

Now I read 'hard to interpret' result of a new PECL PHP [src] derived from the code at https://github.com/pierrejoye/php_zip.

Please encourage us 'developers' to see the full picture in this PHP tree for Fedora 20+.
Comment 17 Peter Bowey 2013-10-18 08:39:59 EDT
Using the latest Drupal 7 CMS as the the PHP 'report agent' here:

Warning:

Zip archive support	Missing

PHP does not have the zip archive extension available. Webform module requires zip support for exporting submissions to Microsoft Excel.
Comment 18 Remi Collet 2013-10-18 08:43:54 EDT
Sorry I don't see any problem (or can't understand what is your problem).

If a package need this extension it should, by Guidelines requires php-zip which is provided by php-common in older version and by php-pecl-zip in new version.

As a Fedora 20 (devel) user, coder and programmer, you should know which extensions you need and install them.
Comment 19 Peter Bowey 2013-10-18 08:55:20 EDT
Remi thanks,

OK I will do that extra work to enable the 'new' ZIP change, but it is 'unusual' to need to resort to this practice in Fedora builds.

If you need to drop PHP extensions out of the normal 'tree' build, then you might explain this in your much appreciated and regular builds at http://koji.fedoraproject.org/koji/

'My issue' is the change to a predictable build on PHP, with or without any 'previous unknown' PECL packages being a supplement [an option].
Comment 20 Remi Collet 2013-10-18 09:06:14 EDT
Again, you need to know the full list of extensions you need to install.

As for gettext, session, mbstring, mysql, intl, ... zip is not different.

You should "never" rely on the packaging layout which have already change in the past, and will very probably change again in the future.

If you need, p.e: zip + session + mbstring, then you have to 

   yum install php-zip php-session php-mbstring

And this, whatever the OS (RHEL, Fedora), OS version or PHP version are.
Comment 21 Peter Bowey 2013-10-18 09:13:12 EDT
The simple 'PHP tree' logic 'solution' is replace the old "php-5.5.5/ext/zip' code with the git at https://github.com/pierrejoye/php_zip.

In your 'words' this is a mandate for the Fedora 20 use of 'libzip 0.11'.

Reading thought the latest Fedora 20 php.spec you kindly supplied illustrates this flow:

%if 0%{?fedora} == 17 || 0%{?fedora} == 18 || 0%{?fedora} == 19 || 0%{?rhel} == 7
%global with_zip     1
%global with_libzip  1
%else
%global with_zip     0
%global with_libzip  0
%endif

%if %{with_libzip}
BuildRequires: libzip-devel >= 0.10
BuildRequires: libzip-devel <  0.11
%endif

There is no reference to 'php-pecl-zip' other than:

%if %{with_zip}
Provides: php-zip, php-zip%{?_isa}
Obsoletes: php-pecl-zip < 1.11
%endif 

and here:

* Mon Nov 27 2006 Joe Orton <jorton@redhat.com> 5.2.0-5
- build json and zip shared, in -common (Remi Collet, #215966)
- obsolete php-json and php-pecl-zip
Comment 22 Remi Collet 2013-10-18 09:20:40 EDT
(In reply to Peter Bowey from comment #21)
> The simple 'PHP tree' logic 'solution' is replace the old
> "php-5.5.5/ext/zip' code with the git at
> https://github.com/pierrejoye/php_zip.

Definitively not acceptable.
Comment 23 Peter Bowey 2013-10-18 09:33:27 EDT
Sorry Remi,(In reply to Remi Collet from comment #20)

I build (compile) PHP based on the PHP's existing tree, or simply add valid extensions to the PHP tree, then compile!

This works well for the 'new' remi based 'json', 'apcu' (with mods),and now PHP zip.

It must be said that I am very appreciative of all the great support you do for Fedora / Redhat PHP devel!!

> Again, you need to know the full list of extensions you need to install.

I do know them well!

> 
> As for gettext, session, mbstring, mysql, intl, ... zip is not different.
> 
> You should "never" rely on the packaging layout which have already change in
> the past, and will very probably change again in the future.

Fedora was once a predictable creature until 'systemd' and 'php' bloomed into the new arts (@Lennart Poettering)

> 
> If you need, p.e: zip + session + mbstring, then you have to 
> 
>    yum install php-zip php-session php-mbstring
> 
> And this, whatever the OS (RHEL, Fedora), OS version or PHP version are.

Great reminders of the long journey for us *all*!
Comment 24 Peter Bowey 2013-10-18 09:39:15 EDT
Thanks Remi,

So what is 'acceptable', to avoid the future 'confusion' and 'loss' of developerds good code time.

Are we all reduced to trials with changes, or can this be 'documented' for all interested?

I am happy enough to encourage this strategic and work towards same great 'result'
Comment 25 Remi Collet 2013-10-18 10:25:06 EDT
Note added in https://fedoraproject.org/wiki/Common_F20_bugs#php-zip
Comment 26 Peter Bowey 2013-10-18 10:36:58 EDT
Many thanks Remi!
Comment 27 Mike Ruckman 2013-11-12 13:48:29 EST
Added CommonBugs link to the Whiteboard.

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