https://bugzilla.redhat.com/show_bug.cgi?id=1795461
Ankur Sinha (FranciscoD) <sanjay.an...@gmail.com> changed:
What |Removed |Added
----------------------------------------------------------------------------
Status|ASSIGNED |POST
Flags|fedora-review? |fedora-review+
--- Comment #20 from Ankur Sinha (FranciscoD) <sanjay.an...@gmail.com> ---
(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@lists.fedoraproject.org
To unsubscribe send an email to package-review-le...@lists.fedoraproject.org
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@lists.fedoraproject.org
Do not reply to spam on the list, report it:
https://pagure.io/fedora-infrastructure
