https://bugzilla.redhat.com/show_bug.cgi?id=2402878
--- Comment #6 from Ben Beasley <[email protected]> --- The following review is for the original submission. Most of it still applies, except: - You’ve now correctly applied and documented the s390x patch. - You are now running the tests, but: * The comment about using a URL for test.py still applies * The comments about just packaging from the GitHub archive being even better still apply. - I am not convinced that setting -DNDEBUG is a proper workaround for the test failures. This just hides the underlying issue. If you were to submit the package without a fix, it would be better to skip the affected tests using pytest as described in the review that to simply turn off all C assertions. There might be a performance case for setting -DNDEBUG, which is certainly a common practice when building C and C++ software, but I still think that there is a lot of risk (of difficult-to-debug issues appearing in dependent package) associated with just ignoring the assertion failures and the invariant violations they represent. Package Review ============== Legend: [x] = Pass, [!] = Fail, [-] = Not applicable, [?] = Not evaluated ===== Issues ==== - I think the added indirection of %{pypi_name} macro just makes the spec file harder to read, without making it meaningfully more “reusable” in practice. It’s permissible to leave it in, but I recommend removing it and just writing out pytricia wherever you have %{pypi_name}. - This is not incorrect, but it’s needlessly verbose to write out all the default values. Source: %{pypi_source %{pypi_name} %{version} tar.gz} Try: Source: %{pypi_source %{pypi_name}} Or, if you drop %{pypi_name}, Source: %{pypi_source pytricia} - You don’t need these: BuildRequires: python3dist(setuptools) BuildRequires: python3dist(wheel) The setuptools dependency will be generated by %pyproject_buildrequires, and the wheel dependency is actually unnecessary. - The license files are already handled automatically in %{pyproject_files}, $ rpm -qL -p results/python3-pytricia-1.3.0-1.fc44.aarch64.rpm /usr/lib64/python3.14/site-packages/pytricia-1.3.0.dist-info/licenses/COPYING /usr/lib64/python3.14/site-packages/pytricia-1.3.0.dist-info/licenses/COPYING.LESSER /usr/share/licenses/python3-pytricia/COPYING /usr/share/licenses/python3-pytricia/COPYING.LESSER so you don’t need to manually install duplicate license files. Please remove %license COPYING %license COPYING.LESSER and write %pyproject_save_files -l %{pypi_name} or %pyproject_save_files -l pytricia where the -l option asserts that at least one license file was detected and properly listed in the dist-info directory. This guards against accidental disappearance of the packaged license files in a future update. - The submitted spec file and source RPM do not match. In the spec file, you copy in test.py from the corresponding GitHub archive and run the tests. This is a good idea. I think you *should* run the tests. However, if you want to include a single file from the corresponding archive, it would be better to do so by URL. Instead of # This is the version 1.3.0 test file from the git repo Source1: test.py you could write # This is the version 1.3.0 test file from the git repo Source1: %{url}/raw/eacae4c67784ca85e2ef995a7a63be60da66c61e/test.py where eacae4c67784ca85e2ef995a7a63be60da66c61e is clearly the commit hash that corresponds to the 1.3.0 PyPI release. Even better, just package from the GitHub archive directly instead of from the PyPI sdist. This is exactly the situation described by https://docs.fedoraproject.org/en-US/packaging-guidelines/Python/#_source_files_from_pypi. I know that this is tedious since upstream hasn’t been tagging releases, https://github.com/jsommers/pytricia/issues/43, but it’s still possible to identify the commit that corresponds to the 1.3.0 release. Name: python-pytricia # Upstream does not tag releases, # https://github.com/jsommers/pytricia/issues/43, but we can still identify # the commit corresponding to the PyPI release. Since we are confident that # this commit corresponds to the release, we do not need # to include a # snapshot information field in the Version. %global commit eacae4c67784ca85e2ef995a7a63be60da66c61e Version: 1.3.0 Release: %autorelease Summary: A library for fast IP address lookup in Python License: LGPL-3.0-or-later URL: https://github.com/jsommers/pytricia Source: %{url}/archive/%{commit}/pytricia-%{commit}.tar.gz […] %prep %autosetup -n pytricia-%{commit} Then you could write: %check %pyproject_check_import %{py3_test_envvars} %{python3} -m unittest discover When I tried this, I ran into an assertion failure, which I see is already reported upstream at https://github.com/jsommers/pytricia/issues/42. I was able to reproduce this in a virtualenv by enabling link-time optimization (LTO), which is the default in Fedora; see https://github.com/jsommers/pytricia/issues/42#issuecomment-3394037301. This is a bug and should be diagnosed and fixed upstream. I tried to work around it for now by disabling LTO for the package (see https://docs.fedoraproject.org/en-US/packaging-guidelines/#compiler), # Opt out of LTO, since it exposes a bug: # test.py failure: Assertion `prefix->bitlen <= patricia->maxbits' failed. # https://github.com/jsommers/pytricia/issues/42 %global _lto_cflags %{nil} but I still encountered the assertion failure in the tests. We can add # We can run the tests with # %%{py3_test_envvars} %%{python3} -m unittest discover # but pytest makes it easier to skip failing tests. BuildRequires: %{py3_dist pytest} and write # test.py failure: Assertion `prefix->bitlen <= patricia->maxbits' failed. # https://github.com/jsommers/pytricia/issues/42 k="${k-}${k+ and }not (PyTriciaTests and testDelete)" k="${k-}${k+ and }not (PyTriciaTests and testGet2)" k="${k-}${k+ and }not (PyTriciaTests and testNonStringKey)" %pytest -k "${k-}" -v test.py testload.py and thereby run the tests that do pass. However, I think the assertion failures look significant, and I question whether this should be packaged without resolving the test failures. - The file patricia.c is derived from src/lib/radix/radix.c in the Multi-threaded Routing Toolkit, MRT, https://sourceforge.net/projects/mrt/, but it is not handled in accordance with https://docs.fedoraproject.org/en-US/packaging-guidelines/#bundling and the original license is not accounted for. I opened an upstream PR to distribute this license text in its own file, https://github.com/jsommers/pytricia/pull/47. It looks like the license terms are almost an exact match for BSD-4-Clause-UC, but the copyright owners are not The Regents of the University of California. Please file an issue at https://gitlab.com/fedora/legal/fedora-license-data asking for license review. It may be that we should ask SPDX to adjust the definition of BSD-4-Clause-UC to make this part replaceable text so that. Assuming that this license is found to be BSD-4-Clause-UC, you will need to include it in the License expression: # The entire source is LGPL-3.0-or-later, except that patricia.c contains # code derived from the Multi-threaded Routing Toolkit, MRT, under # BSD-4-Clause-UC. License: LGPL-3.0-or-later AND BSD-4-Clause-UC You should also treat this as a case of bundling, https://docs.fedoraproject.org/en-US/packaging-guidelines/#bundling. # The file patricia.c is copied and modified from src/lib/radix/radix.c in the # Multi-threaded Routing Toolkit, MRT, https://sourceforge.net/projects/mrt/. # Since MRT was not designed for use as a shared library, and since it is no # longer actively developed, there appears to be no path to unbundling this. It’s # not clear exactly which version of MRT was used as the basis for patricia.c. Provides: bundled(mrt) - It looks like you have a fix for big-endian problems, but it’s only in the submitted spec file, and not the source RPM. Also, you should link the upstream PR in a comment, https://docs.fedoraproject.org/en-US/packaging-guidelines/#_all_patches_should_have_an_upstream_bug_link_or_comment: # Fix integer key lookups on big-endian architectures # https://github.com/jsommers/pytricia/pull/45 Patch: … Interestingly, I wasn’t able to reproduce the s390x issue myself, because according to https://github.com/jsommers/pytricia/issues/44, the affected test is testNonStringKey, and that test is also affected by https://github.com/jsommers/pytricia/issues/42. ===== MUST items ===== C/C++: [x]: Package does not contain kernel modules. [x]: Development (unversioned) .so files in -devel subpackage, if present. Note: Unversioned so-files in private %_libdir subdirectory (see attachment). Verify they are not in ld path. python3-pytricia: /usr/lib64/python3.14/site-packages/pytricia.cpython-314-aarch64-linux-gnu.so This is a properly-installed Python extension module. Nothing is wrong here. [x]: If your application is a C or C++ application you must list a BuildRequires against gcc, gcc-c++ or clang. [x]: Package does not contain any libtool archives (.la) [x]: Package contains no static executables. [x]: Rpath absent or only used for internal libs. 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. However, the MRT license needs to be conclusively identified and added to the License expression. [!]: License field in the package spec file matches the actual license. Note: Checking patched sources after %prep for licenses. Licenses found: "Unknown or generated", "GNU Lesser General Public License, Version 3", "*No copyright* GNU Lesser General Public License, Version 3", "BSD 4-Clause License and/or GNU Lesser General Public License v3.0 or later", "*No copyright* GNU Lesser General Public License v3.0 or later". 6 files have unknown license. Detailed output of licensecheck in /home/ben/fedora/review/2402878-python- pytricia/licensecheck.txt Missing the MRT license, probably BSD-4-Clause-UC, from patricia.c. See Issues. [x]: Package must own all directories that it creates. Note: Directories without known owners: /usr/lib64/python3.14/site- packages, /usr/lib64/python3.14 Spurious diagnostic: python3-libs owns these. [x]: %build honors applicable compiler flags or justifies otherwise. [!]: Package contains no bundled libraries without FPC exception. Derivation of patricia.c from a source file in MRT should be documented as a case of bundling. Explicit FPC exception is no longer required, but we must satisfy the requirements of https://docs.fedoraproject.org/en-US/packaging-guidelines/#bundling. [x]: Changelog in prescribed format. [x]: Sources contain only permissible code or content. [-]: Package contains desktop file if it is a GUI application. [-]: 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. [ ]: 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 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]: 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 does not use a name that already exists. [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 9890 bytes in 1 files. [x]: Packages must not store files under /srv, /opt or /usr/local Python: [x]: Python eggs must not download any dependencies during the build process. [x]: 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 [x]: Binary eggs must be removed in %prep ===== 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. I asked upstream to distribute the MRT license terms in their own file, https://github.com/jsommers/pytricia/pull/47. [x]: Final provides and requires are sane (see attachments). [!]: Package functions as described. The three test failures appear to reveal a really significant problem. [x]: Latest version is packaged. [x]: Package does not include license text files separate from upstream. [-]: Patches link to upstream bugs/comments/lists or are otherwise justified. The s390x patch is missing from the submitted .spec file. When you add it, make sure you include an upstream status link. [-]: 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. The package does compile as submitted, with the tests not enabled. [!]: %check is present and all tests pass. The package is submitted with the tests disabled; there are significant test failures when they are enabled. [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: [!]: Spec file according to URL is the same as in SRPM. Note: Spec file as given by url is not the same as in SRPM (see attached diff). See: (this test has no URL) [x]: Rpmlint is run on all installed packages. Note: No rpmlint messages. [x]: Large data in /usr/share should live in a noarch subpackage if package is arched. Rpmlint ------- Checking: python3-pytricia-1.3.0-1.fc44.aarch64.rpm python-pytricia-1.3.0-1.fc44.src.rpm ============================ rpmlint session starts ============================ rpmlint: 2.7.0 configuration: /usr/lib/python3.13/site-packages/rpmlint/configdefaults.toml /etc/xdg/rpmlint/fedora-spdx-licenses.toml /etc/xdg/rpmlint/fedora.toml /etc/xdg/rpmlint/scoring.toml /etc/xdg/rpmlint/users-groups.toml /etc/xdg/rpmlint/warn-on-functions.toml rpmlintrc: [PosixPath('/tmp/tmpe7252pg3')] checks: 32, packages: 2 python-pytricia.src: E: spelling-error ('patricia', '%description -l en_US patricia -> Patricia, patrician, patricidal') 2 packages and 0 specfiles checked; 1 errors, 0 warnings, 7 filtered, 1 badness; has taken 0.4 s Rpmlint (installed packages) ---------------------------- ============================ rpmlint session starts ============================ rpmlint: 2.7.0 configuration: /usr/lib/python3.14/site-packages/rpmlint/configdefaults.toml /etc/xdg/rpmlint/fedora-spdx-licenses.toml /etc/xdg/rpmlint/fedora.toml /etc/xdg/rpmlint/scoring.toml /etc/xdg/rpmlint/users-groups.toml /etc/xdg/rpmlint/warn-on-functions.toml checks: 32, packages: 1 1 packages and 0 specfiles checked; 0 errors, 0 warnings, 3 filtered, 0 badness; has taken 0.0 s Unversioned so-files -------------------- python3-pytricia: /usr/lib64/python3.14/site-packages/pytricia.cpython-314-aarch64-linux-gnu.so Source checksums ---------------- https://files.pythonhosted.org/packages/source/p/pytricia/pytricia-1.3.0.tar.gz : CHECKSUM(SHA256) this package : 1c3a3d6909e10d4c9c2f0fe4542a2481e109d29aab99cc027ca7fe93f8c8853f CHECKSUM(SHA256) upstream package : 1c3a3d6909e10d4c9c2f0fe4542a2481e109d29aab99cc027ca7fe93f8c8853f Requires -------- python3-pytricia (rpmlib, GLIBC filtered): ld-linux-aarch64.so.1()(64bit) libc.so.6()(64bit) python(abi) rtld(GNU_HASH) Provides -------- python3-pytricia: python-pytricia python3-pytricia python3-pytricia(aarch-64) python3.14-pytricia python3.14dist(pytricia) python3dist(pytricia) Diff spec file in url and in SRPM --------------------------------- --- /home/ben/fedora/review/2402878-python-pytricia/srpm/python-pytricia.spec 2025-10-12 07:55:18.497476753 +0100 +++ /home/ben/fedora/review/2402878-python-pytricia/srpm-unpacked/python-pytricia.spec 2025-10-09 01:00:00.000000000 +0100 @@ -9,7 +9,4 @@ URL: https://github.com/jsommers/pytricia Source: %{pypi_source %{pypi_name} %{version} tar.gz} -# This is the version 1.3.0 test file from the git repo -Source1: test.py -Patch: fix-big-endian.patch BuildRequires: python3-devel @@ -28,6 +25,5 @@ %prep -%autosetup -p1 -n %{pypi_name}-%{version} -cp %{SOURCE1} . +%autosetup -n %{pypi_name}-%{version} %generate_buildrequires @@ -35,6 +31,4 @@ %build -# Disable assertions in the C extensions in production build -export CFLAGS="${CFLAGS:-} -DNDEBUG" %pyproject_wheel @@ -45,5 +39,4 @@ %check %pyproject_check_import -PYTHONPATH=%{buildroot}%{python3_sitearch} %{python3} -m unittest test.py %files -n python3-%{pypi_name} -f %{pyproject_files} Generated by fedora-review 0.10.0 (e79b66b) last change: 2023-07-24 Command line :/usr/bin/fedora-review -b 2402878 Buildroot used: fedora-rawhide-aarch64 Active plugins: Generic, C/C++, Python, Shell-api Disabled plugins: Perl, PHP, SugarActivity, Java, R, Haskell, fonts, Ocaml Disabled flags: EXARCH, EPEL6, EPEL7, DISTTAG, BATCH -- 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=2402878 Report this comment as SPAM: https://bugzilla.redhat.com/enter_bug.cgi?product=Bugzilla&format=report-spam&short_desc=Report%20of%20Bug%202402878%23c6 -- _______________________________________________ package-review mailing list -- [email protected] To unsubscribe send an email to [email protected] 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/[email protected] Do not reply to spam, report it: https://pagure.io/fedora-infrastructure/new_issue
