https://bugzilla.redhat.com/show_bug.cgi?id=2493314
--- Comment #18 from Cristian Le <[email protected]> --- Needs some more work, I will focus the comments as a downstream package review, there are only a few notes I can make for handling in upstream that I will comment separately. - Please make a separate git repository from which you submit the review request [1]. You can in principle you can use the copr project, but the version in packit+copr upstream will mangale the version, so doing it from a fresh repository with an empty `fix-spec-file` is needed. - Macro `libxs_build_testing` is not defined. Use `%{with tests}` instead - `%bcond_*` is deprecated because it is super unintuitive. Use `%bcond tests` instead - It does not make much practical difference, but use `-D...:<TYPE>` in the cmake variables when you can. In practice it only affects `PATH` types, but it helps with a readability to know what type it was meant to be when it was first added - Instead of `rm -f %{buildroot}%{_datadir}/%{name}/LICENSE.md` just use that as `%license %{_datadir}/%{name}/LICENSE.md` - It is common to have `soversion` as a macro - `_smp_build_ncpus` is not needed and already covered in the `%ctest` macro via `%{?_smp_mflags}` - `BUILD_SHARED_LIBS` is already covered in `%cmake` - Please keep a 1-2 new line gap between sections [3] (`%prep` -> `%build` does not have one). We have people with strong opinions on both 1 and 2, so it is up to you, as long as you are consistent. - Strongly recommend to use the `%conf` section unless you need rhel10 and less compatibility - Fortran modules must be installed in `%{_fmoddir}` according to the current guidelines [4]. Btw we can adjust the guidelines as needed, I am not a fan of that part either - This project has a good implementation of pkgconfig generation. Nothing to do, just wanted to comment on that :). The only thing that I would change is to add a check for absolute path. I think Qt has one of the best implementation of it so far (not biased at all :D) and can replicate it to catch some of these edge-case handlings - Please patch out the installation of the source into `/usr/include`. Why does upstream need this - Documentation are currently installed in an unstructured directory. `CMAKE_INSTALL_DOCDIR` is a good enough default to install to instead - Open an issue/tracker about opting out of `LIBXS_VERSION_RC` being dynamically created. We do not have the git information during the rpm build. You could use setuptools-scm's `.git_archival.txt` pattern if really needed, but you can also get it from the `%{version}` and manually pass it - Make a `doc` sub-package (noarch-ed) Still missing in the review: - license checks. fedora-review failed on me because of the unusual SPEC/SRPM urls - other fedora-review checklist that I have missed [1]: Example of how I manage https://github.com/LecrisUT/FedoraRPMs/tree/koneko [2]: https://src.fedoraproject.org/rpms/cmake/blob/rawhide/f/macros.cmake.in [3]: Sections are enumerated in https://rpm.org/docs/latest/manual/spec.html#build-scriptlets [4]: https://docs.fedoraproject.org/en-US/packaging-guidelines/Fortran/ -- You are receiving this mail because: You are always notified about changes to this product and component You are on the CC list for the bug. https://bugzilla.redhat.com/show_bug.cgi?id=2493314 Report this comment as SPAM: https://bugzilla.redhat.com/enter_bug.cgi?product=Bugzilla&format=report-spam&short_desc=Report%20of%20Bug%202493314%23c18 -- _______________________________________________ package-review mailing list -- [email protected] To unsubscribe send an email to [email protected] 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/[email protected] Do not reply to spam, report it: https://forge.fedoraproject.org/infra/tickets/issues/new
