Bug 201502 (phpDocumentor) - Review Request: php-pear-PhpDocumentor
Summary: Review Request: php-pear-PhpDocumentor
Keywords:
Status: CLOSED NEXTRELEASE
Alias: phpDocumentor
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Chris Weyl
QA Contact: Fedora Package Reviews List
URL:
Whiteboard:
Depends On: 196802
Blocks: FE-ACCEPT
TreeView+ depends on / blocked
 
Reported: 2006-08-06 15:29 UTC by Konstantin Ryabitsev
Modified: 2007-11-30 22:11 UTC (History)
3 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2007-06-11 15:29:21 UTC
Type: ---
Embargoed:
cweyl: fedora-review+
kevin: fedora-cvs+


Attachments (Terms of Use)

Description Konstantin Ryabitsev 2006-08-06 15:29:54 UTC
Spec URL: http://blues.mcgill.ca/~icon/fe/php-pear-PhpDocumentor.spec
SRPM URL: http://blues.mcgill.ca/~icon/fe/php-pear-PhpDocumentor-1.3.0-0.2.RC6.src.rpm
Description:
phpDocumentor is the current standard auto-documentation tool for the 
php language. phpDocumentor has support for linking between documentation, 
incorporating user level documents like tutorials and creation of 
highlighted source code with cross referencing to php general 
documentation.

phpDocumentor uses an extensive templating system to change your source 
code comments into human readable, and hence useful, formats. This system 
allows the creation of easy to read documentation in 15 different 
pre-designed HTML versions, PDF format, Windows Helpfile CHM format, and 
in Docbook XML. 

Note:
1. The reason why phpdoc is a separate package is because FC6 separates php-cli (required for command-line execution) into a separate package. PhpDocumentor doesn't require the command-line utility -- it can be run from the web if configured to do so.
2. PHP Pear packaging guidelines are still only a draft, so I have used Remi Collet's existing php-pear modules currently already in Extras.

Comment 1 Chris Weyl 2006-08-24 06:09:46 UTC
The release tag is off by a little bit, the %{?dist} tag should always be at
the end of the tag unless it's a sub-release bump.

    0.2%{?dist}.%{rcrel} -> 0.2.%{rcrel}%{?dist}

According to Packaging/PHP, the package should also "Requires: php".

The latest version is 1.3.0 GA, but RC6 is being packaged.  To be fair, GA was
released the day after I assigned the review to myself, but... :)  Please
update the package to 1.3.0.

For the rpmlint warnings, the second is spurious and can be ignored.  The
first, however, could be fixed, but it appears part of the pear install
process is to check the md5sums of each file.  If the line ending can be fixed
w/o too much pain and mucking around with the pear-based install process, I'd
like to see it addressed.

You have php.ini both listed as source1, and created in %prep.  One or the
other works nicely, but both is redundant.  (Especially given nothing is ever
done with %{SOURCE0}...)

Also, while using php.ini to override the default memory and execution limits
works during the rpm build, the install fails on %post with the defaults on
the system:

 [root@zeus mock]# rpm -ivh php-pear-PhpDocumentor-1.3.0-0.2.fc5.RC6.noarch.rpm
 Preparing...                ########################################### [100%]
   1:php-pear-PhpDocumentor ########################################### [100%]
   PHP Fatal error:  Allowed memory size of 8388608 bytes exhausted (tried to
   allocate 229 bytes) in /usr/share/pear/PEAR/Installer.php on line 283
   Allowed memory size of 8388608 bytes exhausted (tried to allocate 72 bytes)

Installing the created php.ini as an additional .ini file in /etc/php.d
enables the install to suceed, e.g. in %install:

 mkdir -p $RPM_BUILD_ROOT%{_sysconfdir}/php.d
 cp -p php.ini $RPM_BUILD_ROOT%{_sysconfdir}/php.d/phpDocumentor.ini

I don't get a good feeling from overriding core php.ini values in a package's
own modular ini file, but there doesn't seem to be another clean way to do
this.  (ideas/comments from those reading are solicited :) )  Perhaps it would
be a good idea to open a bug asking for the core memory limits be updated?
The response at least from the php maintainer would be valuable.

There's nearly a meg of tutorials and examples, which is great, but that's a
bit large to not be put in a -docs subpackage.

SO, to recap:
 * release tag
 * check on fixing rpmlint warning
 * update version
 * decide how php.ini is going to be included in srpm
 * deal with memory limits (a la a /etc/php.d/foo.ini file), open bug
 * split %_docdir/Documentation and %_docdir/tutorials off into a -docs
   package

X package meets naming and packaging guidelines. (release tag)
+ specfile is properly named, is cleanly written and uses macros consistently.
X dist tag is present (but in the wrong place)
+ build root is correct.
+ license field matches the actual license.
+ license is open source-compatible (LGPL). License text included in package.
+ source files match upstream:
 e6f31c313b0b06c09acaf7047e6a5b23  PhpDocumentor-1.3.0RC6.tgz
 e6f31c313b0b06c09acaf7047e6a5b23  PhpDocumentor-1.3.0RC6.tgz.srpm
X latest version is being packaged.
+ BuildRequires are proper.
X package has required requires/provides statements.
+ package builds in mock (devel/x86_64).
O rpmlint is silent. (see below)
O final provides and requires are sane (except as noted above):
 ** phpdoc-1.3.0-0.2.fc5.RC6.noarch.rpm
 == rpmlint
 == provides
 phpdoc = 1.3.0-0.2.fc5.RC6
 == requires
 /usr/bin/php
 php-cli >= 5
 php-pear(PhpDocumentor) = 1.3.0
 ** php-pear-PhpDocumentor-1.3.0-0.2.fc5.RC6.noarch.rpm
 == rpmlint
 W: php-pear-PhpDocumentor wrong-file-end-of-line-encoding
 /usr/share/doc/php-pear-PhpDocumentor-1.3.0/Documentation/Release-old/Release-0.3.0
 W: php-pear-PhpDocumentor dangerous-command-in-%post install
 == provides
 php-pear(PhpDocumentor) = 1.3.0
 php-pear-PhpDocumentor = 1.3.0-0.2.fc5.RC6
 == requires
 /bin/sh
 /usr/bin/pear
 php-pear(PEAR) >= 1.4.9
+ no shared libraries are present.
+ package is not relocatable.
+ owns the directories it creates.
+ doesn't own any directories it shouldn't.
+ no duplicates in %files.
+ file permissions are appropriate.
+ %clean is present.
O %check is not present; no automated test suite
+ sane scriptlets present.
+ code, not content.
X documentation is large (~876k), so -docs subpackage is necessary.
+ %docs are not necessary for the proper functioning of the package.
+ no headers.
+ no pkgconfig files.
+ no libtool .la droppings.
+ not a GUI app.


Comment 2 Chris Weyl 2006-08-29 17:42:30 UTC
While I don't have time at the instant moment to check to see if 16MB is enough
to install this package with, I'm going to mark this bug as depending on bug
196802.  (Definitely something we should validate however -- would solve a lot
of the "bad feelings" I get contemplating overriding a core php.ini value.)

Comment 3 Konstantin Ryabitsev 2006-08-29 18:14:26 UTC
Yeah, let's block on that. I've added the hacks to create a temporary local
php.ini in %post and %postun, but that's a horrible hack. I'd rather wait until
PHP moves out of 1996, when 8M was considered a lot for a process. :)

Comment 4 Christopher Stone 2006-09-03 04:33:42 UTC
There is a new and improved way to do the setup and build sections, see bug
#198706 we are trying to get this into the newrpmspec command.

Comment 5 Konstantin Ryabitsev 2006-09-04 15:29:15 UTC
Okay, sorry for the delay. Here's the updated spec file and srpm:

http://blues.mcgill.ca/~icon/fe/php-pear-PhpDocumentor.spec
http://blues.mcgill.ca/~icon/fe/php-pear-PhpDocumentor-1.3.0-0.3.fc6.src.rpm

Comment 6 Remi Collet 2006-09-17 14:57:09 UTC
One  comment :

%setup -q -n PhpDocumentor-%{version}

Is not safe because package.xml will be store in BUILD directory (can be overide
by another build running at the same time).

%setup -c -q 
cd PhpDocumentor-%{version}

Seem safer
This should probably be add to the PHP Guidelines (for PEAR and PECL)

See /etc/rpmdevtools/spectemplate-php-pear.spec from latest rpmdevtools-5.1-1.fc5

Comment 7 Chris Weyl 2006-12-04 00:58:32 UTC
sorry this took so long.  I totally spaced :(

Anyways.

Building from the new spec, looks like the issues talked about previously have
been addressed.  rpmlint does complain a bit:

[build@zeus php-pear-PhpDocumentor]$ rpmlint
php-pear-PhpDocumentor-1.3.0-0.3.fc5.src.rpm 
W: php-pear-PhpDocumentor no-%build-section

Easily rectifyable by including an empty %build section...

** phpdoc-1.3.0-0.3.fc5.noarch.rpm
== rpmlint
E: phpdoc subdir-in-bin /usr/bin/scripts/makedoc.sh
== provides
phpdoc = 1.3.0-0.3.fc5
== requires
/bin/bash  
/usr/bin/php  
php-pear(PhpDocumentor) = 1.3.0

provides/requires looks sane.  The error is well taken however; on examination
it would appear that /usr/bin/scripts is better placed in %doc.

** php-pear-PhpDocumentor-1.3.0-0.3.fc5.noarch.rpm
== rpmlint
== provides
php-pear(PhpDocumentor) = 1.3.0
php-pear-PhpDocumentor = 1.3.0-0.3.fc5
== requires
/bin/sh  
/bin/sh  
/usr/bin/pear  
/usr/bin/pear  
php-pear(PEAR)  
** php-pear-PhpDocumentor-docs-1.3.0-0.3.fc5.noarch.rpm
== rpmlint
W: php-pear-PhpDocumentor-docs wrong-file-end-of-line-encoding
/usr/share/doc/php-pear-PhpDocumentor-docs-1.3.0/Documentation/Release-old/Release-0.3.0
== provides
php-pear-PhpDocumentor-docs = 1.3.0-0.3.fc5
== requires
php-pear-PhpDocumentor = 1.3.0-0.3.fc5

Again, requires/provides look sane for both packages; the rpmlint warning is
well taken.  

I note the release is still in the "0.x" format...  Fine for now, but I trust
it'll be corrected before check-in.

So, address the rpmlint warnings/errors and the odd placement of scripts/, and
I'll approve.

Comment 8 Konstantin Ryabitsev 2007-01-02 21:22:57 UTC
Hey, Chris:

Finally some time to finish it up. :) All your points have been addressed:

http://blues.mcgill.ca/~icon/fe/php-pear-PhpDocumentor.spec
http://blues.mcgill.ca/~icon/fe/php-pear-PhpDocumentor-1.3.0-1.fc6.src.rpm


Comment 9 Michael Stahnke 2007-01-21 06:52:00 UTC
Is there a reason that one package is called phpdoc and not
php-pear-PhpDocumentor?  The source looks to all be from PhpDocumentor.tgz.  I
realize phpdoc is the name of the binary, and can be run from the command line,
but it seems odd to me to have one source rpm produce packages of completely
different names.  

Also, is the web interface enabled by default?  I didn't see anyting in
/etc/httpd/conf.d.  I guess I thought something like

#Created for PhpDocumentor package
Alias /phpdoc '/usr/share/pear/data/PhpDocumentor'
<Directory /phpdoc>
  Options -Indexes
</Directory>

I am not sure if enabling this on the web by default is allowed (encouraged)
though.  




Comment 10 Konstantin Ryabitsev 2007-01-27 20:56:03 UTC
1. Because php-pear-PhpDocumentor is a library that phpdoc uses. Think of them
as "curl" and "libcurl".
2. I don't think enabling web access by default is a good idea -- it's not a
common usage, and that would make it depend on httpd, which is not necessary for
most uses of phpdoc.

Comment 12 Chris Weyl 2007-02-19 18:00:02 UTC
Note -docs now picks up a rpmlint warning:
W: php-pear-PhpDocumentor-docs wrong-file-end-of-line-encoding
/usr/share/doc/php-pear-PhpDocumentor-docs-1.3.1/Documentation/Release-old/Release-0.3.0

Please fix this before building; everything else looks to have been addressed...

APPROVED

Comment 13 Remi Collet 2007-02-19 18:06:26 UTC
Comment #6 not fixed.

IN : http://fedoraproject.org/wiki/Packaging/PHP

> The source archive contains a package.xml outside any directory, so you have
to use use
> %setup -q -c
> in your %prep section to avoid writing files to the build root. 


Comment 14 Konstantin Ryabitsev 2007-02-19 18:44:48 UTC
Noted. Will address both before importing.

Comment 15 Konstantin Ryabitsev 2007-02-20 22:37:34 UTC
So, is someone supposed to set the "fedora-review" flag to "+"?

Sorry, I'm extremely confused about the new system.

Comment 16 Chris Weyl 2007-02-21 01:30:17 UTC
I think most of us feel that way at this point :)

The fedora-review flag / review procedure is _not_ in effect and has not yet
been approved by FESCo.  (It is currently in use for merge reviews, but as this
is not a core package this is not a merge review.)

Note that to import/branch & add to owners.list, the fedora-cvs flag procedure
has been approved by FESCo and should be used here. (basically, just set
fedora-cvs to '?' and at the same time add a comment as described in the
procedure with owner/branch info.) see
http://www.fedoraproject.org/wiki/CVSAdminProcedure

Comment 17 Chris Weyl 2007-04-08 18:39:04 UTC
Setting fedora-review => +

You should be able to toggle 'fedora-cvs' to ? and import/branch now :)

Comment 18 Chris Weyl 2007-05-10 02:08:59 UTC
ping? :)

Comment 19 Konstantin Ryabitsev 2007-05-10 12:57:38 UTC
Yep, just waiting for the dust from the merge to settle. :)

Comment 20 Michael Stahnke 2007-06-08 21:27:03 UTC
Is there a final/preview build available?  I have need for this on FC6 and would
like to give it a whirl. 



Comment 21 Konstantin Ryabitsev 2007-06-10 21:40:24 UTC
New Package CVS Request
=======================
Package Name: php-pear-PhpDocumentor
Short Description: The complete documentation solution for PHP
Owners: icon
Branches: FC-6 F-7 EL-5
InitialCC: 

TIA!

Comment 22 Kevin Fenzi 2007-06-11 03:39:28 UTC
cvs done.

Comment 23 Konstantin Ryabitsev 2007-06-11 15:29:21 UTC
Thanks, all!

Comment 24 Michael Schwendt 2007-06-12 20:47:00 UTC
Caution!

Package breaks deps in FE-6, and since it is an uncaught mistake
in the spec file at least since 2007-01-29, the other targets
are affected, too.



Comment 25 Konstantin Ryabitsev 2007-06-12 21:02:46 UTC
Yep, my bad. Fixing.

Comment 26 Michael Schwendt 2007-06-14 14:35:12 UTC
php-pear-PhpDocumentor-1.3.2-2.fc6 removed from blacklist



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