Bug 1780885 - Review Request: cutelyst - C++ Qt-based Web Framework
Summary: Review Request: cutelyst - C++ Qt-based Web Framework
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
Target Milestone: ---
Assignee: Michel Lind
QA Contact: Fedora Extras Quality Assurance
Depends On: 1780882
TreeView+ depends on / blocked
Reported: 2019-12-08 08:05 UTC by Dakota Williams
Modified: 2020-12-03 00:45 UTC (History)
4 users (show)

Fixed In Version:
Doc Type: If docs needed, set a value
Doc Text:
Clone Of:
Last Closed: 2020-12-03 00:45:18 UTC
Type: ---

Attachments (Terms of Use)

Description Dakota Williams 2019-12-08 08:05:05 UTC
Spec URL: https://copr-be.cloud.fedoraproject.org/results/raineforest/cutelyst/fedora-rawhide-x86_64/01126060-cutelyst/cutelyst.spec
SRPM URL: https://copr-be.cloud.fedoraproject.org/results/raineforest/cutelyst/fedora-rawhide-x86_64/01126060-cutelyst/cutelyst-2.9.0-1.fc32.src.rpm

Cutelyst CLI tool providing support for developing cutelyst applications.

Fedora Account System Username: raineforest

Comment 1 Kevin Kofler 2019-12-08 22:55:44 UTC
(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.

Comment 2 Kevin Kofler 2019-12-08 22:59:26 UTC
(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.)

Comment 3 Michael Schwendt 2019-12-31 19:06:11 UTC
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}

  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*.

Comment 4 Michel Lind 2020-11-02 23:45:05 UTC
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?

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_package 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


after the %install section, this should run them.

Comment 5 Package Review 2020-12-03 00:45:18 UTC
This is an automatic action taken by review-stats script.

The ticket submitter failed to clear the NEEDINFO flag in a month.
As per https://fedoraproject.org/wiki/Policy_for_stalled_package_reviews
we consider this ticket as DEADREVIEW and proceed to close it.

Note You need to log in before you can comment on or make changes to this bug.