https://bugzilla.redhat.com/show_bug.cgi?id=1753837 --- Comment #4 from Jerry James <loganjerry@xxxxxxxxx> --- Thank you for the review, Michal! I did indeed forget to mark this review as needing the libdivide review done first, so thank you for taking care of that. (In reply to Michal Schorm from comment #3) > 1) The require on line 20 isn't IMHO needed at all. > If the binary links with the library, the RPM build will automatically > generate dependency to that library. You are correct that the library Requires is automatically generated. However, my reading of https://docs.fedoraproject.org/en-US/packaging-guidelines/#_requiring_base_package is that when there are dependencies between subpackages and the main package, they must be fully specified with %{version}-%{release}. > 2) It's better to use the %{set_build_flags}, instead of just %{optflags}. > As you can see e.g. here: > https://src.fedoraproject.org/rpms/mariadb-connector-c/blob/master/f/mariadb- > connector-c.spec#_78 I don't see how that can be used in this case. The whole point is to add -DLIBDIVIDE_SSE2 to the build flags when an x86 architecture is in use. Otherwise, we let the %cmake macro set the build flags. Am I missing something? > 3) Personally, I prefer to write short patch justification as a comment in > the SPECfile and a verbose justification at the beginning of the patch > (file). That's good practice. I have added justifications to the beginning of the patch files. > 4) You can also fix some of the RPMLint errors by whitelisting them with a > justification. > Take a glimpse at my package, how it looks: > https://src.fedoraproject.org/rpms/mariadb-connector-c/blob/master/f/mariadb- > connector-c.rpmlintrc Thank you for the example. I've done something similar. > 5) I haven't tested it builds and works on all supported architectures, > since the requirement is not yet in Fedora. > I marked this BZ as "Depends On: 1753084" Right, no scratch build is possible unless libdivide is bundled. New URLs: Spec URL: https://jjames.fedorapeople.org/primecount/primecount.spec SRPM URL: https://jjames.fedorapeople.org/primecount/primecount-5.1-2.fc32.src.rpm RPMLINTRC URL: https://jjames.fedorapeople.org/primecount/primecount.rpmlintrc -- 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
