Bug 1795461

>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_sections

>Source0:        https://sourceforge.net/projects/pracrand/files/PractRand_%{version}.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

I can review this one.

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,

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

Hello!

Any update on this one?

Hello Jirka,

Any progress here?

Cheers,
Ankur

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

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

Hi Jiri,

Any updates here?


Cheers,
Ankur

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

Hi Jirka,

Hope you are doing well.

Just a general check to see if the release was made so this can proceed?

Cheers,
Ankur

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

Hi Jirka,

Thanks very much! I'll try to complete the review in the coming weeks.

Cheers,

Awesome. I do appreciate that! 

Jirka

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

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

Sounds good:-)

Thank you, Ankur!

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/LicensingGuidelines/#_license_text

- 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

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/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:

 - 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

(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

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

(fedscm-admin):  The Pagure repository was created at https://src.fedoraproject.org/rpms/practrand

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

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)

Got it. Thank you, Ankur!