Bug 1780885 - Review Request: cutelyst - C++ Qt-based Web Framework
Summary: Review Request: cutelyst - C++ Qt-based Web Framework
Keywords:
Status: CLOSED NOTABUG
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
unspecified
medium
Target Milestone: ---
Assignee: Michel Lind
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On: 1780882
Blocks: FE-DEADREVIEW
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:
Environment:
Last Closed: 2020-12-03 00:45:18 UTC
Type: ---
Embargoed:


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

Description:
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}

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

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?
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_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

%check
%ctest

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.