https://bugzilla.redhat.com/show_bug.cgi?id=2259250
Bug ID: 2259250 Summary: Review Request: HPCombi - High Performance Combinatorics in C++ using vector instructions Product: Fedora Version: rawhide Hardware: All OS: Linux Status: NEW Component: Package Review Severity: medium Priority: medium Assignee: nobody@fedoraproject.org Reporter: loganjerry@gmail.com QA Contact: extras-qa@fedoraproject.org CC: package-review@lists.fedoraproject.org Target Milestone: --- Classification: Fedora
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.
https://bugzilla.redhat.com/show_bug.cgi?id=2259250
Tom Rix trix@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |trix@redhat.com Status|NEW |ASSIGNED Flags| |fedora-review? Doc Type|--- |If docs needed, set a value
https://bugzilla.redhat.com/show_bug.cgi?id=2259250
--- Comment #1 from Tom Rix trix@redhat.com --- 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
https://bugzilla.redhat.com/show_bug.cgi?id=2259250
--- Comment #2 from Jerry James loganjerry@gmail.com --- 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_sens... 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.
https://bugzilla.redhat.com/show_bug.cgi?id=2259250
--- Comment #3 from Tom Rix trix@redhat.com --- 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
https://bugzilla.redhat.com/show_bug.cgi?id=2259250
Tom Rix trix@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Assignee|nobody@fedoraproject.org |trix@redhat.com
https://bugzilla.redhat.com/show_bug.cgi?id=2259250
--- Comment #4 from Jerry James loganjerry@gmail.com --- (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
https://bugzilla.redhat.com/show_bug.cgi?id=2259250
Jerry James loganjerry@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Flags| |needinfo?(trix@redhat.com)
--- Comment #5 from Jerry James loganjerry@gmail.com --- @trix@redhat.com what else do you need from me to move this review forward?
https://bugzilla.redhat.com/show_bug.cgi?id=2259250
Tom Rix trix@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Flags|needinfo?(trix@redhat.com) |
--- Comment #6 from Tom Rix trix@redhat.com --- sorry for delay, i will take a look now.
https://bugzilla.redhat.com/show_bug.cgi?id=2259250
Tom Rix trix@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |POST Flags|fedora-review? |fedora-review+
--- Comment #7 from Tom Rix trix@redhat.com --- Thanks for the changes. Approved
https://bugzilla.redhat.com/show_bug.cgi?id=2259250
--- Comment #8 from Jerry James loganjerry@gmail.com --- Thank you for the review, Tom. Much appreciated.
https://bugzilla.redhat.com/show_bug.cgi?id=2259250
--- Comment #9 from Fedora Admin user for bugzilla script actions fedora-admin-xmlrpc@fedoraproject.org --- The Pagure repository was created at https://src.fedoraproject.org/rpms/HPCombi
https://bugzilla.redhat.com/show_bug.cgi?id=2259250
Jerry James loganjerry@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Resolution|--- |RAWHIDE Status|POST |CLOSED Fixed In Version| |HPCombi-1.0.1-2.fc41 Last Closed| |2024-03-15 15:18:32
--- Comment #10 from Jerry James loganjerry@gmail.com --- HPCombi has been built in Rawhide.
package-review@lists.fedoraproject.org