https://bugzilla.redhat.com/show_bug.cgi?id=2025124 --- Comment #17 from Carl George 🤠 <carl@xxxxxxxxxx> --- 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%{pypi_version}/bin/qtile +Source: https://github.com/qtile/qtile/archive/v%{version}/qtile-%{version}.tar.gz https://docs.fedoraproject.org/en-US/packaging-guidelines/Python/#_source_files_from_pypi https://docs.fedoraproject.org/en-US/packaging-guidelines/SourceURL/#_git_tags ================================================================================ 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/#Automatically-generated-dependencies 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? -- You are receiving this mail because: You are always notified about changes to this product and component You are on the CC list for the bug. https://bugzilla.redhat.com/show_bug.cgi?id=2025124 _______________________________________________ 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
