[Bug 1795461] New: Review Request: practrand - Software package for the Randon number generation & testing
https://bugzilla.redhat.com/show_bug.cgi?id=1795461
Bug ID: 1795461 Summary: Review Request: practrand - Software package for the Randon number generation & testing Product: Fedora Version: rawhide Hardware: All OS: Linux Status: NEW Component: Package Review Severity: medium Priority: medium Assignee: nobody@fedoraproject.org Reporter: hladky.jiri@gmail.com QA Contact: extras-qa@fedoraproject.org CC: package-review@lists.fedoraproject.org Target Milestone: --- Classification: Fedora
Spec URL: https://jhladky.fedorapeople.org/practrand.spec SRPM URL: https://jhladky.fedorapeople.org/practrand-0.95-1.fc29.src.rpm
Description: Software package for the Random number generation & testing. A suite of statistical tests for fast PRNGs. Multithreaded for speed, command-line tools for automation, no upper limit on data size. A variety of C++ pseudo-random number generators with well-designed interfaces aimed at practical uses, not just research.
Fedora Account System Username: jhladky
https://bugzilla.redhat.com/show_bug.cgi?id=1795461
Jiri Hladky hladky.jiri@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |hladky.jiri@gmail.com Doc Type|--- |If docs needed, set a value
https://bugzilla.redhat.com/show_bug.cgi?id=1795461
Artur Iwicki fedora@svgames.pl changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |fedora@svgames.pl
--- Comment #1 from Artur Iwicki fedora@svgames.pl ---
Summary: Software package for the Randon number generation & testing
A typo here - "randon" instead of "random" ("m" replaced with "n").
Group: Applications/System
The Group: tag is not used in Fedora. https://docs.fedoraproject.org/en-US/packaging-guidelines/#_tags_and_section...
Source0: https://sourceforge.net/projects/pracrand/files/PractRand_%%7Bversion%7D.zip
This URL does not exist - there's only "PractRand-pre0.95.zip".
BuildRequires: gcc, help2man, valgrind, dos2unix [...] g++ -c src/*.cpp src/RNGs/*.cpp src/RNGs/other/*.cpp -I include -std=c++11 -O3 -g
/usr/bin/g++ is provided by the "gcc-c++" package, not "gcc".
You should probably also call %set_build_flags at the start of %build so Fedora's CFLAGS and LDFLAGS are applied.
Also, since you're calling g++ directly, it might be good to leave a comment saying that the upstream sources don't contain a Makefile (nor anything similar).
mkdir -p %{buildroot}%{_defaultdocdir}/%{name} cp -p doc/* %{buildroot}%{_defaultdocdir}/%{name}
Reading through the Packaging Guidelines, I think you should use %{_pkgdocdir} here instead of %{_defaultdocdir}. https://docs.fedoraproject.org/en-US/packaging-guidelines/#_documentation
https://bugzilla.redhat.com/show_bug.cgi?id=1795461
Ankur Sinha (FranciscoD) sanjay.ankur@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |ASSIGNED CC| |sanjay.ankur@gmail.com Assignee|nobody@fedoraproject.org |sanjay.ankur@gmail.com Flags| |fedora-review?
--- Comment #2 from Ankur Sinha (FranciscoD) sanjay.ankur@gmail.com --- I can review this one.
https://bugzilla.redhat.com/show_bug.cgi?id=1795461
--- Comment #3 from Ankur Sinha (FranciscoD) sanjay.ankur@gmail.com --- As Artur pointed out, you need to add g++ to the BuildRequires. The build does not currently succeed in mock because g++ is missing:
+ RPM_EC=0 ++ jobs -p + exit 0 Executing(%build): /bin/sh -e /var/tmp/rpm-tmp.sxPUlv + umask 022 + cd /builddir/build/BUILD + cd practrand-0.95 + g++ -c src/math.cpp src/non_uniform.cpp src/platform_specifics.cpp src/rand.cpp src/sha2.cpp src/test_batteries.cpp src/tests.cpp src/RNGs/arbee.cpp src/RNGs/chacha.cpp src/RNGs/efiix.cpp src/RNGs/hc256.cpp src/RNGs/isaac.cpp src/RNGs/jsf.cpp src/RNGs/mt19937.cpp src/RNGs/rarns.cpp src/RNGs/salsa.cpp src/RNGs/sfc.cpp src/RNGs/sha2_based_pool.cpp src/RNGs/trivium.cpp src/RNGs/xsm.cpp src/RNGs/other/fibonacci.cpp src/RNGs/other/indirection.cpp src/RNGs/other/mult.cpp src/RNGs/other/simple.cpp src/RNGs/other/transform.cpp -I include -std=c++11 -O3 -g /var/tmp/rpm-tmp.sxPUlv: line 33: g++: command not found error: Bad exit status from /var/tmp/rpm-tmp.sxPUlv (%build)
Can you please correct the sourceurl also as Artur pointed out? Fedora-review gives a warning already.
Could you also please test that your src rpm builds in mock (if it doesn't, it won't build on koji either): https://fedoraproject.org/wiki/Using_Mock_to_test_package_builds
I can continue with a full review once the build succeeds.
Cheers,
https://bugzilla.redhat.com/show_bug.cgi?id=1795461
--- Comment #4 from Jiri Hladky hladky.jiri@gmail.com --- Thanks a lot for the review!
I have fixed the issues pointed out in comment #1 and comment #3. I had to do some modifications for s390x as well.
Modified SPEC and SRPM files are here:
Spec URL: https://jhladky.fedorapeople.org/practrand.spec SRPM URL: https://jhladky.fedorapeople.org/practrand-0.95-1.fc29.src.rpm
And there is the link to the successful Koji scratch build:
koji build --scratch rawhide practrand-0.95-1.fc29.src.rpm https://koji.fedoraproject.org/koji/taskinfo?taskID=41385054
Can you please correct the sourceurl also as Artur pointed out? Fedora-review gives a warning already.
This is the last unresolved issue. I got a preliminary version but it's not released yet. There are few minors issues which the author would like to fix before releasing it.
Let's put this review on hold until 0.95 release is out. I will then let you know and we will finish the process.
Thanks a lot! Jirka
https://bugzilla.redhat.com/show_bug.cgi?id=1795461
--- Comment #5 from Ankur Sinha (FranciscoD) sanjay.ankur@gmail.com --- Hello!
Any update on this one?
https://bugzilla.redhat.com/show_bug.cgi?id=1795461
Ankur Sinha (FranciscoD) sanjay.ankur@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Flags| |needinfo?(hladky.jiri@gmail | |.com)
--- Comment #6 from Ankur Sinha (FranciscoD) sanjay.ankur@gmail.com --- Hello Jirka,
Any progress here?
Cheers, Ankur
https://bugzilla.redhat.com/show_bug.cgi?id=1795461
Jiri Hladky hladky.jiri@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Flags|needinfo?(hladky.jiri@gmail | |.com) |
--- Comment #7 from Jiri Hladky hladky.jiri@gmail.com --- Hello Ankur,
I'm still waiting for the author to release the new version with some changes needed to package as the rpm. He is working on a new feature.
Let me ping him if he can estimate a release date.
Cheers, Jirka
https://bugzilla.redhat.com/show_bug.cgi?id=1795461
--- Comment #8 from Jiri Hladky hladky.jiri@gmail.com --- Hello Ankur,
I have talked to the upstream developer and a new version should be ready in two weeks. Please bear with us.
Thanks! Jirka
https://bugzilla.redhat.com/show_bug.cgi?id=1795461
--- Comment #9 from Ankur Sinha (FranciscoD) sanjay.ankur@gmail.com --- Hi Jiri,
Any updates here?
Cheers, Ankur
https://bugzilla.redhat.com/show_bug.cgi?id=1795461
--- Comment #10 from Jiri Hladky hladky.jiri@gmail.com --- Hi Ankur,
the upstream version (which includes the changes necessary to make it a rpm package) is delayed, but it should be released really soon:
=========================================================== Cerian Knight - 2020-07-26 I am nearing the final milestone for release ===========================================================
Please bear with us!
Thanks Jirka
https://bugzilla.redhat.com/show_bug.cgi?id=1795461
--- Comment #11 from Ankur Sinha (FranciscoD) sanjay.ankur@gmail.com --- Hi Jirka,
Hope you are doing well.
Just a general check to see if the release was made so this can proceed?
Cheers, Ankur
https://bugzilla.redhat.com/show_bug.cgi?id=1795461
--- Comment #12 from Jiri Hladky hladky.jiri@gmail.com --- Hi Ankur,
I'm really sorry it took so long.
Finally, everything is ready for the final review. I have fixed all the issues discussed above.
Spec URL: https://jhladky.fedorapeople.org/practrand.spec SRPM URL: https://jhladky.fedorapeople.org/practrand-0.951-1.fc32.src.rpm
There is the link to the successful Koji scratch build:
koji build --scratch rawhide practrand-0.951-1.fc32.src.rpm https://koji.fedoraproject.org/koji/taskinfo?taskID=62609232
Could you please help me to finish the review?
Thanks a lot! Jirka
https://bugzilla.redhat.com/show_bug.cgi?id=1795461
--- Comment #13 from Ankur Sinha (FranciscoD) sanjay.ankur@gmail.com --- Hi Jirka,
Thanks very much! I'll try to complete the review in the coming weeks.
Cheers,
https://bugzilla.redhat.com/show_bug.cgi?id=1795461
--- Comment #14 from Jiri Hladky hladky.jiri@gmail.com --- Awesome. I do appreciate that!
Jirka
https://bugzilla.redhat.com/show_bug.cgi?id=1795461
--- Comment #15 from Jiri Hladky hladky.jiri@gmail.com --- Hi Ankur,
have you a chance to check it out?
As we already did an initial review and fixed many problems (see comment #3), I think that the final approval will be in fact pretty easy and quick.
Thanks! Jirka
https://bugzilla.redhat.com/show_bug.cgi?id=1795461
--- Comment #16 from Ankur Sinha (FranciscoD) sanjay.ankur@gmail.com --- Hi Jirka,
Sorry, I've not had a chance to go through it yet. I'm afraid I need to do a full review (with fedora-review etc.) before it can be approved.
I'll try and do it before the start of next week,
Cheers, Ankur
https://bugzilla.redhat.com/show_bug.cgi?id=1795461
--- Comment #17 from Jiri Hladky hladky.jiri@gmail.com --- Sounds good:-)
Thank you, Ankur!
https://bugzilla.redhat.com/show_bug.cgi?id=1795461
--- Comment #18 from Ankur Sinha (FranciscoD) sanjay.ankur@gmail.com --- Looks good, a few queries before it can be approved:
Package Review ==============
Legend: [x] = Pass, [!] = Fail, [-] = Not applicable, [?] = Not evaluated [ ] = Manual review needed
Issues: ======= - 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?
- 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/LicensingGuideline...
- 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.
- build flags aren't used in the compilation commands.
- should the package include a -devel sub-package that includes headers and so on?
===== MUST items =====
C/C++: [x]: Package does not contain kernel modules. [x]: Package contains no static executables. [x]: If your application is a C or C++ application you must list a BuildRequires against gcc, gcc-c++ or clang. [x]: Header files in -devel subpackage, if present. [x]: Package does not contain any libtool archives (.la) [x]: Rpath absent or only used for internal libs.
Generic: [x]: Package is licensed with an open-source compatible license and meets other legal requirements as defined in the legal section of Packaging Guidelines. [x]: License field in the package spec file matches the actual license. Note: Checking patched sources after %prep for licenses. Licenses found: "Unknown or generated". 156 files have unknown license. Detailed output of licensecheck in /home/asinha/dump/fedora- reviews/1795461-practrand/licensecheck.txt [x]: License file installed when any subpackage combination is installed. [!]: %build honors applicable compiler flags or justifies otherwise. ^ Build flags are set, but not used in the build:
+ g++ -c src/math.cpp src/non_uniform.cpp src/platform_specifics.cpp src/rand.cpp src/sha2.cpp src/test_batteries.cpp src/tests.cpp src/RNGs/arbee.cpp src/RNGs/chacha.cpp src/RNGs/efiix.cpp src/RNGs/hc256.cpp src/RNGs/isaac.cpp src/RNGs/jsf.cpp src/RNGs/mt19937.cpp src/RNGs/rarns.cpp src/RNGs/salsa.cpp src/RNGs/sfc.cpp src/RNGs/sha2_based_pool.cpp src/RNGs/trivium.cpp src/RNGs/xsm.cpp src/RNGs/other/fibonacci.cpp src/RNGs/other/indirection.cpp src/RNGs/other/mult.cpp src/RNGs/other/simple.cpp src/RNGs/other/transform.cpp -I include -std=c++11 -O3 -g + g++ -o practrand-RNG_test tools/RNG_test.cpp arbee.o chacha.o efiix.o fibonacci.o hc256.o indirection.o isaac.o jsf.o math.o mt19937.o mult.o non_uniform.o platform_specifics.o rand.o rarns.o salsa.o sfc.o sha2.o sha2_based_pool.o simple.o test_batteries.o tests.o transform.o trivium.o xsm.o -I include -I tools -pthread -std=c++11 -O3 -g + g++ -o practrand-RNG_output tools/RNG_output.cpp arbee.o chacha.o efiix.o fibonacci.o hc256.o indirection.o isaac.o jsf.o math.o mt19937.o mult.o non_uniform.o platform_specifics.o rand.o rarns.o salsa.o sfc.o sha2.o sha2_based_pool.o simple.o test_batteries.o tests.o transform.o trivium.o xsm.o -I include -I tools -pthread -std=c++11 -O3 -g + g++ -o practrand-RNG_benchmark tools/RNG_benchmark.cpp arbee.o chacha.o efiix.o fibonacci.o hc256.o indirection.o isaac.o jsf.o math.o mt19937.o mult.o non_uniform.o platform_specifics.o rand.o rarns.o salsa.o sfc.o sha2.o sha2_based_pool.o simple.o test_batteries.o tests.o transform.o trivium.o xsm.o -I include -I tools -pthread -std=c++11 -O3 -g
You'll have to use:
$CC $CXXFLAGS ...
to use the exported variables
Any reason to not use the included Makefile?
[x]: Package contains no bundled libraries without FPC exception. [x]: Changelog in prescribed format. [x]: Sources contain only permissible code or content. [-]: Package contains desktop file if it is a GUI application. [!]: Development files must be in a -devel package ^ Is this meant to be used as a library too? Should the headers be packaged?
[x]: Package uses nothing in %doc for runtime. [x]: Package consistently uses macros (instead of hard-coded directory names). [x]: Package is named according to the Package Naming Guidelines. [x]: Package does not generate any conflict. [x]: Package obeys FHS, except libexecdir and /usr/target. [-]: If the package is a rename of another package, proper Obsoletes and Provides are present. [x]: Requires correct, justified where necessary. [x]: Spec file is legible and written in American English. [-]: Package contains systemd file(s) if in need. [x]: Useful -debuginfo package or justification otherwise. [x]: Package is not known to require an ExcludeArch tag. [-]: Large documentation must go in a -doc subpackage. Large could be size (~1MB) or number of files. Note: Documentation size is 256000 bytes in 22 files. [x]: Package complies to the Packaging Guidelines [x]: Package successfully compiles and builds into binary rpms on at least one supported primary architecture. Scratch build for rawhide looks good: https://koji.fedoraproject.org/koji/taskinfo?taskID=65029183
[x]: Package installs properly. [x]: Rpmlint is run on all rpms the build produces. Note: There are rpmlint messages (see attachment). [x]: Package requires other packages for directories it uses. [x]: Package must own all directories that it creates. [x]: Package does not own files or directories owned by other packages. [x]: Package uses either %{buildroot} or $RPM_BUILD_ROOT [x]: Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the beginning of %install. [x]: Macros in Summary, %description expandable at SRPM build time. [x]: Dist tag is present. [x]: Package does not contain duplicates in %files. [x]: Permissions on files are set properly. [x]: Package must not depend on deprecated() packages. [x]: Package use %makeinstall only when make install DESTDIR=... doesn't work. [x]: Package is named using only allowed ASCII characters. [x]: Package does not use a name that already exists. [x]: Package is not relocatable. [x]: Sources used to build the package match the upstream source, as provided in the spec URL. [x]: Spec file name must match the spec package %{name}, in the format %{name}.spec. [x]: File names are valid UTF-8. [x]: Packages must not store files under /srv, /opt or /usr/local
===== SHOULD items =====
Generic: [-]: If the source package does not include license text(s) as a separate file from upstream, the packager SHOULD query upstream to include it. [x]: Final provides and requires are sane (see attachments). [?]: Package functions as described. ^ Not tested this out yet.
[x]: Latest version is packaged. [x]: Package does not include license text files separate from upstream. [-]: Sources are verified with gpgverify first in %prep if upstream publishes signatures. Note: gpgverify is not used. [-]: Description and summary sections in the package spec file contains translations for supported Non-English languages, if available. [x]: Package should compile and build into binary rpms on all supported architectures. [x]: %check is present and all tests pass. [x]: Packages should try to preserve timestamps of original installed files. [x]: Reviewer should test that the package builds in mock. [x]: Buildroot is not present [x]: Package has no %clean section with rm -rf %{buildroot} (or $RPM_BUILD_ROOT) [x]: No file requires outside of /etc, /bin, /sbin, /usr/bin, /usr/sbin. [x]: Fully versioned dependency in subpackages if applicable. [x]: Packager, Vendor, PreReq, Copyright tags should not be in spec file [x]: Sources can be downloaded from URI in Source: tag [x]: SourceX is a working URL. [x]: Spec use %global instead of %define unless justified.
===== EXTRA items =====
Generic: [x]: Rpmlint is run on debuginfo package(s). Note: No rpmlint messages. [x]: Rpmlint is run on all installed packages. Note: There are rpmlint messages (see attachment). [x]: Large data in /usr/share should live in a noarch subpackage if package is arched. [x]: Spec file according to URL is the same as in SRPM.
Rpmlint ------- Checking: practrand-0.951-1.fc35.x86_64.rpm practrand-debuginfo-0.951-1.fc35.x86_64.rpm practrand-debugsource-0.951-1.fc35.x86_64.rpm practrand-0.951-1.fc35.src.rpm practrand.x86_64: W: spelling-error %description -l en_US Multithreaded -> Multicolored practrand.src: W: spelling-error %description -l en_US Multithreaded -> Multicolored 4 packages and 0 specfiles checked; 0 errors, 2 warnings.
Rpmlint (debuginfo) ------------------- Checking: practrand-debuginfo-0.951-1.fc35.x86_64.rpm 1 packages and 0 specfiles checked; 0 errors, 0 warnings.
Rpmlint (installed packages) ---------------------------- practrand.x86_64: W: spelling-error %description -l en_US Multithreaded -> Multicolored 3 packages and 0 specfiles checked; 0 errors, 1 warnings.
Source checksums ---------------- https://github.com/jirka-h/PractRand/archive/0.951/PractRand-0.951.tar.gz : CHECKSUM(SHA256) this package : 0e4e172449d25df1eeb149dae8614f3cd2b03110ffdafc2f659097040df0f558 CHECKSUM(SHA256) upstream package : 0e4e172449d25df1eeb149dae8614f3cd2b03110ffdafc2f659097040df0f558
Requires -------- practrand (rpmlib, GLIBC filtered): libc.so.6()(64bit) libgcc_s.so.1()(64bit) libgcc_s.so.1(GCC_3.0)(64bit) libm.so.6()(64bit) libpthread.so.0()(64bit) libstdc++.so.6()(64bit) libstdc++.so.6(CXXABI_1.3)(64bit) libstdc++.so.6(CXXABI_1.3.8)(64bit) rtld(GNU_HASH)
practrand-debuginfo (rpmlib, GLIBC filtered):
practrand-debugsource (rpmlib, GLIBC filtered):
Provides -------- practrand: practrand practrand(x86-64)
practrand-debuginfo: debuginfo(build-id) practrand-debuginfo practrand-debuginfo(x86-64)
practrand-debugsource: practrand-debugsource practrand-debugsource(x86-64)
Generated by fedora-review 0.7.6 (b083f91) last change: 2020-11-10 Command line :/usr/bin/fedora-review -b 1795461 Buildroot used: fedora-rawhide-x86_64 Active plugins: Shell-api, C/C++, Generic Disabled plugins: PHP, Haskell, R, Python, Java, Ocaml, fonts, Perl, SugarActivity Disabled flags: EPEL6, EPEL7, DISTTAG, BATCH, EXARCH
https://bugzilla.redhat.com/show_bug.cgi?id=1795461
--- Comment #19 from Jiri Hladky hladky.jiri@gmail.com --- 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.
- 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/LicensingGuideline...
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:
- 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.
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?
Thanks! Jirka
https://bugzilla.redhat.com/show_bug.cgi?id=1795461
Ankur Sinha (FranciscoD) sanjay.ankur@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |POST Flags|fedora-review? |fedora-review+
--- Comment #20 from Ankur Sinha (FranciscoD) sanjay.ankur@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/LicensingGuideline...
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
https://bugzilla.redhat.com/show_bug.cgi?id=1795461
--- Comment #21 from Jiri Hladky hladky.jiri@gmail.com --- Hi Ankur,
thanks a lot for the review!
Good point about adding the comments to the spec file. The updated version is here: https://jhladky.fedorapeople.org/practrand.spec
I have requested a new git repository here: https://pagure.io/releng/fedora-scm-requests/issue/33487
Jirka
https://bugzilla.redhat.com/show_bug.cgi?id=1795461
--- Comment #22 from Gwyn Ciesla gwync@protonmail.com --- (fedscm-admin): The Pagure repository was created at https://src.fedoraproject.org/rpms/practrand
https://bugzilla.redhat.com/show_bug.cgi?id=1795461
--- Comment #23 from Jiri Hladky hladky.jiri@gmail.com --- The rawhide package was successfully created.
I have created packages for F32, F33, F34, and epel7, and epel8 and submitted them to bodhi.
https://bodhi.fedoraproject.org/updates/FEDORA-EPEL-2021-8228f43268 https://bodhi.fedoraproject.org/updates/FEDORA-EPEL-2021-55fd7ff3fd https://bodhi.fedoraproject.org/updates/FEDORA-2021-320f9492ce https://bodhi.fedoraproject.org/updates/FEDORA-2021-9fac10904f https://bodhi.fedoraproject.org/updates/FEDORA-2021-f9297de48e
https://bugzilla.redhat.com/show_bug.cgi?id=1795461
Ankur Sinha (FranciscoD) sanjay.ankur@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|POST |CLOSED Resolution|--- |RAWHIDE Last Closed| |2021-04-16 10:14:22
--- Comment #24 from Ankur Sinha (FranciscoD) sanjay.ankur@gmail.com --- Sounds good. This ticket can be closed.
(You can also mention the bug id in bodhi, and it'll automatically close the bug when the update goes stable)
https://bugzilla.redhat.com/show_bug.cgi?id=1795461
--- Comment #25 from Jiri Hladky hladky.jiri@gmail.com --- Got it. Thank you, Ankur!
package-review@lists.fedoraproject.org
-
bugzilla＠redhat.com