Bug 1047604 - Review Request: php-phpseclib-crypt-random - Random Number Generator
Review Request: php-phpseclib-crypt-random - Random Number Generator
Status: CLOSED RAWHIDE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Remi Collet
Fedora Extras Quality Assurance
:
Depends On: 1047596
Blocks: FE-Legal 1047608 1047611
  Show dependency treegraph
 
Reported: 2013-12-31 22:17 EST by Adam Williamson
Modified: 2014-03-15 18:50 EDT (History)
3 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2014-01-14 14:59:53 EST
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)
phpci.log (9.10 KB, text/plain)
2014-01-05 04:29 EST, Remi Collet
no flags Details
review.txt (8.67 KB, text/plain)
2014-01-05 04:29 EST, Remi Collet
no flags Details

  None (edit)
Description Adam Williamson 2013-12-31 22:17:47 EST
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 04:29:11 EST
Created attachment 845728 [details]
phpci.log
Comment 3 Remi Collet 2014-01-05 04:29:57 EST
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
Comment 4 Remi Collet 2014-01-05 04:30:53 EST
[!]: 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 04:34:13 EST
Notice, could make sense to define and use everywhere:
%global   pear_channel   phpseclib.sourceforge.net
Comment 6 Remi Collet 2014-01-05 04:44:12 EST
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 12:05:10 EST
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 12:06:45 EST
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 12:27:47 EST
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.
Comment 10 Adam Williamson 2014-01-05 12:45:44 EST
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 12:51:48 EST
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 13:00:58 EST
URL fixed for all phpseclib reviews (silent fix, same spec / SRPM URLs as before).
Comment 13 Adam Williamson 2014-01-05 13:02:55 EST
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 03:33:18 EST
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 ==
Comment 15 Adam Williamson 2014-01-10 15:03:51 EST
New Package SCM Request
=======================
Package Name: php-phpseclib-crypt-random
Short Description: Pure PHP Random Number Generator
Owners: adamwill
Branches: f20 el6
InitialCC:
Comment 16 Jon Ciesla 2014-01-10 15:18:54 EST
Git done (by process-git-requests).
Comment 17 Adam Williamson 2014-01-14 14:59:53 EST
Building for Rawhide: http://koji.fedoraproject.org/koji/taskinfo?taskID=6405138
Comment 18 Adam Williamson 2014-02-23 22:25:48 EST
Package Change Request
======================
Package Name: php-phpseclib-crypt-random
New Branches: f19 epel7
Comment 19 Jon Ciesla 2014-02-24 08:24:45 EST
No owners specified.
Comment 20 Adam Williamson 2014-02-25 13:25:24 EST
Package Change Request
======================
Package Name: php-phpseclib-crypt-random
New Branches: f19 epel7
Owners: adamwill
Comment 21 Jon Ciesla 2014-02-25 13:48:52 EST
Git done (by process-git-requests).

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