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
