[Bug 1795461] Review Request: practrand - Software package for the Randon number generation & testing

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



https://bugzilla.redhat.com/show_bug.cgi?id=1795461

Ankur Sinha (FranciscoD) <sanjay.ankur@xxxxxxxxx> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|ASSIGNED                    |POST
              Flags|fedora-review?              |fedora-review+



--- Comment #20 from Ankur Sinha (FranciscoD) <sanjay.ankur@xxxxxxxxx> ---
(In reply to Jiri Hladky from comment #19)
> Hi Ankur,
> 
> thanks a lot for the review! My answers are below:
> 
> > upstream doesn't have the 0.951 release here: https://sourceforge.net/projects/pracrand/files/
> > The source URL you use is your own fork: https://github.com/jirka-h/PractRand/
> > So, are we packaging your fork here?
> 
> Yes, for the time being. The author is currently not responding and several
> forks on GitHub have been created. 
> 
> I have taken the last development version I got from the author. (We have
> collaborated to prepare the release on Linux.) I made some minor
> modifications, mainly regarding the documentation and man pages:
> https://github.com/jirka-h/PractRand/commits/master
> 
> I plan to merge the fork as soon as the author will resume the development. 

OK. That sounds fine. Please make a note of the fork in the spec so that it is
clear to any readers.


> 
> > - Please remove the license.txt file from docs and mark it using the %license macro
> >  License file license.txt is not marked as %license See: https://docs.fedoraproject.org/en-US/packaging-guidelines/LicensingGuidelines/#_license_text
> 
> Completed. Thanks for the hint!
> 
> > - You also do not need to copy the docs to the docdir. You can just use: %doc doc/ in %files and that'll copy over the files.
> 
> Fixed.
> 
> > - build flags aren't used in the compilation commands.
> 
> Fixed. 
> 
> > - should the package include a -devel sub-package that includes headers and so on?
> 
> I have been thinking about it but I came to the conclusion it's not a good
> idea for several reasons:

Sounds good.

> 
>  - software is developed as an application, not as a library. Without
> upstream support, creating a shared library is not recommended:
> https://docs.fedoraproject.org/en-US/packaging-guidelines/#_shared_libraries
> 
>  - API is not documented
> 
>  - there is an excellent command-line interface to test any data from stdin.
> This is a very flexible approach and it follows the *nix strategy of small
> utilities performing one task and performing it well.
> 
>  - to test custom RNG using the current implementation you have to create a
> C++ class derived from PractRand::RNGs class for your RNG. See also
> https://github.com/jirka-h/PractRand/blob/master/doc/Tests_overview.txt#L83
> Let me cite :"it tends to require more work as you have to understand some
> of the subtleties of the internals of PractRand."
> 
>  - on systems with multiple CPUs, using one process to generate test data
> and running practrand-RNG to test it as another process, can run actually
> faster than integrating new RNG to the practrand-RNG test. This is because
> the pipe approach can use more CPUs. You are wasting some resources by
> sending the data over the pipe, but you are using more CPUs which outweighs
> the cost of sending data through the pipe. Tested with jsf64 on a laptop
> with 4 cores (8 CPUs thanks to hyperthreading):
> 
> time ./practrand-RNG_test jsf64 -tlmax 4G -multithreaded
> real    0m16.555s
> user    0m44.767s
> sys     0m0.146s
> 
> time { ./practrand-RNG_output jsf64 $(bc <<< 4*1024^3) |
> ./practrand-RNG_test stdin32 -tlmax 4G -multithreaded ; }
> real    0m16.065s
> user    0m48.656s
> sys     0m2.445s
> 
> The latter approach is faster (comparing real time) but takes more resources
> (compare user time). 
> 
> > Any reason to not use the included Makefile?
> Makefile coming with the source code is currently broken. The author has
> plans to fix it in the new version. When it's ready, I will switch to use
> it. 

Ah, sounds good. Also worth adding a comment in the spec.

> 
> Updated SPEC file and SRPM:
> 
> Spec URL: https://jhladky.fedorapeople.org/practrand.spec
> SRPM URL: https://jhladky.fedorapeople.org/practrand-0.951-2.fc32.src.rpm
> 
> Could you please have a look?

Looks good now. XXX APPROVED XXX

Cheers!
Ankur


-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
_______________________________________________
package-review mailing list -- package-review@xxxxxxxxxxxxxxxxxxxxxxx
To unsubscribe send an email to package-review-leave@xxxxxxxxxxxxxxxxxxxxxxx
Fedora Code of Conduct: https://docs.fedoraproject.org/en-US/project/code-of-conduct/
List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines
List Archives: https://lists.fedoraproject.org/archives/list/package-review@xxxxxxxxxxxxxxxxxxxxxxx
Do not reply to spam on the list, report it: https://pagure.io/fedora-infrastructure




[Index of Archives]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Yosemite Conditions]     [KDE Users]

  Powered by Linux