SPEC URL: https://topazus.fedorapeople.org/rpms/kaidan.spec SRPM URL: https://topazus.fedorapeople.org/rpms/kaidan-0.9.1-1.fc39.src.rpm Description: Kaidan is a simple, user-friendly and modern chat client. It uses the open communication protocol XMPP (Jabber). The user interface makes use of Kirigami and QtQuick, while the back-end of Kaidan is entirely written in C++ using Qt and the Qt-based XMPP library QXmpp. Fedora Account System Username: topazus koji build: https://koji.fedoraproject.org/koji/taskinfo?taskID=102435877 Reproducible: Always
Some early cleanup stuff I found in the spec: First, add the following as a build dependancy: BuildRequires: kf5-rpm-macros Then, replace: %build %cmake \ -GNinja \ -DCMAKE_BUILD_TYPE=Release \ -DUSE_KNOTIFICATIONS=ON \ -DBUILD_TESTS=ON \ %cmake_build With: %build %cmake_kf5 %cmake_build Next, those should be added as requirements. I have a couple packages myself built for KDE and I was instructed to add this for packages using the QML module: # QML module dependencies Requires: kf5-kirigami2%{?_isa} Requires: kf5-kirigami2-addons%{?_isa} Requires: kf5-kquickcharts%{?_isa} Requires: qt5-qtgraphicaleffects%{?_isa} Requires: qt5-qtquickcontrols2%{?_isa} Requires: qt5-qtwebchannel%{?_isa} Requires: qt5-qtwebengine%{?_isa} Next, under files, replace: %{_datadir}/icons/hicolor/*/apps/%{name}.* with: %{_datadir}/icons/hicolor/128x128/apps/kaidan.png %{_datadir}/icons/hicolor/scalable/apps/kaidan.svg Reason: For shared folders, you should not be using wildcards (https://docs.fedoraproject.org/en-US/packaging-guidelines/#_explicit_lists) Lastly, the Source0 should use the KDE invent tags system, specially if Kaidan ever becomes part of the KDE gear. In this case, it would look something like: Source0: https://invent.kde.org/network/%{name}/-/archive/v%{version}/%{name}-v%{version}.tar.gz
Oh this is also probably required, considering qml uses the qtwebengine: %if 0%{?fedora} || 0%{?epel} > 7 # handled by qt5-srpm-macros, which defines %%qt5_qtwebengine_arches # Package doesn't build on arches that qtwebengine is not built on. ExclusiveArch: %{qt5_qtwebengine_arches} %endif
> First, add the following as a build dependancy: > BuildRequires: kf5-rpm-macros > %build > %cmake_kf5 > %cmake_build > Next, those should be added as requirements. I have a couple packages myself built for KDE and I was instructed to add this for packages using the QML module:> > # QML module dependencies > Requires: kf5-kirigami2%{?_isa} > Requires: kf5-kirigami2-addons%{?_isa} > Requires: kf5-kquickcharts%{?_isa} > Requires: qt5-qtgraphicaleffects%{?_isa} > Requires: qt5-qtquickcontrols2%{?_isa} > Requires: qt5-qtwebchannel%{?_isa} > Requires: qt5-qtwebengine%{?_isa} Done. > Next, under files, replace: > %{_datadir}/icons/hicolor/*/apps/%{name}.* > with: > %{_datadir}/icons/hicolor/128x128/apps/kaidan.png > %{_datadir}/icons/hicolor/scalable/apps/kaidan.svg > Reason: For shared folders, you should not be using wildcards (https://docs.fedoraproject.org/en-US/packaging-guidelines/#_explicit_lists) I see the posted link, it seemed it said that it should not use wilcards to include all files of a directory. Anyway, it is a proper suggestion and can explicitly list all files in the directory. > Packagers SHOULD NOT simply glob everything under a shared directory. > In particular, the following SHOULD NOT be used in %files: > %{_bindir}/* > %{_datadir}/* > %{_includedir}/* > %{_mandir}/* > %{_docdir}/* > Lastly, the Source0 should use the KDE invent tags system, specially if Kaidan ever becomes part of the KDE gear. In this > case, it would look something like: > Source0: https://invent.kde.org/network/%{name}/-/archive/v%{version}/%{name}-v%{version}.tar.gz Done. > %if 0%{?fedora} || 0%{?epel} > 7 > # handled by qt5-srpm-macros, which defines %%qt5_qtwebengine_arches > # Package doesn't build on arches that qtwebengine is not built on. > ExclusiveArch: %{qt5_qtwebengine_arches} > %endif Added this.
Thanks for your advice of reviewing this package. SPEC URL: https://topazus.fedorapeople.org/rpms/kaidan.spec SRPM URL: https://topazus.fedorapeople.org/rpms/kaidan-0.9.1-1.fc39.src.rpm
Note: At present, the build of qxmpp package failed on f37, due to the issue of https://bugzilla.redhat.com/show_bug.cgi?id=2216479 Ref: https://src.fedoraproject.org/rpms/qxmpp/c/4302e2d9de0e5aeab8ff40571a8a004919061dbf?branch=rawhide
Copr build: https://copr.fedorainfracloud.org/coprs/build/6145392 (succeeded) Review template: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2216600-kaidan/fedora-rawhide-x86_64/06145392-kaidan/fedora-review/review.txt Please take a look if any issues were found. --- This comment was created by the fedora-review-service https://github.com/FrostyX/fedora-review-service If you want to trigger a new Copr build, add a comment containing new Spec and SRPM URLs or [fedora-review-service-build] string.
A little strange issue is that ctest showed no tests found. Add `-DBUILD_TESTING=ON` to %cmake_kf5 worked. Fix issue of owner of directories. > [ ]: Package must own all directories that it creates. > Note: Directories without known owners: > /usr/share/icons/hicolor/128x128, > /usr/share/icons/hicolor/scalable/apps, /usr/share/icons/hicolor, > /usr/share/icons/hicolor/128x128/apps, > /usr/share/icons/hicolor/scalable Added `Requires: hicolor-icon-theme`. SPEC URL: https://topazus.fedorapeople.org/rpms/kaidan.spec SRPM URL: https://topazus.fedorapeople.org/rpms/kaidan-0.9.1-1.fc39.src.rpm
Created attachment 1974263 [details] The .spec file difference from Copr build 6145392 to 6145636
Copr build: https://copr.fedorainfracloud.org/coprs/build/6145636 (succeeded) Review template: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2216600-kaidan/fedora-rawhide-x86_64/06145636-kaidan/fedora-review/review.txt Please take a look if any issues were found. --- This comment was created by the fedora-review-service https://github.com/FrostyX/fedora-review-service If you want to trigger a new Copr build, add a comment containing new Spec and SRPM URLs or [fedora-review-service-build] string.
Looks good to me. I'll approve this.
The Pagure repository was created at https://src.fedoraproject.org/rpms/kaidan
FEDORA-2023-78076de24d has been submitted as an update to Fedora 39. https://bodhi.fedoraproject.org/updates/FEDORA-2023-78076de24d
FEDORA-2023-78076de24d has been pushed to the Fedora 39 stable repository. If problem still persists, please make note of it in this bug report.
Steve Cossette asked me to double-check the review. ---- The suggestions offered in the review process looked reasonable. ---- Regarding remaining rpmlint messages: > kaidan.x86_64: W: no-manual-page-for-binary kaidan Man pages are desired in general (https://docs.fedoraproject.org/en-US/packaging-guidelines/#_manpages) but this is a SHOULD rather than a MUST. It’s worth pointing this out in a review, but it shouldn’t be considered a blocker. It’s often difficult to convince upstreams of the value of maintaining man pages. I find that most upstreams won’t consider it unless the man pages can be automatically generated from some existing source. > kaidan.x86_64: W: files-duplicate /usr/share/kaidan/images/kaidan.svg /usr/share/icons/hicolor/scalable/apps/kaidan.svg This is fine. These files could probably safely be hardlinked without the risk of cross-filesystem hardlinks, since both are under /usr/share/ (but rpmlint would then complain about cross-directory hardlinks possibly being on different filesystems), or they could be explicitly symlinked, but duplicating a single 4 kiB file is just not a big deal. I would probably not recommend any change to the packaging. ---- The fedora-review tool suggests that large data in /usr/share should live in a noarch subpackage if package is arched. This allows the noarch subpackage to be shared across architectures, and reduces duplicated data on the mirrors. Whether ~1MiB is large enough to worry about is a matter of opinion. It would be reasonable to consider moving the data into a noarch -data subpackage, but I don’t think the data files are large enough to say this *needs* to happen. ---- I think the License should include an “AND LGPL-2.0-or-later” term for src/qml/elements/IconTopButton.qml. ---- The files src/hsluv-c/hsluv.h and src/hsluv-c/hsluv.c are a bundled copy of http://github.com/hsluv/hsluv-c. This is designed as a ”copylib” and doesn’t have an upstream build system for a separate library, so it’s unlikely it can be separately packaged for Fedora. However, it still needs to be handled in accordance with https://docs.fedoraproject.org/en-US/packaging-guidelines/#bundling. At minimum, the appropriate virtual Provides should be added. ---- The sole source (Source0:) doesn’t need to be numbered; you can just write ”Source:”. Numbering it doesn’t hurt anything, though. ---- As a reviewer, it’s a good practice to post the filled-in review checklist template from the fedora-review output. This shows you’ve considered all of the things that it can’t check automatically, and is generally useful when people need to go back and check old reviews. ---- Overall, this looks like a clean package and a high-quality review.
Thanks for the checking. > I think the License should include an “AND LGPL-2.0-or-later” term for src/qml/elements/IconTopButton.qml. Could you explain why? I see the file src/qml/elements/IconTopButton.qml is licensed under GPL-3.0-or-later. https://invent.kde.org/network/kaidan/-/blob/master/src/qml/elements/IconTopButton.qml > The files src/hsluv-c/hsluv.h and src/hsluv-c/hsluv.c are a bundled copy of http://github.com/hsluv/hsluv-c. This is designed as a ”copylib” and doesn’t have an upstream build system for a separate library, so it’s unlikely it can be separately packaged for Fedora. However, it still needs to be handled in accordance with https://docs.fedoraproject.org/en-US/packaging-guidelines/#bundling. At minimum, the appropriate virtual Provides should be added. I will add the hsluv-c as the bundled library. Also, I noticed that the source code files at src/singleapp directory are the bundled library of SingleApplication, https://github.com/itay-grudev/SingleApplication. I will add this too.
(In reply to Felix Wang from comment #15) > Thanks for the checking. > > > I think the License should include an “AND LGPL-2.0-or-later” term for src/qml/elements/IconTopButton.qml. > > Could you explain why? I see the file src/qml/elements/IconTopButton.qml is > licensed under GPL-3.0-or-later. > > https://invent.kde.org/network/kaidan/-/blob/master/src/qml/elements/ > IconTopButton.qml This file was relicensed upstream in a giant “Reuseify QML files” commit, which hasn’t yet appeared in a release: https://invent.kde.org/network/kaidan/-/commit/14c712eb72e7094d22f9faeec8a8a86effe72ade?page=2#c88f477dc2798d0fbde587cea39b54208314e23b In the packaged release, this file is still LGPL-2.0-or-later: /* * SPDX-FileCopyrightText: 2022 Nate Graham <nate> * * SPDX-License-Identifier: LGPL-2.0-or-later */
(In reply to Felix Wang from comment #15) > I will add the hsluv-c as the bundled library. Also, I noticed that the > source code files at src/singleapp directory are the bundled library of > SingleApplication, https://github.com/itay-grudev/SingleApplication. I will > add this too. Thanks! Nice catch!
Thanks for that Music! I'll keep all of this in mind for my future reviews! Thank you for your help!
(In reply to Ben Beasley from comment #16) > (In reply to Felix Wang from comment #15) > > Thanks for the checking. > > > > > I think the License should include an “AND LGPL-2.0-or-later” term for src/qml/elements/IconTopButton.qml. > > > > Could you explain why? I see the file src/qml/elements/IconTopButton.qml is > > licensed under GPL-3.0-or-later. > > > > https://invent.kde.org/network/kaidan/-/blob/master/src/qml/elements/ > > IconTopButton.qml > > This file was relicensed upstream in a giant “Reuseify QML files” commit, > which hasn’t yet appeared in a release: > > https://invent.kde.org/network/kaidan/-/commit/ > 14c712eb72e7094d22f9faeec8a8a86effe72ade?page=2#c88f477dc2798d0fbde587cea39b5 > 4208314e23b > > In the packaged release, this file is still LGPL-2.0-or-later: > > /* > * SPDX-FileCopyrightText: 2022 Nate Graham <nate> > * > * SPDX-License-Identifier: LGPL-2.0-or-later > */ Many thanks for your clarification about the issues of licenses and bundled libraries. I have adopted your suggestions. https://src.fedoraproject.org/rpms/kaidan/c/4f7ef22f3519e7f340e93bdc6602b49450689c59?branch=rawhide