https://bugzilla.redhat.com/show_bug.cgi?id=2279008 --- Comment #5 from Dominik Wombacher <dominik@xxxxxxxxxxxx> --- Thanks your valuable and comprehensive review. I really appreciate it and that helped me a lot! > If s2n-tls-cmake.patch is suitable for offering upstream, please do so. Otherwise, please add a comment briefly explaining what the patch does and why it makes sense for it to be downstream-only. Not suitable for upstream, comment added. > The organization into a libs subpackage and a base package with only documentation and license files and a libs package, with a circular dependency between the two, is workable, but unusual and probably more complicated than necessary considering that there are no command-line tools to package in the base package. I think that it would make sense to either: > 1. put the shared libraries %{_libdir}/libs2n.so.1{,.*} in the base s2n-tls > package, and let the -devel package depend on that – the simplest, most > “normal” approach – or, > 2. put the license and small doc files README.md in VERSIONING.rst in the > -libs subpackage, and let the base package have no %files section at > all, so that no s2n-tls binary package is built Sounds reasonable and indeed I seem to "over engineer" a bit here. Let me try option 1 :) > This is correct, and there is nothing wrong with it: > > %dir %{_libdir}/cmake/s2n > %dir %{_libdir}/cmake/s2n/modules > %dir %{_libdir}/cmake/s2n/shared > %{_libdir}/cmake/s2n/*.cmake > %{_libdir}/cmake/s2n/modules/*.cmake > %{_libdir}/cmake/s2n/shared/*.cmake > > However, you may find it easier to just name the directory and all of its > contents: > > %{_libdir}/cmake/s2n/ > > The trailing slash is not required but is good style to make it clear that a > directory is meant (and it does keep it from matching a file with the same > name). OK that saves a lot of writing and adjusting when files are added in newer releases, thank you! > The package creates, but does not own, /usr/share/doc/s2n-tls; the -doc > package needs to own that: > > %dir %{_docdir}/s2n-tls/ done > Version 1.4.14 is now available. Please update. It doesn’t look like any > packaging changes will be required. done > It is unnecessary (albeit permissible) to number the sources and patches in Fedora. Good to know. I'm a bit of a fan of it when there are multiple sources/patches but as long it's just one I'm going to adjust it. > The runtime dependency on openssl-libs is already handled by the RPM dependency generators Understood, lesson learned, only build requirements have to be specific, runtime is automatically handled. Adjusted. > The dependency on openssl-devel from s2n-tls-devel should be arch-specific. 100% agree, thanks, changed. > You can, if you choose, avoid repetition by writing > > Summary: %{summary} > > in each of the subpackages. Similarly, in the base package, you can write > > %global _description %{expand: > s2n-tls is a C99 implementation of the TLS/SSL protocols that is > designed to be simple, small, fast, and with security as a priority.} > > %description %{_description} > > and then > > %description doc %{_description} > > and so on. Like that, makes it indeed much easier and less repetitive, done. > The package fails to build on i686: OK that's new, let me drop it then, I don't think it's worth the effort or anything upstream will interested to fix. > The package also fails to build on s390x due to several test failures: s390x is know, statement from upstream (related package aws-c-common) is that they not support the platform. I forgot to update this spec. Adjusted. > It looks like the -doc subpackage contains only arch-independent files. You should make the subpackage noarch Done. Spec URL: https://download.copr.fedorainfracloud.org/results/wombelix/aws-c-libs/fedora-rawhide-x86_64/07446364-s2n-tls/s2n-tls.spec SRPM URL: https://download.copr.fedorainfracloud.org/results/wombelix/aws-c-libs/fedora-rawhide-x86_64/07446364-s2n-tls/s2n-tls-1.4.14-1.fc41.src.rpm Last successful build in my copr project: https://copr.fedorainfracloud.org/coprs/wombelix/aws-c-libs/build/7446364/ -- 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 https://bugzilla.redhat.com/show_bug.cgi?id=2279008 Report this comment as SPAM: https://bugzilla.redhat.com/enter_bug.cgi?product=Bugzilla&format=report-spam&short_desc=Report%20of%20Bug%202279008%23c5 -- _______________________________________________ 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 Do not reply to spam, report it: https://pagure.io/fedora-infrastructure/new_issue
