https://bugzilla.redhat.com/show_bug.cgi?id=2120418 --- Comment #2 from Kalev Lember <klember@xxxxxxxxxx> --- Hi Nicola, Thanks for reviewing this package! Let me try to answer all the questions below. > 1. W: May I ask why are the man pages absent? There are just no man pages upstream :) Someone needs to write them first before we can package them up. > 2. W: About licenses. SPDX Expressions are fine, but rpmlint is complaining > with a warning. I suggest correcting the license with GPLv3+, and, > eventually, > LGPLv2+ and LGPLv3+, just for the sake of clearing warnings. Looks like rpmlint needs updating here, because we have new guidelines in Fedora that went live a few weeks ago that mandate the use of SPDX license IDs for new packages. See https://lists.fedoraproject.org/archives/list/devel@xxxxxxxxxxxxxxxxxxxxxxx/thread/6GKXXE7AMBB76IBRMCTU3L57GLCCZL55/#5M6UBNLEX4LHITP74TAIDAG3DDYJL2HR > Other considerations: > > - Why is the pkgname `d-spy` and downloadURL/tarball/GNOME name is `dspy`? > This is NOT a blocker, but I noticed what could be a small inconsistency. I had the same question which name to prefer, so I reached out to the upstream author and discussed this before submitting the package and he suggested d-spy. In fact, in response to this the upstream tarball (or rather, the next upcoming tarball) has been renamed to d-spy as well, see https://gitlab.gnome.org/GNOME/d-spy/-/commit/b0765bc7557ffbbf0bf9b6741b393adc2778cb06 > - Regarding d-spy-libs: it is my preference to include all license files > under %license. In your scenario, I would consider doing: > %license COPYING.lgplv2 COPYING.lgplv3 No, I don't think this is right. Upstream is licensing the shared library and the app differently, so that the app is under the GPL license (%license COPYING is included in the subpackage that ships the main app), but the shared library is under LGPL (%license COPYING.lgplv3 for the -libs subpackage). It's deliberately different licenses for different subpackages and it's split along the licensing split so that each subpackage has a different license. Looks like I've gotten the -devel subpackage license wrong though, let me fix that (it should be the same as -libs). > A second option, would be putting all license files in a separate dir > (LICENSE) and simply specify the dir with: > %license LICENSE > Again, NOT a blocker. Just my preference. Right, that would make sense if there are many licenses that are applicable to one package, but in this case it's one license for one subpackage and another one for the other one. > - desktop file: > - Quoting[^1]: "Packages containing GUI applications must include a > %{name}.desktop file". This is a gray zone, but I believe it should be > fixed by changing the desktop file name to d-spy.desktop. No, first this is upstream decision how to name it. I don't think we as a downstream should start changing the desktop or appstream IDs. Second, since flatpak became a thing the app IDs have been slowly migrating to reverse dns notation, which is now the preferred naming. See e.g. https://www.freedesktop.org/software/appstream/docs/chap-Metadata.html#tag-id-generic > - I would use BuildRequires: desktop-file-utils[^2] instead of > BuildRequires: /usr/bin/desktop-file-validate I find it clearer when we require the specific tool that we use in the spec file, which makes it self-documenting. Otherwise we'd have to have something like: # for desktop-file-validate in %check BuildRequires: desktop-file-utils But I agree that writing /usr/bin in there is a bit hairy and it's bothering me as well. So I'm not super convinced which way to prefer :) > - May I ask why the installation part of the desktop file > (desktop-file-install) is skipped? Is it included in the installation > script? In that case, that's fine. desktop-file-install is a tool that both installs and validates and allows editing a desktop file when installing it. In this case, it's upstream build system that installs it so we only need to validate it. > - appdata: > - I would use BuildRequires: libappstream-glib[^3] instead of > BuildRequires: /usr/bin/appstream-util See above. > - Usually, subpackages other than devel should require the base package > using a > fully versioned dependency.[^4] This is not the case for all packages, I > just > wanted to point this out to be sure it is not a mistake. Is it intentional? In this case the packaging is a bit more complicated because both the GUI app and shared libraries come from the same source package. The GUI app is in the d-spy binary package and the libraries are in the d-spy-libs binary package. When the packaging guidelines say that -devel should require the base package using a fully versioned dependency, what they really mean is that the -devel package can't solely rely on automatic soname deps to pull in the base library package, but instead it needs to tighten the deps by using a fully versioned dependency. So in this case, -devel provides headers and other bits that are needed for linking with the libraries in -libs, so we need to make sure that -devel has a fully versioned dependency on -libs (which is does). > Here are all my comments, feel free to disagree and discuss my suggestions. > Overall, the package is good. It builds and installs fine! I managed to disagree with almost all the suggestions but I really appreciate the time you took to carefully think about this package! Thank you! Let me just quickly update the -devel subpackage license. Kalev -- 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 https://bugzilla.redhat.com/show_bug.cgi?id=2120418 _______________________________________________ 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 Do not reply to spam, report it: https://pagure.io/fedora-infrastructure/new_issue
