https://bugzilla.redhat.com/show_bug.cgi?id=1764175 --- Comment #4 from Alejandro Alvarez <a.alvarezayllon@xxxxxxxxx> --- Hello, Thanks for the review. New Spec URL: https://raw.githubusercontent.com/astrorama/copr/13232373c14b71ea405ab6990de9faca38752284/Elements/Elements.spec New SRPM URL: https://kojipkgs.fedoraproject.org//work/tasks/9518/38619518/Elements-5.8-2.fc30.src.rpm New Koji build: https://koji.fedoraproject.org/koji/taskinfo?taskID=38619517 Responses to your comments: >> The upstream tarball checksum does not match the used one I did a local git archive, which indeed does not match the .tar.gz generated by GitHub. Using the GitHub tar.gz directly now. >> This package creates libraries that do not have sonames. Please talk to upstream about that. Patched, and sent a pull request to upstream: https://github.com/degauden/Elements/pull/6 >> Use either %{buildroot} or $RPM_BUILD_ROOT, not both. Fixed since %cleanup has been dropped >> The source files have the "any later version" language, so I believe the license should be LGPLv3+, not LGPLv3. Fixed >> Something is going wrong generating the documentation. Elements' build script tries to download cppreference-doxygen-web.tag.xml and pass it to Doxygen so it can generate links to cppreference.com. I have added that file to the source rpm, as it is CC-BY-SA, which is permitted. >> Perhaps building PDF documentation should be disabled? Building PDF disabled. In any case, latex is used for generating the formulas embeded on the Doxygen documentation, so I have added dependencies on: texlive-latex, texlive-dvips, texlive-newunicodechar. graphviz was also missing. >> Remove all Group tags and the %clean section: https://docs.fedoraproject.org/en-US/packaging-guidelines/#_tags_and_sections Removed >> There is a comment explaining Patch0. Please add a comment for Patch1. Added >> Remove [...] >> Requires: python3 >> Requires(post): /sbin/ldconfig >> Requires(postun): /sbin/ldconfig >> Also remove the %post and %postun scripts. Removed >> In %description, write "A C++ based" instead of "A C++ base". "C++ base" is how upstream defines it: https://github.com/degauden/Elements/blob/develop/CMakeLists.txt#L45 I think they mean it is a framework to be used as a base for C++ projects, not that it is C++-based. Anyway, it can be confusing, so I have replaced it by "C++/Python Build Framework", which they also use. >> Change the Requires in the devel package to these:kin >> Requires: cmake-filesystem >> Requires: %{name}%{?_isa} = %{version}-%{release} Fixed >> The doc subpackage should not have "Requires: %{name}-devel", as you do not need headers installed to read documentation. Removed -doc dependencies >> Consider replacing the body of %prep with "%autosetup -p1 -c %{name}-%{version}". See https://docs.fedoraproject.org/en-US/packaging-guidelines/#_autosetup Replaced >> Remove this line from all %files sections: >> %defattr(-,root,root,-) Removed >> [...] I suggest having a single %changelog entry, with something short like "Initial RPM". I was following the .spec file generated by Elements itself, but fair point. Changed. -- 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
