https://bugzilla.redhat.com/show_bug.cgi?id=2025124
Bug ID: 2025124 Summary: Review Request: <main package name here> - <short summary here> Product: Fedora Version: rawhide Hardware: All OS: Linux Status: NEW Component: Package Review Severity: medium Priority: medium Assignee: nobody@fedoraproject.org Reporter: jkadlcik@redhat.com QA Contact: extras-qa@fedoraproject.org CC: package-review@lists.fedoraproject.org Target Milestone: --- Classification: Fedora
Spec URL: https://download.copr.fedorainfracloud.org/results/frostyx/qtile/fedora-rawh... SRPM URL: https://download.copr.fedorainfracloud.org/results/frostyx/qtile/fedora-rawh... Description: A pure-Python tiling window manager.
Features ========
* Simple, small and extensible. It's easy to write your own layouts, widgets and commands. * Configured in Python. * Command shell that allows all aspects of Qtile to be managed and inspected. * Complete remote scriptability - write scripts to set up workspaces, manipulate windows, update status bar widgets and more. * Qtile's remote scriptability makes it one of the most thoroughly unit-tested window mangers around.
Fedora Account System Username: frostyx
https://bugzilla.redhat.com/show_bug.cgi?id=2025124
--- Comment #1 from Jakub Kadlčík jkadlcik@redhat.com --- Qtile was in Fedora from F22 up to F34 https://src.fedoraproject.org/rpms/qtile
Unfortunately, it was orphaned and needs to go through a package review before I can continue maintaining it. I took the specfile from F34, polished it a little, and updated it to work with the most recent Qtile version.
https://bugzilla.redhat.com/show_bug.cgi?id=2025124
--- Comment #2 from Jakub Kadlčík jkadlcik@redhat.com --- FTR: Releng ticket to unretire the qtile package https://pagure.io/releng/issue/10391
https://bugzilla.redhat.com/show_bug.cgi?id=2025124
Jakub Kadlčík jkadlcik@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Summary|Review Request: <main |Review Request: qtile - A |package name here> - <short |pure-Python tiling window |summary here> |manager
https://bugzilla.redhat.com/show_bug.cgi?id=2025124
Jhordy Caceres jhordy.caceres@outlook.com changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |jhordy.caceres@outlook.com
--- Comment #3 from Jhordy Caceres jhordy.caceres@outlook.com --- *** Bug 2027073 has been marked as a duplicate of this bug. ***
https://bugzilla.redhat.com/show_bug.cgi?id=2025124
Vanessa Christopher vanessaigwe1@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |vanessaigwe1@gmail.com Blocks| |1276941 (fedora-neuro)
Referenced Bugs:
https://bugzilla.redhat.com/show_bug.cgi?id=1276941 [Bug 1276941] Fedora NeuroImaging and NeuroScience tracking bug
https://bugzilla.redhat.com/show_bug.cgi?id=2025124
Vanessa Christopher vanessaigwe1@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Blocks|1276941 (fedora-neuro) |
Referenced Bugs:
https://bugzilla.redhat.com/show_bug.cgi?id=1276941 [Bug 1276941] Fedora NeuroImaging and NeuroScience tracking bug
https://bugzilla.redhat.com/show_bug.cgi?id=2025124
--- Comment #4 from Jakub Kadlčík jkadlcik@redhat.com --- Meanwhile, a new upstream version was released.
Spec URL: https://download.copr.fedorainfracloud.org/results/frostyx/qtile/fedora-rawh... SRPM URL: https://download.copr.fedorainfracloud.org/results/frostyx/qtile/fedora-rawh...
https://bugzilla.redhat.com/show_bug.cgi?id=2025124
--- Comment #5 from Jakub Kadlčík jkadlcik@redhat.com --- Yet another upstream version was released.
Spec URL: https://download.copr.fedorainfracloud.org/results/frostyx/qtile/fedora-rawh... SRPM URL: https://download.copr.fedorainfracloud.org/results/frostyx/qtile/fedora-rawh...
https://bugzilla.redhat.com/show_bug.cgi?id=2025124
--- Comment #6 from Jakub Kadlčík jkadlcik@redhat.com --- I bumped a release and added Recommends: for packages, that are needed by widgets.
Spec URL: https://download.copr.fedorainfracloud.org/results/frostyx/qtile/fedora-rawh... SRPM URL: https://download.copr.fedorainfracloud.org/results/frostyx/qtile/fedora-rawh...
https://bugzilla.redhat.com/show_bug.cgi?id=2025124
--- Comment #7 from Jakub Kadlčík jkadlcik@redhat.com --- Another upstream release
Spec URL: https://download.copr.fedorainfracloud.org/results/frostyx/qtile/fedora-rawh... SRPM URL: https://download.copr.fedorainfracloud.org/results/frostyx/qtile/fedora-rawh...
https://bugzilla.redhat.com/show_bug.cgi?id=2025124
--- Comment #8 from Jakub Kadlčík jkadlcik@redhat.com --- Oops, sorry, wrong link in the previous comment
Spec URL: https://download.copr.fedorainfracloud.org/results/frostyx/qtile/fedora-rawh... SRPM URL: https://download.copr.fedorainfracloud.org/results/frostyx/qtile/fedora-rawh...
https://bugzilla.redhat.com/show_bug.cgi?id=2025124
lqlarry lqlarry@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |lqlarry@gmail.com
--- Comment #9 from lqlarry lqlarry@gmail.com --- (In reply to Jakub Kadlčík from comment #8)
Oops, sorry, wrong link in the previous comment
Spec URL: https://download.copr.fedorainfracloud.org/results/frostyx/qtile/fedora- rawhide-x86_64/03814640-qtile/qtile.spec SRPM URL: https://download.copr.fedorainfracloud.org/results/frostyx/qtile/fedora- rawhide-x86_64/03814640-qtile/qtile-0.21.0-1.fc37.src.rpm
(In reply to Jakub Kadlčík from comment #6)
I bumped a release and added Recommends: for packages, that are needed by widgets.
Spec URL: https://download.copr.fedorainfracloud.org/results/frostyx/qtile/fedora- rawhide-x86_64/03571096-qtile/qtile.spec SRPM URL: https://download.copr.fedorainfracloud.org/results/frostyx/qtile/fedora- rawhide-x86_64/03571096-qtile/qtile-0.20.0-2.fc37.src.rpm
@jak Thanks for doing thisproject and making it available. Everything is like a stock Qtile installment. I was able to slip my dot files in, install programs that I need to personal use and it workded like a charm.
Fedora does need a Qtile install method and now Fedora has one.
Thanks again.
https://bugzilla.redhat.com/show_bug.cgi?id=2025124
--- Comment #10 from lqlarry lqlarry@gmail.com --- (In reply to Jakub Kadlčík from comment #0)
Spec URL: https://download.copr.fedorainfracloud.org/results/frostyx/qtile/fedora- rawhide-x86_64/02968035-qtile/qtile.spec SRPM URL: https://download.copr.fedorainfracloud.org/results/frostyx/qtile/fedora- rawhide-x86_64/02968035-qtile/qtile-0.18.1-2.fc36.src.rpm Description: A pure-Python tiling window manager.
Features
* Simple, small and extensible. It's easy to write your own layouts, widgets and commands. * Configured in Python. * Command shell that allows all aspects of Qtile to be managed and inspected. * Complete remote scriptability - write scripts to set up workspaces, manipulate windows, update status bar widgets and more. * Qtile's remote scriptability makes it one of the most thoroughly unit-tested window mangers around.
Fedora Account System Username: frostyx
Fedora does need a Qtile install method and now Fedora has one.
Great work @Jak
https://bugzilla.redhat.com/show_bug.cgi?id=2025124
--- Comment #11 from Jakub Kadlčík jkadlcik@redhat.com --- Thank you for the kind words, Larry. I am happy that the Copr repository for Qtile is useful.
Just to clarify what is the actual blocker (I might have miscommunicated it previously). We need some Fedora package maintainer to do a formal review https://docs.fedoraproject.org/en-US/package-maintainers/Package_Review_Proc...
https://bugzilla.redhat.com/show_bug.cgi?id=2025124
--- Comment #12 from Jakub Kadlčík jkadlcik@redhat.com --- Another upstream release
Spec URL: https://download.copr.fedorainfracloud.org/results/frostyx/qtile/fedora-rawh... SRPM URL: https://download.copr.fedorainfracloud.org/results/frostyx/qtile/fedora-rawh...
https://bugzilla.redhat.com/show_bug.cgi?id=2025124
Carl George 🤠 carl@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |alex@akdev.xyz
--- Comment #13 from Carl George 🤠 carl@redhat.com --- *** Bug 2036556 has been marked as a duplicate of this bug. ***
https://bugzilla.redhat.com/show_bug.cgi?id=2025124
Carl George 🤠 carl@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Flags| |fedora-review? CC| |carl@redhat.com Status|NEW |ASSIGNED Assignee|nobody@fedoraproject.org |carl@redhat.com
--- Comment #14 from Carl George 🤠 carl@redhat.com --- I can take this review.
================================================================================
Do you think you can update this spec file to use the current Python packaging guidelines? At a glance it looks mostly correct for the legacy guidelines (which are still allowed), but since we're re-reviewing it now is a great time to convert it.
https://docs.fedoraproject.org/en-US/packaging-guidelines/Python/ https://docs.fedoraproject.org/en-US/packaging-guidelines/Python_201x/
================================================================================
The license field needs to be updated to use an SPDX license expression.
-License: MIT and GPLv3+ +License: MIT AND GPL-3.0-or-later
It also appears that the license breakdown comment is out of date. licensecheck found libqtile/widget/cmus.py and libqtile/widget/moc.py as the only files under the GPL.
https://docs.fedoraproject.org/en-US/legal/license-field/
================================================================================
Upstream has a test suite, try to run it in %check if possible. The upstream tox file looks like it tries to pip install stuff, which won't work in the build environment. You might choose to patch this file and run the tests with %tox. Alternatively you could run the tests directly with %pytest.
https://docs.fedoraproject.org/en-US/packaging-guidelines/#_test_suites https://docs.fedoraproject.org/en-US/packaging-guidelines/Python/#_tests
================================================================================
For the %setup macros, the default directory name is %{name}-%{version}, so that flag can be left out.
-%setup -q -n qtile-%{version} +%setup -q
Optionally you could switch this to %autosetup, which will automatically apply patches, if and when they are needed.
-%setup -q -n qtile-%{version} +%autosetup -p 1
https://docs.fedoraproject.org/en-US/packaging-guidelines/#_autosetup
https://bugzilla.redhat.com/show_bug.cgi?id=2025124
--- Comment #15 from Jakub Kadlčík jkadlcik@redhat.com --- I can't believe somebody finally took the review, thank you Carl :-)
I updated the spec to follow the current Python packaging guidelines, fixed the license, autosetup macro, and added tests. Also, I remembered that some time ago, I added Wayland support to the package but it requires dependencies that are not in Fedora yet, so I added bcond to build the package without Wayland support for the official Fedora repos but easily allow people to build it for themselves with Wayland. I intend to add the dependencies into Fedora as well but it can take some time.
Spec URL: https://download.copr.fedorainfracloud.org/results/frostyx/qtile/fedora-rawh... SRPM URL: https://download.copr.fedorainfracloud.org/results/frostyx/qtile/fedora-rawh...
Here is the diff, so you can easily see the changes https://github.com/frostyx/rpmbuild-topdir/commit/6dd1e84
https://bugzilla.redhat.com/show_bug.cgi?id=2025124
--- Comment #16 from Jakub Kadlčík jkadlcik@redhat.com --- Fixed one more error
Spec URL: https://download.copr.fedorainfracloud.org/results/frostyx/qtile/fedora-rawh... SRPM URL: https://download.copr.fedorainfracloud.org/results/frostyx/qtile/fedora-rawh... Diff: https://github.com/FrostyX/rpmbuild-topdir/commit/554e6d6
https://bugzilla.redhat.com/show_bug.cgi?id=2025124
--- Comment #17 from Carl George 🤠 carl@redhat.com --- Thanks for the adjustments. Here are a few more things we can improve.
================================================================================
I can see you had to add another source from the GitHub repo in order to run the tests. If the PyPI tarball is missing files needed for the tests we can just use the GitHub tarball directly instead. Also it's optional to use numbered sources now, and really only makes sense if you need to reference the sources independently later in the spec file.
-Source0: %{pypi_source} -Source1: https://raw.githubusercontent.com/qtile/qtile/v%%7Bpypi_version%7D/bin/qtile +Source: https://github.com/qtile/qtile/archive/v%%7Bversion%7D/qtile-%%7Bversion%7D....
https://docs.fedoraproject.org/en-US/packaging-guidelines/Python/#_source_fi... https://docs.fedoraproject.org/en-US/packaging-guidelines/SourceURL/#_git_ta...
================================================================================
A quick test build on my end after switching to the GitHub tarball has me run into the warning listed in the "Source files from PyPI" documentation linked above. We'll also need to export an environment variable in two spots.
%generate_buildrequires +export SETUPTOOLS_SCM_PRETEND_VERSION=%{version} %pyproject_buildrequires
%build +export SETUPTOOLS_SCM_PRETEND_VERSION=%{version} %pyproject_wheel
================================================================================
Since Name and Version are exactly the same as pypi_name and pypi_version, for readability it would be better to just skip defining the extra macros and use the built in fields.
-%global pypi_name qtile -%global pypi_version 0.22.1
-Name: %{pypi_name} -Version: %{pypi_version} +Name: qtile +Version: 0.22.1
================================================================================
There are a few buildrequires and requires that are duplicated between the automatic generators and explicit declarations in the spec file. The explicit ones should be removed in favor of the automatic ones. The automatic ones are based on the setup_requires and install_requires fields in the setup.cfg file. Some of the explicit dependencies might not even be necessary if upstream hasn't declared them. Please review and remove the ones you can.
I'll note that there is also a test_requires field in setup.cfg, but I don't believe that is read by %pyproject_buildrequires. Even if it were we'd need to patch out the linting dependencies. Ideally the only explicit Python buildrequires we should have are python3-devel and python3-pytest. If there are other Python ones we should reach out to upstream and try to get them added to the "test" extra, which we could pull in via `%pyproject_buildrequires -x test`. Hopefully upstream will also be open to the idea of having separate "test" and "lint" extras so we can skip the linting deps. Non-Python buildrequirements will probably still need to be listed explicitly.
I'll also point out that once the wayland dependencies are packaged and available, we can include them with `%pyproject_buildrequires -x wayland` without listing them explictly.
https://github.com/qtile/qtile/blob/v0.22.1/setup.cfg https://docs.fedoraproject.org/en-US/packaging-guidelines/Python/#Automatica... https://docs.fedoraproject.org/en-US/packaging-guidelines/Python/#_linters
================================================================================
The code has a few system libraries that it loads with ffi.dlopen. These should be listed as explict requires (and buildrequires for the test suite). It gets a little tricky due to the suffix in the provides, but the following should work for what we need.
+# Some dependencies are loaded with ffi.dlopen, and to declare them properly +# we'll need this suffix. +%if 0%{?__isa_bits} == 32 +%global libsymbolsuffix %{nil} +%else +%global libsymbolsuffix ()(%{__isa_bits}bit) +%endif
-BuildRequires: pango-devel +BuildRequires: libgobject-2.0.so.0%{libsymbolsuffix} +BuildRequires: libpango-1.0.so.0%{libsymbolsuffix} +BuildRequires: libpangocairo-1.0.so.0%{libsymbolsuffix}
+Requires: libgobject-2.0.so.0%{libsymbolsuffix} +Requires: libpango-1.0.so.0%{libsymbolsuffix} +Requires: libpangocairo-1.0.so.0%{libsymbolsuffix}
https://github.com/qtile/qtile/blob/v0.22.1/libqtile/pangocffi.py#L55-L57
On a related note, I realized that python3-cairocffi also does some dlopen stuff, and doesn't indicate it's requirements properly. That's why this package appears to need gdk-pixbuf2-devel. I'll file a bug with that package because `python3 -c 'import cairocffi.pixbuf'` straight up doesn't work unless you have gdk-pixbuf2 installed. As a temporary fix we can use these lines.
+# missing from python3-cairocffi +BuildRequires: libgdk_pixbuf-2.0.so.0%{libsymbolsuffix} +BuildRequires: libglib-2.0.so.0%{libsymbolsuffix} +BuildRequires: libgdk-3.so.0%{libsymbolsuffix}
+# missing from python3-cairocffi +Requires: libgdk_pixbuf-2.0.so.0%{libsymbolsuffix} +Requires: libglib-2.0.so.0%{libsymbolsuffix} +Requires: libgdk-3.so.0%{libsymbolsuffix}
https://github.com/Kozea/cairocffi/blob/v1.3.0/cairocffi/pixbuf.py#L22-L37
================================================================================
Since this package doesn't have any subpackages, it's not necessary to define the description in a separate %_description macro. That's mentioned in the guidelines because usually Python packages need to repeat the same description in the top level %description section and in the python3 subpackage %description section. That's not the case here.
================================================================================
I see you added the desktop-file-validate commands to %check. That's a good improvement, but instead of running separate install (in %install) and desktop-file-validate (in %check) commands, we could just run desktop-file-install (in %install) instead and do the installation and validation at the same time.
https://docs.fedoraproject.org/en-US/packaging-guidelines/#_desktop_files
================================================================================
The upstream setup.cfg file defines the pytest testpaths as "test", so I don't think we need to pass that to %pytest.
-%pytest test +%pytest
================================================================================
It's not necessary to use `-n qtile` when starting the %files section. Our Name field is qtile, and %files defaults to that. That flag is only needed when you need to define it as something else.
-%files -n qtile -f %{pyproject_files} +%files -f %{pyproject_files}
================================================================================
We can remove the license line outright, because the pyproject macros pick up what the upstream has defined as the license automatically and marks it with the proper RPM metadata. You can verify that on a built RPM file with the command `rpm -qpL`, which is currently showing me both /usr/lib/python3.11/site-packages/qtile-0.22.1.dist-info/LICENSE and /usr/share/licenses/qtile/LICENSE as license files in the package. These files are identical so we can skip the non-automatic one.
-%license LICENSE
================================================================================
Not a correction but just a curiosity on my part, what changes when enabling wayland support to switch the package from noarch to being architecture specific?
https://bugzilla.redhat.com/show_bug.cgi?id=2025124
--- Comment #18 from Carl George 🤠 carl@redhat.com --- Another thing I found in the upstream docs was that only the x11 backend is tested by default. To test both we'll need to pass some additional flags to pytest, and we can make the wayland one conditional.
-%pytest test +%pytest --backend x11 %{?with_wayland:--backend wayland}
https://docs.qtile.org/en/latest/manual/hacking.html#backends
================================================================================
Similarly we can pull in the "wayland" extras requirements and not have to specify them manually.
-%if %{with wayland} -# These packages are not in Fedora yet, however they are packaged in Copr -# https://copr.fedorainfracloud.org/coprs/frostyx/qtile/ -# So let's temporarily build the official Fedora package without Wayland support -# but build the Copr package with Wayland support. -BuildRequires: python3-pywlroots -BuildRequires: python3-pywayland -BuildRequires: python3-xkbcommon -%endif
-%pyproject_buildrequires +%pyproject_buildrequires %{?with_wayland:-x wayland}
That covers the buildrequires for those, but not the runtime requires. For that I think the best approach may be to create some subpackages.
* split the libqtile Python library into a python3-libqtile subpackage * use %pyproject_extras_subpkg to generate a python3-libqtile+wayland subpackage * recommend python3-libqtile+wayland from the main qtile package
https://docs.fedoraproject.org/en-US/packaging-guidelines/Python/#Extras https://docs.fedoraproject.org/en-US/packaging-guidelines/Python/#pyproject_...
This might be getting a bit too complex for bugzilla comments. Let me know if you'd prefer I just send a PR to your spec file on GitHub.
================================================================================
I sent a PR upstream to split out the "test" and "lint" dependencies. If that looks good to you we can include it as a patch and then install the test dependencies with %pyproject_buildrequires.
https://github.com/qtile/qtile/pull/4084
https://bugzilla.redhat.com/show_bug.cgi?id=2025124
--- Comment #19 from Jakub Kadlčík jkadlcik@redhat.com --- Thank you for the feedback, I am learning some new tricks :-)
I made almost all the requested changes, please take a look.
Spec URL: https://download.copr.fedorainfracloud.org/results/frostyx/qtile/fedora-rawh... SRPM URL: https://download.copr.fedorainfracloud.org/results/frostyx/qtile/fedora-rawh... Diff: https://github.com/frostyx/rpmbuild-topdir/commit/9cec169
There were two problems though.
1. When I started removing the explicit BuildRequires, tests started falling. Do you think, we are installing the generated dependencies incorrectly? Also, just for the record, I am fine with having explicit BuildRequires :-)
2. Wayland support requires much more work than I can allocate to it right now. There are the missing dependencies, and also tests started failing for some reason. I would like to keep the Wayland-specific changes in a WIP state and iteratively fix them one by one.
Not a correction but just a curiosity on my part, what changes when enabling Wayland support to switch the package from noarch to being architecture specific?
I am curious about this as well. Builds started failing with a message, that I use `BuildArch: noarch` for an architecture-specific package. So I added the condition to fix the issue, but I don't quite understand why it started building architecture-specific RPMs.
https://bugzilla.redhat.com/show_bug.cgi?id=2025124
--- Comment #20 from Jakub Kadlčík jkadlcik@redhat.com --- Updated to the new upstream version 0.23.0.
Spec URL: https://download.copr.fedorainfracloud.org/results/frostyx/qtile-f39/fedora-... SRPM URL: https://download.copr.fedorainfracloud.org/results/frostyx/qtile-f39/fedora-...
https://bugzilla.redhat.com/show_bug.cgi?id=2025124
Fedora Review Service fedora-review-bot@fedoraproject.org changed:
What |Removed |Added ---------------------------------------------------------------------------- URL| |http://qtile.org
--- Comment #21 from Fedora Review Service fedora-review-bot@fedoraproject.org --- Copr build: https://copr.fedorainfracloud.org/coprs/build/6610458 (succeeded)
Review template: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-rev...
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.
https://bugzilla.redhat.com/show_bug.cgi?id=2025124
--- Comment #22 from Carl George 🤠 carl@redhat.com --- https://github.com/FrostyX/rpmbuild-topdir/pull/1
https://bugzilla.redhat.com/show_bug.cgi?id=2025124
--- Comment #23 from Carl George 🤠 carl@redhat.com --- That first PR is the mandatory stuff for the review.
Previously in comment 18 I mentioned implementing python3-libqtile and python3-libqtile+wayland subpackages. This would prevent the need to manually keep the wayland dependencies in sync with upstream. Separate from that PR, here is a commit with a possible implementation of those subpackages, as well as a qtile-wayland subpackage to make it obvious for users on how to opt-in. If you like this, I can send it as a PR as well. These changes are not mandatory for the review, and we could even leave them out for now until you're ready to enable the wayland conditional.
https://github.com/carlwgeorge/FrostyX-rpmbuild-topdir/commit/fa66a83d7ccbf6...
https://bugzilla.redhat.com/show_bug.cgi?id=2025124
--- Comment #24 from Jakub Kadlčík jkadlcik@redhat.com --- Thank you very much for the patches Carl George, they helped a ton.
Spec URL: https://download.copr.fedorainfracloud.org/results/frostyx/qtile-f39/fedora-... SRPM URL: https://download.copr.fedorainfracloud.org/results/frostyx/qtile-f39/fedora-...
https://bugzilla.redhat.com/show_bug.cgi?id=2025124
--- Comment #25 from Fedora Review Service fedora-review-bot@fedoraproject.org --- Created attachment 1998876 --> https://bugzilla.redhat.com/attachment.cgi?id=1998876&action=edit The .spec file difference from Copr build 6610458 to 6626809
https://bugzilla.redhat.com/show_bug.cgi?id=2025124
--- Comment #26 from Fedora Review Service fedora-review-bot@fedoraproject.org --- Copr build: https://copr.fedorainfracloud.org/coprs/build/6626809 (failed)
Build log: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-rev...
Please make sure the package builds successfully at least for Fedora Rawhide.
- If the build failed for unrelated reasons (e.g. temporary network unavailability), please ignore it. - If the build failed because of missing BuildRequires, please make sure they are listed in the "Depends On" field
--- 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.
https://bugzilla.redhat.com/show_bug.cgi?id=2025124
--- Comment #27 from Jakub Kadlčík jkadlcik@redhat.com ---
Copr build: https://copr.fedorainfracloud.org/coprs/build/6626809 (failed)
I think we can ignore the failure. The tests sometimes fail because of "AssertionError: couldn't connect to xvfb". Simply resubmitting the build usually fixes the issue. I am going to report it to the upstream.
https://bugzilla.redhat.com/show_bug.cgi?id=2025124
--- Comment #28 from Jakub Kadlčík jkadlcik@redhat.com ---
I am going to report it to the upstream.
Reported here https://github.com/qtile/qtile/issues/4573
and mentioned in the spec https://github.com/frostyx/rpmbuild-topdir/commit/809c00763f4d4e3aa1153f2954...
https://bugzilla.redhat.com/show_bug.cgi?id=2025124
--- Comment #29 from Carl George 🤠 carl@redhat.com --- This is looking good. I also figured out the wayland test errors, please see this PR:
https://github.com/FrostyX/rpmbuild-topdir/pull/2
The fedora-review output is already looking good on the last copr build. Once that PR is merged let's do one more build and I'll run fedora-review against that and approve the package.
https://bugzilla.redhat.com/show_bug.cgi?id=2025124
--- Comment #30 from Jakub Kadlčík jkadlcik@redhat.com --- Thank you again for the PR.
Spec URL: https://download.copr.fedorainfracloud.org/results/frostyx/qtile-f39/fedora-... SRPM URL: https://download.copr.fedorainfracloud.org/results/frostyx/qtile-f39/fedora-...
https://bugzilla.redhat.com/show_bug.cgi?id=2025124
--- Comment #31 from Fedora Review Service fedora-review-bot@fedoraproject.org --- Created attachment 1999160 --> https://bugzilla.redhat.com/attachment.cgi?id=1999160&action=edit The .spec file difference from Copr build 6626809 to 6630297
https://bugzilla.redhat.com/show_bug.cgi?id=2025124
--- Comment #32 from Fedora Review Service fedora-review-bot@fedoraproject.org --- Copr build: https://copr.fedorainfracloud.org/coprs/build/6630297 (succeeded)
Review template: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-rev...
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.
https://bugzilla.redhat.com/show_bug.cgi?id=2025124
Carl George 🤠 carl@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |POST Flags|fedora-review? |fedora-review+
--- Comment #33 from Carl George 🤠 carl@redhat.com --- Package is APPROVED.
Package Review ==============
Legend: [x] = Pass, [!] = Fail, [-] = Not applicable, [?] = Not evaluated
===== MUST items =====
C/C++: [x]: Development (unversioned) .so files in -devel subpackage, if present.
Generic: [x]: Package is licensed with an open-source compatible license and meets other legal requirements as defined in the legal section of Packaging Guidelines. [x]: License field in the package spec file matches the actual license. Note: Checking patched sources after %prep for licenses. Licenses found: "Unknown or generated", "MIT License", "*No copyright* MIT License", "GNU General Public License v3.0 or later". 227 files have unknown license. Detailed output of licensecheck in /home/carl/packaging/reviews/qtile/2025124-qtile/licensecheck.txt [x]: License file installed when any subpackage combination is installed. [x]: If the package is under multiple licenses, the licensing breakdown must be documented in the spec. [x]: %build honors applicable compiler flags or justifies otherwise. [x]: Package contains no bundled libraries without FPC exception. [x]: Changelog in prescribed format. [x]: Sources contain only permissible code or content. [-]: Development files must be in a -devel package [x]: Package uses nothing in %doc for runtime. [x]: Package consistently uses macros (instead of hard-coded directory names). [x]: Package is named according to the Package Naming Guidelines. [x]: Package does not generate any conflict. [x]: Package obeys FHS, except libexecdir and /usr/target. [-]: If the package is a rename of another package, proper Obsoletes and Provides are present. [x]: Requires correct, justified where necessary. [x]: Spec file is legible and written in American English. [-]: Package contains systemd file(s) if in need. [x]: Useful -debuginfo package or justification otherwise. [x]: Package is not known to require an ExcludeArch tag. [x]: Package complies to the Packaging Guidelines [x]: Package successfully compiles and builds into binary rpms on at least one supported primary architecture. [x]: Package installs properly. [x]: Rpmlint is run on all rpms the build produces. Note: There are rpmlint messages (see attachment). [x]: If (and only if) the source package includes the text of the license(s) in its own file, then that file, containing the text of the license(s) for the package is included in %license. [x]: The License field must be a valid SPDX expression. [x]: Package requires other packages for directories it uses. [x]: Package must own all directories that it creates. [x]: Package does not own files or directories owned by other packages. [x]: Package uses either %{buildroot} or $RPM_BUILD_ROOT [x]: Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the beginning of %install. [x]: Macros in Summary, %description expandable at SRPM build time. [x]: Package contains desktop file if it is a GUI application. [x]: Package installs a %{name}.desktop using desktop-file-install or desktop-file-validate if there is such a file. [x]: Dist tag is present. [x]: Package does not contain duplicates in %files. [x]: Permissions on files are set properly. [x]: Package must not depend on deprecated() packages. [x]: Package use %makeinstall only when make install DESTDIR=... doesn't work. [x]: Package is named using only allowed ASCII characters. [x]: Package is not relocatable. [x]: Sources used to build the package match the upstream source, as provided in the spec URL. [x]: Spec file name must match the spec package %{name}, in the format %{name}.spec. [x]: File names are valid UTF-8. [x]: Large documentation must go in a -doc subpackage. Large could be size (~1MB) or number of files. Note: Documentation size is 3707 bytes in 1 files. [x]: Packages must not store files under /srv, /opt or /usr/local
Python: [-]: Binary eggs must be removed in %prep [x]: Python eggs must not download any dependencies during the build process. [-]: A package which is used by another package via an egg interface should provide egg info. [x]: Package meets the Packaging Guidelines::Python [x]: Package contains BR: python2-devel or python3-devel [x]: Packages MUST NOT have dependencies (either build-time or runtime) on packages named with the unversioned python- prefix unless no properly versioned package exists. Dependencies on Python packages instead MUST use names beginning with python2- or python3- as appropriate. [x]: Python packages must not contain %{pythonX_site(lib|arch)}/* in %files
===== SHOULD items =====
Generic: [-]: If the source package does not include license text(s) as a separate file from upstream, the packager SHOULD query upstream to include it. [x]: Final provides and requires are sane (see attachments). [x]: Fully versioned dependency in subpackages if applicable. Note: No Requires: %{name}%{?_isa} = %{version}-%{release} in python3-libqtile , qtile-wayland , python3-libqtile+wayland [x]: Package functions as described. [x]: Latest version is packaged. [x]: Package does not include license text files separate from upstream. [-]: Sources are verified with gpgverify first in %prep if upstream publishes signatures. Note: gpgverify is not used. [x]: Package should compile and build into binary rpms on all supported architectures. [x]: %check is present and all tests pass. [x]: Packages should try to preserve timestamps of original installed files. [x]: Reviewer should test that the package builds in mock. [x]: Buildroot is not present [x]: Package has no %clean section with rm -rf %{buildroot} (or $RPM_BUILD_ROOT) [x]: No file requires outside of /etc, /bin, /sbin, /usr/bin, /usr/sbin. [x]: Packager, Vendor, PreReq, Copyright tags should not be in spec file [x]: Sources can be downloaded from URI in Source: tag [x]: SourceX is a working URL. [x]: Spec use %global instead of %define unless justified.
===== EXTRA items =====
Generic: [x]: Rpmlint is run on all installed packages. Note: There are rpmlint messages (see attachment). [x]: Large data in /usr/share should live in a noarch subpackage if package is arched. [x]: Spec file according to URL is the same as in SRPM.
https://bugzilla.redhat.com/show_bug.cgi?id=2025124
--- Comment #34 from Jakub Kadlčík jkadlcik@redhat.com --- Thank you very much for the review Carl George <3
https://bugzilla.redhat.com/show_bug.cgi?id=2025124
Carl George 🤠 carl@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Resolution|--- |RAWHIDE Fixed In Version| |qtile-0.23.0-6.fc40 Status|POST |CLOSED Last Closed| |2024-05-01 16:08:54
--- Comment #35 from Carl George 🤠 carl@redhat.com --- Looks like this is back in Fedora now, so closing this review.
https://bodhi.fedoraproject.org/updates/FEDORA-2024-0d6cf108ef
package-review@lists.fedoraproject.org