Created attachment 844552 [details]
phpcompatinfo.log

phpcompatinfo version 2.26.0

Created attachment 844553 [details]
fedora-review.txt

Generated by fedora-review 0.5.1 (bb9bf27) last change: 2013-12-13
Command line :/usr/bin/fedora-review --mock-config fedora-rawhide-x86_64 -b 1047599
Buildroot used: fedora-rawhide-x86_64
Active plugins: Generic, PHP, Shell-api
Disabled plugins: Java, C/C++, Python, fonts, SugarActivity, Ocaml, Perl, Haskell, R, Ruby
Disabled flags: EXARCH, EPEL5, BATCH, DISTTAG

===== MUST items =====

[!]: Package does not own files or directories owned by other packages.
     Note: Dirs in package are owned also by: /usr/share/pear/Math(php-pear-
     Math-Stats)

     %files should be:

     %dir %{pear_phpdir}/Math
     %{pear_phpdir}/Math/BigInteger.php

[!]: Package is named according to the Package Naming Guidelines.

     Even though existing pkgs use CamelCase, we have been trying to adhere
     to the naming guideline "You should generally use lowercase", so:
     php-phpseclib-math-biginteger

[!]: Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the
     beginning of %install.

     Please remove this EPEL 5 piece "rm -rf $RPM_BUILD_ROOT" in %install

===== SHOULD items =====

[!]: Package has no %clean section with rm -rf %{buildroot} (or
     $RPM_BUILD_ROOT)
     Note: %clean present but not required

     Please remove this EPEL 5 piece

[!]: If the source package does not include license text(s) as a separate file
     from upstream, the packager SHOULD query upstream to include it.

     Please submit a query to upstream to include a license file and link to it
     in your spec.

[x]: Final provides and requires are sane (see attachments).

     Please add the following from phpcompatinfo (can be done after initial
     import; these are already loaded from php-common but we explicitly specify
     all PHP extension requirements)
     * php-date
     * php-pcre

     Description already mentions optional BCMath (php-bcmath) and GMP
     (php-gmp) but you should also add a note about optional OpenSSL (php-openssl)

[x]: Package does not include license text files separate from upstream.

     We usually add a LICENSE file while we are waiting for upstream to add
     one from our request.

I agree.

About name: from very recent Guidelines change: "You should generally use lowercase and turn underscores into dashes unless there's a compelling reason to follow a different upstream convention."

Something to be fixed in "pear make-rpm-spec..."

About:
  [ -f package2.xml ] || mv package.xml package2.xml
  mv package2.xml %{pear_name}-%{version}/%{name}.xml

Cosmetic: It's usually enough to just use the package.xml (having both package.xml and package2.xml could exists in very old package but is not supported anymore (by PEAR), so usually, just check line <package packagerversion="1.9.2" version="2.0"..)

About dependencies, from Guidelines: "PHP extensions must have a Requires on all of the dependent extensions (php-date, php-gd, php-mbstring, ...). "

So yes php-date and php-pcre are mandatory (as extensions/packages split have change and will very probably change again)

From Guidelines: "Common licenses that require including their texts with all derivative works include ASL 2.0, EPL, BSD and MIT. ", so Yes LICENSE file is mandatory for this package.

All of these come from pear make-rpm-spec, so we should probably fix that for the future.

I disagree with Shawn's first suggestion:

%files should be:

%dir %{pear_phpdir}/Math
%{pear_phpdir}/Math/BigInteger.php

There is no difference between ours except mine is simpler. In both cases the package owns /usr/share/pear/Math and /usr/share/pear/Math/BigInteger.php .

As long as there isn't a 'php-filesystem' package or something to own 'shared' namespaces like /usr/share/pear/Crypt , /usr/share/pear/Math etc, packages have no good choice, only three bad ones:

(bad) Own the directory, violating the guideline
(worse) Don't own the directory, and require a package that's completely unrelated and not required at all to ensure that *something* owns the directory
(worse?) Don't own the directory and don't require any other package, leaving the directory unowned unless one of the other packages that happens to own the directory is installed

In any case, both my current version and Shawn's suggestion implement the first of these.

I'll go through and clean the other stuff. Who wants to fix pear make-rpm-spec ? :)

I think (In reply to Adam Williamson from comment #5)
> (bad) Own the directory, violating the guideline

This is not violating the Guidelines and is the right solution.

Packages must own all directories they put files in, except for: 
* any directories owned by the filesystem, man, or other explicitly created -filesystem packages
* any directories owned by other packages in your package's natural dependency chain

You create the dir, and don't require anything that own it, so you have to own it.

> In any case, both my current version and Shawn's suggestion implement the
> first of these.

Yes.

> I'll go through and clean the other stuff. Who wants to fix pear
> make-rpm-spec ? :)

I think it will be me... just need to found time to dig again in this old code...

remi: indeed, rather than "violating the guideline" I should have written "making rpmlint unhappy" :) the rpmlint 'error' is a false positive.

On the licensing issue: the license is included inline in each of the actual source files. I don't believe this is a violation of the license itself or of Fedora's licensing policies, and I don't believe we need to ship a separate copy of the license text or ask upstream to do so.

Updated:

https://www.happyassassin.net/reviews/php-phpseclib-math-biginteger/php-phpseclib-math-biginteger.spec
https://www.happyassassin.net/reviews/php-phpseclib-math-biginteger/php-phpseclib-math-biginteger-0.3.5-2.fc21.src.rpm

The openssl.cfg have obviously not the correct role (upstream bug)

Should be role="cfg", thus will be installed in /etc/pear

Notice: this is a bit ugly to manage in source code, as it is needed to check if run from sources tree or from install one.

Commonly used solution (ex taken from php-bartlett-PHP-CompatInfo)

In Crypt/RSA.php

    $dir = '@cfg_dir@' . DIRECTORY_SEPARATOR . 'Crypt_RSA';
    if (strpos($dir, '@') === false) {
        // PEAR installer was used to install the package
    } else {
        // manual install
        $dir = dirname(__FILE__)
    }
    $filename = $dir . DIRECTORY_SEPARATOR . 'openssl.cnf';

In package.xml

   <file baseinstalldir="Crypt" name="RSA.php" role="php" >
     <tasks:replace from="@cfg_dir@" to="cfg_dir" type="pear-config" />
   </file>
   <file name="openssl.cnf" role="cfg"/>

Sorry for the noise, forget previous comment related to another package.

So, is this one approved or not?

@shawn, what are the remaining blockers ?

If only the missing license, you can probably approved this package, as I have approved the other, thinking having the license text inclued in the file header is enough for a script language.

(In reply to Adam Williamson from comment #12)
> So, is this one approved or not?

Yes :)


(In reply to Remi Collet from comment #13)
> @shawn, what are the remaining blockers ?
> 
> If only the missing license, you can probably approved this package, as I
> have approved the other, thinking having the license text inclued in the
> file header is enough for a script language.

OK

===== MUST items =====
[x]: Package does not own files or directories owned by other packages.
     Note: Dirs in package are owned also by: /usr/share/pear/Math(php-pear-
     Math-Stats)
[x]: Package is named according to the Package Naming Guidelines.
[x]: Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the
     beginning of %install.

===== SHOULD items =====
[x]: Package has no %clean section with rm -rf %{buildroot} (or
     $RPM_BUILD_ROOT)
[-]: If the source package does not include license text(s) as a separate file
     from upstream, the packager SHOULD query upstream to include it.

I forgot....


No blockers.

===== APPROVED =====

New Package SCM Request
=======================
Package Name: php-phpseclib-math-biginteger
Short Description: Pure-PHP arbitrary precision integer arithmetic library
Owners: adamwill
Branches: f20 el6
InitialCC:

Git done (by process-git-requests).

Building for Rawhide. http://koji.fedoraproject.org/koji/taskinfo?taskID=6405059

Package Change Request
======================
Package Name: php-phpseclib-math-biginteger
New Branches: f19 epel7

No owners specified.

Package Change Request
======================
Package Name: php-phpseclib-math-biginteger
New Branches: f19 epel7
Owners: adamwill

Git done (by process-git-requests).