Bug 1047604

Updated:

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

Created attachment 845728 [details]
phpci.log

Created attachment 845729 [details]
review.txt

Generated by fedora-review 0.5.0 (920221d) last change: 2013-08-30
Command line :/usr/bin/fedora-review -b 1047604

[!]: Rpmlint is run on all rpms the build produces.
  invalid-url URL: http://phpseclib.sourceforge.net/package/Crypt_Random HTTP Error 404: Not Found

[!]: Requires correct, justified where necessary.

  # Optional, From package.xml
  Requires: php-openssl
  Requires: php-mcrypt
  Requires: php-pear(phpseclib.sourceforge.net/Crypt_AES) >= 0.2.1
  Requires: php-pear(phpseclib.sourceforge.net/Crypt_TripleDES) >= 0.2.1
  Requires: php-pear(phpseclib.sourceforge.net/Crypt_DES) >= 0.2.1
  Requires: php-pear(phpseclib.sourceforge.net/Crypt_RC4) >= 0.2.1

  This is your choice to require or not optional dep.
  I personnally think  it's better for user experience, especially
  as this doesn't pull "huge" thing, only small pure PHP library.

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

     As you have a link to the license file in package.xml you
     can add this file to the package, which will avoid having
     to block this review for missing License.

Notice, could make sense to define and use everywhere:
%global   pear_channel   phpseclib.sourceforge.net

Forget my comment about dependencies.

Reading code, only "php-openssl" is needed

All others (mcrypt, session, Crypt_*) are only fallbacks which will never be used.

So please:
-Requires: php-session
+Requires: php-openssl

The use of openssl is also optional, I believe:

        // method 1. the fastest
        if (function_exists('openssl_random_pseudo_bytes')) {
            return openssl_random_pseudo_bytes($length);
        }
        // method 2
        static $fp = true;
        if ($fp === true) {

all the openssl stuff just above it is irrelevant because it's for Windows.

Again, the license *is included inline in the source*. Each of these packages ultimately contains a small number of PHP files and the XML file, and all the PHP files have the license at the top. There's no need to include the license text in a separate file if it''s already right there in the source.

Yes openssl is optional.

But as noted in the code: method 1. the fastest
And, well, openssl is always here ;) so this is only cosmetic.
Requires the prefered extension.


About License
https://fedoraproject.org/wiki/Packaging:LicensingGuidelines#License_Text

I will tend to agree with you that the License text is included in the single source script, but the Guidelines seems ambiguous.

I will prefer to get confirmation from "legal", and probably fix the Guidelines for this specific case.

I'm an amateur license nerd. Really, it's fine.

I wouldn't say the guidelines are 'ambiguous', I'd just say they're maybe a bit hard to read. But if you follow the logic...

First paragraph starts with an 'if' clause:

"If the source package includes the text of the license(s) in its own file"

Ours doesn't. So we can completely disregard this paragraph. That's not a showstopper, though, the statement is not yet terminated. The following paragraph comes with an implied 'elseif' ;)

The rest of the opening of that following paragraph reads:

"In cases where the upstream has chosen a license that requires that a copy of the license text be distributed along with the binaries and/or source code, but does not provide a copy of the license text (in the source tree, or in some rare cases, anywhere)"

We're not in one of *those* cases either. If you read it carefully this paragraph has an implied converse: "cases where the upstream has chosen a license that requires a copy of the license text...and does provide a copy of the license text (in the source tree or somewhere)".

That's the case we're in. Upstream has chosen a license that requires a copy of the license text, and provided one. The license text being in line with the source is the strongest possible association you can have between license and source. Also providing a copy as a separate file is entirely redundant.

if this was C and it was getting compiled, that might be different, because then there wouldn't necessarily be a copy of the license included with the binary distribution. but that doesn't apply here. whether you install the 'binary' .rpm or the 'source' .rpm, you get a copy of the license text along with the distribution.

URL fixed for all phpseclib reviews (silent fix, same spec / SRPM URLs as before).

adding legal for confirmation of my position in c#10 (if you're not going to confirm it, then go away :P)

We are not going to wait for legal ack, as we agree (I'm 95% sure it's ok) so going to go (but legal confirmation still welcome)

[x]: Rpmlint is run on all rpms the build produces.
     URL fixed

[x]: Requires correct, justified where necessary.
     I still think php-openssl is better, but this are only 
     optional dep, so packager choice, not a blocker

== APPROVED ==

New Package SCM Request
=======================
Package Name: php-phpseclib-crypt-random
Short Description: Pure PHP Random Number Generator
Owners: adamwill
Branches: f20 el6
InitialCC:

Git done (by process-git-requests).

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

Package Change Request
======================
Package Name: php-phpseclib-crypt-random
New Branches: f19 epel7

No owners specified.

Package Change Request
======================
Package Name: php-phpseclib-crypt-random
New Branches: f19 epel7
Owners: adamwill

Git done (by process-git-requests).