https://bugzilla.redhat.com/show_bug.cgi?id=1650987 --- Comment #4 from Fabio Valentini <decathorpe@xxxxxxxxx> --- Initial comments: 1) Package name Did you consider naming the main package just "genht", and adding a "-libs" sub-package? The upstream project is called "genht", after all. That would also make defining "generic_name" unnecessary, since you could just use "%{name}" everywhere, instead. 2) Build system problems a) The upstream project seems to have some isses with their build system. I don't think it honors build flags from the environment (e.g. CFLAGS, LDFLAGS) - but you didn't set them in the .spec file, either. b) You should use the "%set_build_flags" macro (see [0]) at the top of %build (it sets CFLAGSm LDFLAGS, etc. to the fedora defaults), and change "make" to "%make_build", and "make install" to "%make_install" - those macros set some important flags for fedora builds (including DESTDIR). c) The upstream build system also doesn't define an SONAME for the library ("-Wl,-soname,libfoo.so.1" flag) - that's missing from the upstream Makefile as well, and will throw RPM off - right now, it renders the "-devel" package uninstallable. If upstream won't introduce an SONAME, you will have to add one for the fedora build. d) Don't include the static library in your package (and drop the "-static" sub-package), unless you are sure that other packages will want to use it, and have a good reason to do so. e) The Makefile also defines a binary, is that not used for anything? I'm curious if you could work with the upstream project to add support for a proper build system ("porting" the Makefile to meson could probably be done within an hour, and it would make it more portable than a crusty Makefile). 3) .spec issues You don't need to use macros in summary, description, etc. It just makes the .spec less readable, IMO. But that's a matter of taste, I guess. I also think you can just remove all the comments. They are pretty redundant with the code below them. If you want to keep them, move the ones above %prep and %install into the sections, otherwise RPM will probably treat them as belonging to the previous section. a) By using a glob for the shared library ("%{_libdir}/libgenht.so.*") in line 54, which conceals the library version, you increase the likelihood of unintended breakage because of SONAME bumps. Use "%{_libdir}/libgenht.so.1*") instead, as documented in the Guidelines. b) The summary should not start with "A" and not end with ".". Change it to: "Simple generic hash table implementation in C". c) The "-devel" sub-package must depend on the package containing the shared library (right now, libgenht; if you decide to change the package name to "genht", then "genht-libs") like this: Requires: %{name}%{?_isa} = %{version}-%{release} # for libgenht or this: Requires: %{name}-libs%{?_isa} = %{version}-%{release} # for genht-libs d) What does the comment "# Untar the correct tar.gz file" mean? I think you can just drop it. (The benefit of renaming the main package to "genht" would be that you can drop the "-n" argument from the "%autosetup" call.) e) The following entries in %files lists are equivalent: %dir %{_includedir}/%{generic_name} %{_includedir}/%{generic_name}/* and %{_includedir}/%{generic_name}/ This is probably just a stylistic issue, so you decide which one you'd like to use. [0]: https://src.fedoraproject.org/rpms/redhat-rpm-config/blob/master/f/buildflags.md -- 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://getfedora.org/code-of-conduct.html List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines List Archives: https://lists.fedoraproject.org/archives/list/package-review@xxxxxxxxxxxxxxxxxxxxxxx
