Spec URL: https://kalev.fedorapeople.org/d-spy.spec SRPM URL: https://kalev.fedorapeople.org/d-spy-1.2.1-1.fc37.src.rpm Description: D-Spy is a tool to explore and test end-points and interfaces on the System or Session D-Bus. You can also connect to D-Bus peers by address. D-Spy was originally part of GNOME Builder. Fedora Account System Username: kalev Koji scratch build: https://koji.fedoraproject.org/koji/taskinfo?taskID=91152430
Hello, thanks for your review request. I summarized some considerations that I would like to discuss with you. Most of them are not blockers and only require some clarification about the decisions made to package d-spy. Rpmlint warnings detected (after build and install): $ rpmlint -i d-spy //usr/share/appdata/org.gnome.dspy.appdata.xml: OK =================================================================== rpmlint session starts =================================================================== rpmlint: 2.2.0 configuration: /usr/lib/python3.11/site-packages/rpmlint/configdefaults.toml /etc/xdg/rpmlint/fedora.toml /etc/xdg/rpmlint/licenses.toml /etc/xdg/rpmlint/scoring.toml /etc/xdg/rpmlint/users-groups.toml /etc/xdg/rpmlint/warn-on-functions.toml checks: 32, packages: 1 d-spy.x86_64: W: no-manual-page-for-binary d-spy d-spy.x86_64: W: invalid-license GPL-3.0-or-later ==================================== 1 packages and 0 specfiles checked; 0 errors, 2 warnings, 0 badness; has taken 0.7 s ==================================== 1. W: May I ask why are the man pages absent? 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. 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. - 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 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. - 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. - I would use BuildRequires: desktop-file-utils[^2] instead of BuildRequires: /usr/bin/desktop-file-validate - 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. - appdata: - I would use BuildRequires: libappstream-glib[^3] instead of BuildRequires: /usr/bin/appstream-util - 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? Here are all my comments, feel free to disagree and discuss my suggestions. Overall, the package is good. It builds and installs fine! Nicola [^1]: https://docs.fedoraproject.org/en-US/packaging-guidelines/ReviewGuidelines/ [^2]: https://docs.fedoraproject.org/en-US/packaging-guidelines/#_desktop_file_install_usage [^3]: https://docs.fedoraproject.org/en-US/packaging-guidelines/AppData/#_app_data_validate_usage [^4]: https://docs.fedoraproject.org/en-US/packaging-guidelines/#_requiring_base_package
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@lists.fedoraproject.org/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
Spec URL: https://kalev.fedorapeople.org/d-spy.spec SRPM URL: https://kalev.fedorapeople.org/d-spy-1.2.1-2.fc37.src.rpm * Thu Aug 25 2022 Kalev Lember <klember> - 1.2.1-2 - Correct -devel subpackage license to match -libs (rhbz#2120418)
Hi Kalev, thanks for your answer. Here are my thoughts. I mostly agree on everything with you now. Only the BuildRequires bugs me a little. See below. > There are just no man pages upstream :) Someone needs to write them first before we can package them up. Right. > 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@lists.fedoraproject.org/thread/6GKXXE7AMBB76IBRMCTU3L57GLCCZL55/#5M6UBNLEX4LHITP74TAIDAG3DDYJL2HR Didn't know about it. I was just surprised by the warning and wanted to bring this up. I guess licenses are OK then. > 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 I see, that is great. > 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. I see your point, it is my mistake on interpreting the licenses then. > Looks like I've gotten the -devel subpackage license wrong though, let me fix that (it should be the same as -libs). Great, thank you. > 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. OK, I agree on this and just wanted to point out the documentation. I said it is a gray zone specifically for the strong *must* that is written in the packaging guidelines. I was sure that you/upstream had a good answer for it, which, in fact, you had. Thanks for the clarification. > 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 :) I would still prefer not to have /usr/bin in BuildRequires. I counted how many spec files are using the package or /usr/bin. $ grep -r "/usr/bin/desktop-file-validate" | wc -l 38 $ grep -r "desktop-file-utils" | wc -l 2176 $ grep -r "/usr/bin/appstream-util" | wc -l 78 $ grep -r "libappstream-glib" | wc -l 929 Not saying this is an argument on how to do things the right way, but assuming packagers are doing mostly the right thing, the numbers would point me in the direction. > 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. OK, got it. > 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). OK, I got a good explanation about it, looks good then. > 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! That was exactly what I wanted, :) I needed clarification fron you. > Let me just quickly update the -devel subpackage license. Great. > Spec URL: https://kalev.fedorapeople.org/d-spy.spec > SRPM URL: https://kalev.fedorapeople.org/d-spy-1.2.1-2.fc37.src.rpm > > * Thu Aug 25 2022 Kalev Lember <klember> - 1.2.1-2 > - Correct -devel subpackage license to match -libs (rhbz#2120418) I will do a second check of the spec file just to be sure, but otherwise everything LGTM. Please, let me know what do you think of BuildRequires after my comment. I think the request could be approved after we agree on a solution. Thank you once again, Nicola
Anything still blocking the review? As for the /usr/bin/appstream-util and /usr/bin/desktop-file-validate buildrequires, I am still of the mind that it's best to spell out what executable we need as I've done here, but if you consider this a review blocker, I can of course change them.
Hello, I asked other reviewers an opinion, thank you for your patience. > As for the /usr/bin/appstream-util and /usr/bin/desktop-file-validate buildrequires, I am still of the mind that it's best to spell out what executable we need as I've done here, but if you consider this a review blocker, I can of course change them. Not a blocker. Final note. Consider that using the explicit file path, you will have a longer build time because dnf will have to go through the whole database of paths and download additional data to do so. Just be aware that builds will last longer. If you are fine with it, just ping back, and I would pass the review. Thanks for the submission of this package and for your kind patience.
Just note that at least lists of files under /usr/bin /usr/sbin /etc are stored in repodata "primary" xml file as well as rpm requires / provides information, so "additional data download" should not be needed for dnf.
> Just note that at least lists of files under /usr/bin /usr/sbin /etc are stored in repodata "primary" xml file as well as rpm requires / provides information, so "additional data download" should not be needed for dnf. My bad, only lookup time then.
Why do you say that lookup time is a problem? Since /usr/bin /usr/sbin /etc file paths are in the same metadata file as package names they should have exactly the same lookup time. Note that rpmbuild itself generates requires that use /usr/bin and /usr/sbin file paths, e.g. if you look at almost any package, such as 'rpm -q --requires systemd | grep /usr' you'll see that it has requires on /usr/bin/bash, or that 'rpm -q --requires gtk4-devel' lists /usr/bin/pkg-config for example. So if these are problematic in some way we'll have to redo a whole lot of Fedora packaging.
> Why do you say that lookup time is a problem? Since /usr/bin /usr/sbin /etc file paths are in the same metadata file as package names they should have exactly the same lookup time. I am taking a step back. Initially, my thought process implied that to resolve BuildRequires /some/path, dnf would take an additional step to find the package that provides and installs desktop/appdata files validators. You are right, I completely overthought the problem. Now it's clear on my side. Thank you again for the patience and excuse the convolution in the comments.
Excellent, thank you!
(fedscm-admin): The Pagure repository was created at https://src.fedoraproject.org/rpms/d-spy
FEDORA-2022-5b2fc21f46 has been submitted as an update to Fedora 38. https://bodhi.fedoraproject.org/updates/FEDORA-2022-5b2fc21f46
FEDORA-2022-5b2fc21f46 has been pushed to the Fedora 38 stable repository. If problem still persists, please make note of it in this bug report.