Spec URL: https://jjames.fedorapeople.org/HPCombi/HPCombi.spec SRPM URL: https://jjames.fedorapeople.org/HPCombi/HPCombi-1.0.0-1.fc40.src.rpm Fedora Account System Username: jjames Description: HPCombi is a C++17 header-only library using the SSE and AVX instruction sets, and some equivalents, for very fast manipulation of combinatorial objects such as transformations, permutations, and boolean matrices of small size. The goal of this project is to implement various new algorithms and benchmark them on various compiler and architectures. HPCombi was initially designed using the SSE and AVX instruction sets, and did not work on machines without these instructions (such as ARM). From v1.0.0 HPCombi supports processors with other instruction sets also, via SIMD Everywhere. It might be the case that the greatest performance gains are achieved on processors supporting the SSE and AVX instruction sets, but the HPCombi benchmarks indicate that there are also still significant gains on other processors too. This package is needed to boost performance of the libsemigroups package.
lowercase the spec file and %name. review %license, from license check and LICENSE file it is GPL 3 can you explain # Install documentation with the devel package documentation %global _docdir_fmt %{name}-devel I am not sure what you are trying to do here. %files doc %doc %{_vpath_builddir}/doc/html can you cp/install this to buildroot in the %install can you check on %check, i get this in my local fedora-review + cd HPCombi-1.0.0 + /usr/bin/ctest --test-dir redhat-linux-build --output-on-failure --force-new-ctest-process -j112 Internal ctest changing into directory: /builddir/build/BUILD/HPCombi-1.0.0/redhat-linux-build Test project /builddir/build/BUILD/HPCombi-1.0.0/redhat-linux-build No tests were found!!! + RPM_EC=0
Thank you for the review, Tom! Let me know if there is something I can review for you. (In reply to Tom Rix from comment #1) > lowercase the spec file and %name. I am going to push back on this. https://docs.fedoraproject.org/en-US/packaging-guidelines/Naming/#_case_sensitivity says that we should respect upstream naming. I see it spelled with uppercase, HPCombi, everywhere in the distribution. The primary consumer of this package, libsemigroups, also does so; see https://github.com/libsemigroups/libsemigroups. > review %license, from license check and LICENSE file it is GPL 3 Yes, the LICENSE file contains the text of GPL version 3, but the actual version intended by upstream is not stated anywhere. I have asked upstream for clarification: https://github.com/libsemigroups/HPCombi/issues/36. > can you explain > # Install documentation with the devel package documentation > > %global _docdir_fmt %{name}-devel > > I am not sure what you are trying to do here. That makes the API documentation in HPCombi-doc appear in /usr/share/doc/HPCombi-devel, alongside the documentation files installed by HPCombi-devel. Without that, the API documentation would be in a different directory, /usr/share/doc/HPCombi-doc. > %files doc > > %doc %{_vpath_builddir}/doc/html > > can you cp/install this to buildroot in the %install I can, but why? This is very short and simple. > can you check on %check, i get this in my local fedora-review > + cd HPCombi-1.0.0 > > + /usr/bin/ctest --test-dir redhat-linux-build --output-on-failure > --force-new-ctest-process -j112 > Internal ctest changing into directory: > /builddir/build/BUILD/HPCombi-1.0.0/redhat-linux-build > > Test project /builddir/build/BUILD/HPCombi-1.0.0/redhat-linux-build > > No tests were found!!! > > + RPM_EC=0 Yes, that is correct. Upstream has provided no tests. I put the %check section in the spec file in case a future update adds tests and I fail to notice. If you prefer, I can comment it out for now.
Lets wait on the license feedback. IMO the github page says GPL 3. The text in the code is not great, but it doesn't say 1 and later. I have very very rarely seen gpl1. if there are no ctests dont use them, just remove %check. for the %name.. ok i guess. for the %files list, i like to see it to be boring and reflect what is in the buildroot. I have this shiny new package review to get automatic speech recognition going https://bugzilla.redhat.com/show_bug.cgi?id=2259336
(In reply to Tom Rix from comment #3) > Lets wait on the license feedback. > IMO the github page says GPL 3. > The text in the code is not great, but it doesn't say 1 and later. I have > very very rarely seen gpl1. Upstream has released version 1.0.1 with improved license information. It is GPL-3.0-or-later. The old licensing docs used to say that if you can't find a clear statement about which version of the GPL is intended, then any version of the GPL can be used; i.e., the license defaults to GPL-1.0-or-later. I can't find that in the new licensing docs, so maybe that is not the case anymore. > if there are no ctests dont use them, just remove %check. I commented it out for now so that I remember to look later to see if any tests have been added. > for the %files list, i like to see it to be boring and reflect what is in > the buildroot. I don't think I understand what it is you want me to do, or what you see as a deficiency in the way I have it now. Can you explain a little more, please? > I have this shiny new package review to get automatic speech recognition > going > https://bugzilla.redhat.com/show_bug.cgi?id=2259336 Okay, I will review it. New URLs for version 1.0.1: Spec URL: https://jjames.fedorapeople.org/HPCombi/HPCombi.spec SRPM URL: https://jjames.fedorapeople.org/HPCombi/HPCombi-1.0.1-1.fc40.src.rpm
@trix what else do you need from me to move this review forward?
sorry for delay, i will take a look now.
Thanks for the changes. Approved
Thank you for the review, Tom. Much appreciated.
The Pagure repository was created at https://src.fedoraproject.org/rpms/HPCombi
HPCombi has been built in Rawhide.