https://bugzilla.redhat.com/show_bug.cgi?id=1780885
Bug ID: 1780885 Summary: Review Request: cutelyst - C++ Qt-based Web Framework Product: Fedora Version: rawhide Hardware: All OS: Linux Status: NEW Component: Package Review Severity: medium Assignee: nobody@fedoraproject.org Reporter: raineforest@raineforest.me QA Contact: extras-qa@fedoraproject.org CC: package-review@lists.fedoraproject.org Target Milestone: --- Classification: Fedora
Spec URL: https://copr-be.cloud.fedoraproject.org/results/raineforest/cutelyst/fedora-... SRPM URL: https://copr-be.cloud.fedoraproject.org/results/raineforest/cutelyst/fedora-...
Description: Cutelyst CLI tool providing support for developing cutelyst applications.
Fedora Account System Username: raineforest
https://bugzilla.redhat.com/show_bug.cgi?id=1780885
Dakota Williams raineforest@raineforest.me changed:
What |Removed |Added ---------------------------------------------------------------------------- Depends On| |1780882
Referenced Bugs:
https://bugzilla.redhat.com/show_bug.cgi?id=1780882 [Bug 1780882] Review Request: simple-mail - SMTP library for Qt
https://bugzilla.redhat.com/show_bug.cgi?id=1780885
Kevin Kofler kevin@tigcc.ticalc.org changed:
What |Removed |Added ---------------------------------------------------------------------------- Doc Type|--- |If docs needed, set a value
--- Comment #1 from Kevin Kofler kevin@tigcc.ticalc.org --- (Not a formal review, just an observation:)
Your subpackage naming looks very much OpenSUSE-, Mandriva/Mageia-, or Debian-style to me. In Fedora, we do not usually name library packages with lib*%{_sonum}. We use lib* naming only where it is part of the upstream name, and we only use version suffixes for non-default compatibility versions (and we use the human-readable version number rather than the soversion, though in this case the soversion 2 matches the human-readable major version).
More idiomatic Fedora subpackage naming would be something like: %package plugin-authentication (i.e., a cutelyst-plugin-authentication subpackage) etc.
It is also not clear to me whether we really need all those subpackages, but maybe we do. That is up to you to decide.
https://bugzilla.redhat.com/show_bug.cgi?id=1780885
--- Comment #2 from Kevin Kofler kevin@tigcc.ticalc.org --- (There is no requirement in Fedora to create a separate subpackage for every .so file, though it may make sense to do it anyway, depending on how likely applications are to cherry-pick only specific plugins and on how many dependencies those plugins have.)
https://bugzilla.redhat.com/show_bug.cgi?id=1780885 Bug 1780885 depends on bug 1780882, which changed state.
Bug 1780882 Summary: Review Request: simple-mail - SMTP library for Qt https://bugzilla.redhat.com/show_bug.cgi?id=1780882
What |Removed |Added ---------------------------------------------------------------------------- Status|MODIFIED |CLOSED Resolution|--- |ERRATA
https://bugzilla.redhat.com/show_bug.cgi?id=1780885
--- Comment #3 from Michael Schwendt bugs.michael@gmx.net --- FWIW, I consider the package design highly problematic. Both at the spec level and for the package users.
The many libCutelyst2* subpackages packages don't contain plugins, but [!] system libraries. To make it worse, there are many automatic inter-dependencies in those subpackages, so the benefit of putting each lib into its own subpkg is minuscule or non-existant. Even the base program pulls in one of the subpackages, which in turn pulls in more subpackages. The -devel package pulls in all lib subpkgs. Over time, dependencies will change, libs will come and go, and the packaging maintenance requirements will be high. Also with regard to handling/removing obsolete subpackages.
The spec file adds dependency bloat, which isn't arch-specific and therefore doesn't add much value:
Requires: %{_pluginSession}%{_sonum} = %{version}-%{release}
--> libCutelyst2Qt5Session.so.2()(64bit) libCutelyst2Qt5Session2 = 2.9.0-1.fc31
The first dep is automatic and arch-specific. The explicit dep from the spec file adds V-R but doesn't follow the base package guidelines and would be satisfied by the .i686 package due to multiarch repositories. The base package guidelines (for %_isa usage) should also be followed in other explicit "Requires:" tags that add deps on arch-specific packages, such as "uwsgi".
Qt translation files would be found with %find_lang --with-qt --all-name
The packaging should be simplified *a lot*.
https://bugzilla.redhat.com/show_bug.cgi?id=1780885
Michel Alexandre Salim michel@michel-slm.name changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |ASSIGNED CC| |michel@michel-slm.name Flags| |fedora-review? | |needinfo?(raineforest@raine | |forest.me)
--- Comment #4 from Michel Alexandre Salim michel@michel-slm.name --- Taking this review.
This doesn't currently build as is, could you follow the steps here to adjust for Fedora 33's recent CMake changes? https://fedoraproject.org/wiki/Changes/CMake_to_do_out-of-source_builds
Also bump the version packaged, since 2.13 is now out.
Agreed with previous commenters that: - %find_lang will find the translation files - try starting with only either two packages for now (cutelyst and cutelyst-devel), or three (if the binaries are not needed by running apps and can be packaged separately). Those will be easier to maintain - see https://docs.fedoraproject.org/en-US/packaging-guidelines/#_requiring_base_p... for having cutelyst-devel requires the right architecture for cutelyst (what Michael Schwendt was referring to)
You enabled building tests, but they were never run. Add
%check %ctest
after the %install section, this should run them.
package-review@lists.fedoraproject.org