https://bugzilla.redhat.com/show_bug.cgi?id=2259226 --- Comment #5 from Ben Beasley <code@xxxxxxxxxxxxxxxxxx> --- Package Review ============== Legend: [x] = Pass, [!] = Fail, [-] = Not applicable, [?] = Not evaluated - The AppVeyor files in appveyor/ are both CC0-1.0 and clearly code rather than content. Since CC0-1.0 is not-allowed for code in Fedora, I think you will need to filter these out of the source archive before uploading it to the lookaside cache. (Even though they do not contribute to the licenses of the binary RPMs, these files would still be distributed in the source RPMs.) I know this feels a bit silly, and you’re welcome to get a “second opinion” from Fedora Legal if you think it should be unnecessary. There is an exception that covers somewhat similar situations, https://gitlab.com/fedora/legal/fedora-license-data/-/blob/ee033a25064376059c59e83a3a0f25ee5625b27d/data/CC0-1.0.toml#L16-L20, but it’s only for projects that are trying to conform to the REUSE specification. Please see: https://src.fedoraproject.org/rpms/python-chardet/blob/rawhide/f/python-chardet.spec https://src.fedoraproject.org/rpms/python-chardet/blob/rawhide/f/get_source for a straightforward example of how you can obtain a “filtered” source archive. - While this doesn’t affect the correctness of the License field, # Apache-2: # chainer/functions/pooling/roi_average_pooling_2d.py # chainer/functions/pooling/roi_max_pooling_2d.py # chainer/functions/pooling/roi_pooling_2d.py should be # Apache-2.0 AND MIT since these files contain work under both licenses. - Code in chainer/links/caffe/ is BSD-2-Clause (or is it just chainer/links/caffe/protubuf3?); either way, the License needs an “AND BSD-2-CLause” term. - It would be best to add something like: # Some code forked from an unknown version of Keras appears in: # - chainer/initializer.py # - chainer/initializers/uniform.py # - chainer/initializers/orthogonal.py # - chainer/initializers/normal.py Provides: bundled(python3dist(keras)) to the python3-chainer subpackage to document that some code is “kind of bundled” from that project. - This is a matter of opinion, so no change is required, but I think that %{pypi_name} and %{pypi_version} really clutter the spec file with unnecessary indirection. You can write the version directly as Version: 7.8.1 and refer to it elsewhere as %{version}; and for %{pypi_name}, why not just write chainer? I don’t think macro-izing the name of the package really provides meaningful spec file reusability, certainly not enough to justify all the extra syntax. Again, you’re not required to listen to this suggestion. - There is no need to number the source and patch; you can just write Source: … Patch: … - Consider simplifying the Source URL from %{forgeurl}/archive/refs/tags/v%{version}.tar.gz#/%{pypi_name}-%{version}.tar.gz to %{forgeurl}/archive/v%{version}/%{pypi_name}-%{version}.tar.gz or %{forgeurl}/archive/v%{version}/chainer-%{version}.tar.gz (but, as noted earlier, I think you will need to use a script to download this URL and filter out the appveyor/ directory, so this URL won’t appear directly in the spec file except perhaps in a comment.) - Since upstream doesn’t use tox, I don’t think it makes sense to write: %pyproject_buildrequires -t Normally you could write %pyproject_buildrequires -x test since the project has a “test” extra, but in this case it pins an old pytest version and tries to bring in the deprecated https://pypi.org/project/mock/, so that won’t do. Just write %pyproject_buildrequires and stick with the manual BuildRequires: pytest you already have, which would be better written as BuildRequires: python3dist(pytest) or BuildRequires: %{py3_dist pytest} - This was probably always unnecessary, and is definitely unnecessary with pyproject-rpm-macros, which don’t even package egg-info (they use dist-info instead). Please remove it: # Remove bundled egg-info rm -rf chainer.egg-info - The %python_provide macro is obsolete; please remove this: %{?python_provide:%python_provide python3-chainer} https://docs.fedoraproject.org/en-US/packaging-guidelines/Python/#_deprecated_macros - You can avoid repeating the description text by writing %global _description %{expand: Chainer is a Python-based deep learning framework aiming at flexibility. It provides automatic differentiation APIs based on the define-by-run approach (a.k.a. dynamic computational graphs) as well as object-oriented high-level APIs to build and train neural networks.} and then %description %{_description} and %description -n python3-chainer %{_description} - You can and should use %pyproject_save_files effectively. Change %pyproject_save_files chainer to %pyproject_save_files chainer chainermn chainerx onnx_chainer and write %files -n python3-chainer -f %{pyproject_files} and remove %{python3_sitelib}/chainermn %{python3_sitelib}/chainerx %{python3_sitelib}/onnx_chainer %{python3_sitelib}/chainer %{python3_sitelib}/chainer-%{version}.dist-info Now that you’re using %pyproject_save_files correctly, all of the installed packages get import-smoke-tested, which reveals the need to add some BuildRequires. I found that this worked: # We need one of the mpi4py flavors for testing or test-importing some modules # within chainermn. It could be either openmpi or mpich; since the former is # ExcludeArch: %%{ix86}, we might as well choose the latter. BuildRequires: mpich-devel BuildRequires: python3-mpi4py-mpich # Needed for testing or test-importing onnx_chainer: BuildRequires: %{py3_dist onnx} […] %{_mpich_load} %pyproject_check_import %{_mpich_unload} - I think I agree with the choice to write %license LICENSE chainerx_cc/LICENSE.txt in order to properly mark a copy of chainerx_cc/LICENSE.txt as a license file (even though the main LICENSE file is already handled and marked in .dist-info; see the “-l” option to %pyproject_save_files, documented at https://src.fedoraproject.org/rpms/pyproject-rpm-macros). You might want to add chainer/links/caffe/LICENSE_caffe_pb2 as well. - It’s correct that you’re doing an import-only “smoke test” if you’re not running the tests, but is it really impossible to run any of the tests? And if so, could you at least explain why in a spec-file comment? I tried to run the tests and got a DeprecationWarning about use of pkg_resources. I couldn’t figure out any way to successfully ignore the warning – and since upstream is in a “maintenance only” mode with no further releases planned, and they have pinned an ancient version of pytest, I doubt they are going to fix this, so maybe this is the answer. Still, maybe it’s worth filing a bug upstream. Note that I tried running tests in a virtualenv using a checkout out from the upstream GitHub repo: $ python3 -m venv _e $ . _e/bin/activate $ pip install -e '.[test]' $ python -m pytest […] ModuleNotFoundError: No module named 'imp' $ pip install zombie-imp $ python -m pytest […] ModuleNotFoundError: No module named 'numpy.distutils' $ patch -p1 < ~/Downloads/review/2259226-python-chainer/srpm-unpacked/0001-Remove-osx-numpy-check.patch $ python -m pytest […] ModuleNotFoundError: No module named 'chainerx._testing' $ python -m pytest tests/chainer_tests/ -v collected 152931 items / 8 errors $ python -m pytest tests/chainer_tests/ --ignore=tests/chainer_tests/links_tests/caffe_tests/test_caffe_function.py --ignore=tests/chainer_tests/functions_tests/array_tests/test_get_item.py --ignore=tests/chainer_tests/exporters_tests/test_caffe.py --ignore=tests/chainer_tests/test_function_and_function_node.py -v collected 152931 items [canceled with ^C after a couple thousand tests, which all seemed to be SKIPPED or FAILED] So maybe it’s worth putting a bit more effort into running the tests. Either way, the “bit-rot” with fully supporting Python 3.12 is already becoming apparent. Just make sure you’re fairly confident the package is working properly as packaged, and you understand that you’ll probably be on your own to fix future compatibility issues given the state of upstream support. ===== MUST items ===== 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. [!]: 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* Creative Commons CC0 1.0", "BSD 3-Clause License", "Apache License 2.0 and/or MIT License", "BSD 2-Clause License", "*No copyright* Creative Commons Attribution". 1712 files have unknown license. Detailed output of licensecheck in /home/ben/Downloads/review/2259226-python- chainer/licensecheck.txt - Licensecheck detects “Creative Commons Attribution” in chainer/links/model/vision/vgg.py; this is spurious, and refers to the licenses of downloadable models that are not included in the package. - Some file in appveyor/ are CC0-1.0. This license is not-allowed for code in Fedora (even only in the source RPM!); see Issues. - A BSD-2-Clause license term for chainer/links/caffe/ is needed; see Issues. [x]: If the package is under multiple licenses, the licensing breakdown must be documented in the spec. This is no longer a hard requirement, https://gitlab.com/fedora/legal/fedora-legal-docs/-/issues/64, but it is a useful practice. [x]: Package contains no bundled libraries without FPC exception. It would be nice to treat code copied and forked from Keras as a case of bundling; see Issues. [x]: Changelog in prescribed format. [!]: Sources contain only permissible code or content. Sources contain CC0-1.0-licensed AppVeyor code; see Issues. [-]: 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]: Package is not known to require an ExcludeArch tag. [x]: Package complies to the Packaging Guidelines (unless otherwise noted) [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]: 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 4306 bytes in 1 files. [x]: Packages must not store files under /srv, /opt or /usr/local Python: [-]: Binary eggs must be removed in %prep Note: Cannot find any build in BUILD directory (--prebuilt option?) [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 Some improvements are suggested in Issues. [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). [?]: Package functions as described. [x]: Latest version is packaged. [x]: Package does not include license text files separate from upstream. [x]: Patches link to upstream bugs/comments/lists or are otherwise justified. - Downstream-only patch is adequately justified within the patch file; it would be nice to reproduce this in a spec-file comment. [-]: Sources are verified with gpgverify first in %prep if upstream publishes signatures. Note: gpgverify is not used. [?]: Package should compile and build into binary rpms on all supported architectures. [!]: %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: No rpmlint messages. [x]: Spec file according to URL is the same as in SRPM. Rpmlint ------- Checking: python3-chainer-7.8.1-1.fc40.noarch.rpm python-chainer-7.8.1-1.fc40.src.rpm ============================= rpmlint session starts ============================= rpmlint: 2.5.0 configuration: /usr/lib/python3.12/site-packages/rpmlint/configdefaults.toml /etc/xdg/rpmlint/fedora-legacy-licenses.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/tmpegev8yl8')] checks: 32, packages: 2 python3-chainer.noarch: W: python-missing-require setuptools python3-chainer.noarch: W: python-missing-require typing-extensions python3-chainer.noarch: W: python-missing-require filelock python3-chainer.noarch: W: python-missing-require numpy python3-chainer.noarch: W: python-missing-require protobuf python3-chainer.noarch: W: python-missing-require six 2 packages and 0 specfiles checked; 0 errors, 6 warnings, 7 filtered, 0 badness; has taken 2.0 s Rpmlint (installed packages) ---------------------------- ============================ rpmlint session starts ============================ rpmlint: 2.5.0 configuration: /usr/lib/python3.12/site-packages/rpmlint/configdefaults.toml /etc/xdg/rpmlint/fedora-legacy-licenses.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, 9 filtered, 0 badness; has taken 0.9 s Source checksums ---------------- https://github.com/chainer/chainer/archive/refs/tags/v7.8.1.tar.gz#/chainer-7.8.1.tar.gz : CHECKSUM(SHA256) this package : 62ca7b1933cf76d65ee9441b7f71d747cbbf41529148552efdaeeaf0a31822f2 CHECKSUM(SHA256) upstream package : 62ca7b1933cf76d65ee9441b7f71d747cbbf41529148552efdaeeaf0a31822f2 Requires -------- python3-chainer (rpmlib, GLIBC filtered): python(abi) python3.12dist(filelock) python3.12dist(numpy) python3.12dist(protobuf) python3.12dist(setuptools) python3.12dist(six) python3.12dist(typing-extensions) Provides -------- python3-chainer: python-chainer python3-chainer python3.12-chainer python3.12dist(chainer) python3dist(chainer) Generated by fedora-review 0.10.0 (e79b66b) last change: 2023-07-24 Command line :/usr/bin/fedora-review -b 2259226 --mock-options=--dnf Buildroot used: fedora-rawhide-x86_64 Active plugins: Python, Shell-api, Generic Disabled plugins: fonts, Java, C/C++, Haskell, Ocaml, SugarActivity, PHP, R, Perl 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=2259226 Report this comment as SPAM: https://bugzilla.redhat.com/enter_bug.cgi?product=Bugzilla&format=report-spam&short_desc=Report%20of%20Bug%202259226%23c5 -- _______________________________________________ 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
