Bug 1047604

Summary: Review Request: php-phpseclib-crypt-random - Random Number Generator
Product: [Fedora] Fedora Reporter: Adam Williamson <awilliam>
Component: Package ReviewAssignee: Remi Collet <fedora>
Status: CLOSED RAWHIDE QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: medium Docs Contact:
Priority: medium    
Version: rawhideCC: fedora, package-review, shawn
Target Milestone: ---Flags: fedora: fedora-review+
gwync: fedora-cvs+
Target Release: ---   
Hardware: All   
OS: Linux   
Fixed In Version: Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2014-01-14 19:59:53 UTC Type: ---
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
Bug Depends On: 1047596    
Bug Blocks: 182235, 1047608, 1047611    
Description Flags
review.txt none

Description Adam Williamson 2014-01-01 03:17:47 UTC
Spec URL: https://www.happyassassin.net/reviews/php-phpseclib-Crypt-Random/php-phpseclib-Crypt-Random.spec
SRPM URL: https://www.happyassassin.net/reviews/php-phpseclib-Crypt-Random/php-phpseclib-Crypt-Random-0.3.5-1.fc21.src.rpm
Description: PHP Random Number Generator.
Fedora Account System Username: adamwill

Comment 2 Remi Collet 2014-01-05 09:29:11 UTC
Created attachment 845728 [details]

Comment 3 Remi Collet 2014-01-05 09:29:57 UTC
Created attachment 845729 [details]

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

Comment 4 Remi Collet 2014-01-05 09:30:53 UTC
[!]: 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.

Comment 5 Remi Collet 2014-01-05 09:34:13 UTC
Notice, could make sense to define and use everywhere:
%global   pear_channel   phpseclib.sourceforge.net

Comment 6 Remi Collet 2014-01-05 09:44:12 UTC
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

Comment 7 Adam Williamson 2014-01-05 17:05:10 UTC
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.

Comment 8 Adam Williamson 2014-01-05 17:06:45 UTC
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.

Comment 9 Remi Collet 2014-01-05 17:27:47 UTC
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

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.

Comment 10 Adam Williamson 2014-01-05 17:45:44 UTC
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.

Comment 11 Adam Williamson 2014-01-05 17:51:48 UTC
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.

Comment 12 Adam Williamson 2014-01-05 18:00:58 UTC
URL fixed for all phpseclib reviews (silent fix, same spec / SRPM URLs as before).

Comment 13 Adam Williamson 2014-01-05 18:02:55 UTC
adding legal for confirmation of my position in c#10 (if you're not going to confirm it, then go away :P)

Comment 14 Remi Collet 2014-01-07 08:33:18 UTC
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


Comment 15 Adam Williamson 2014-01-10 20:03:51 UTC
New Package SCM Request
Package Name: php-phpseclib-crypt-random
Short Description: Pure PHP Random Number Generator
Owners: adamwill
Branches: f20 el6

Comment 16 Gwyn Ciesla 2014-01-10 20:18:54 UTC
Git done (by process-git-requests).

Comment 17 Adam Williamson 2014-01-14 19:59:53 UTC
Building for Rawhide: http://koji.fedoraproject.org/koji/taskinfo?taskID=6405138

Comment 18 Adam Williamson 2014-02-24 03:25:48 UTC
Package Change Request
Package Name: php-phpseclib-crypt-random
New Branches: f19 epel7

Comment 19 Gwyn Ciesla 2014-02-24 13:24:45 UTC
No owners specified.

Comment 20 Adam Williamson 2014-02-25 18:25:24 UTC
Package Change Request
Package Name: php-phpseclib-crypt-random
New Branches: f19 epel7
Owners: adamwill

Comment 21 Gwyn Ciesla 2014-02-25 18:48:52 UTC
Git done (by process-git-requests).