https://bugzilla.redhat.com/show_bug.cgi?id=2003700 Miro Hrončok <mhroncok@xxxxxxxxxx> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |mhroncok@xxxxxxxxxx --- Comment #7 from Miro Hrončok <mhroncok@xxxxxxxxxx> --- (In reply to Michal Schorm from comment #4) > 1/ BuildRequire python3-devel [NEED-2ND-OPINION] > [NEED-2ND-OPINION] "Every package that uses Python (at runtime and/or build > time) and/or installs Python modules > MUST explicitly include BuildRequires: python3-devel in > its .spec file, > even if Python is not actually invoked during build time." > Reviewer note: > Your code: > BuildRequires: python%{python3_pkgversion}-devel > I believe the Guideline explicitly shows the code that is mandatory to be > included. And it is without macros. > _My understanding_ is that the pure following line MUST be used instead: > BuildRequires: python3-devel The guideline is meant "effectively", hence python%{python3_pkgversion}-devel is actually fine. > 2/ Naming [OK] > Package name: > container-workflow-tool > Canonical name: > | rpm --eval "%{py_dist_name container-workflow-tool}" > | container-workflow-tool > Main importable module name: > container_workflow_tool > > This package provides BOTH a tool (application) and the importable module. > Currently the primarly function as I see it is to provide the application, > so the following apply: > "Packages that primarily provide applications, services or any kind of > executables > SHOULD be named according to the general Fedora naming guidelines (e.g. > ansible)." Ack. > Also: > "Consider adding a virtual provide according to Library naming above > (e.g. python3-PROJECTNAME), if it would help users find the package." > Since the goal is _also_ to provide the importable module, it might be a > good idea to add such virtual provide. The subpakcage with the importable module is already named python3-container-workflow-tool, so this is basically done. The main package should not also provide python3-container-workflow-tool, or there would be an ambiguity. Note that I personally consider this split unnecessary, unless %{_bindir}/cwt is enormously big (which it isn't, it is merely a small entrypoint script). Also note that if the split is kept, the cwt package needs to require fully-versioned python3-container-workflow-tool = %{?epoch:%{epoch}:}%{version}-%{release}. > 3/ Files to include [OK] > > [OK] "Packages MUST include the source file (*.py) AND > the bytecode cache (*.pyc) for each pure-Python importable module." > [OK] "The source files MUST be included in the same package as the bytecode > cache." > [OK] "Scripts that are not importable (typically ones in %{_bindir} or > %{_libexecdir}) > SHOULD NOT be byte-compiled." > [OK] "The cache files are found in a __pycache__ directory and > have an interpreter-dependent suffix like .cpython-39.pyc." Ack. > 4/ Dist-info metadata [OK] > > [OK] "Each Python package MUST include Package Distribution Metadata > conforming > to PyPA specifications (specifically, Recording installed > distributons)." > [OK] "The metadata SHOULD be included in the same subpackage as > the main importable module, if there is one." Ack. > 5/ Explicit lists [FAIL] > [OK] "Packages MUST NOT own shared directories owned by Python itself" > [FAIL] "Packagers SHOULD NOT simply glob everything under a shared > directory." > Reviewer note: > the package ship a single manual page. Please list the file explicitly, > instead of: > | %{_mandir}/man1/* Ack. Although it is a SHOULD and not a MUST, so it is permitted to stay this way, but frowned upon. > 6/ PyPI parity [FAIL] > [FAIL] "Every Python package in Fedora SHOULD also be available on the > Python Package Index (PyPI)." > Reviewer note: > The attempt to make the project available via PyPI has yet not been made. > [OK] "The command pip install PROJECTNAME MUST install the same package > (possibly in a different version), install nothing, or fail with a > reasonable error message." > [FAIL] "If your package is not or cannot be published on PyPI, you can: > * Ask Python SIG to block the name on PyPI for you" > Reviewer note: > It is unclear whether the upstream would like to maintain the project in > PyPI, > however to avoid future name collisions, it seems wise to me to block the > name. > If the name collision would happen, you (maintainer) would be force to > change the package name AND > you would have to try to convince upstream to rename its projects, since > the names of the > upstream project and the Fedora package SHOULD match. > That is my understanding of the Python Naming Guidelines Correct. If upstream does not plan to release on PyPI, you should block this name on PyPI. You can ping pviktori@xxxxxxxxxx (encukou on #fedora-python IRC) to do that for you. If upstream is willing to release on PyPI "eventually", please coordinate with them to upload a pre-release or similar to reserve the name. I personally strongly suggest releasing on PyPI, since this is a Red Hat project, we have the power to do so. > 7/ Provides and requirements [FAIL] > | Requires > | -------- > | container-workflow-tool (rpmlib, GLIBC filtered): > | /usr/bin/python3 > | python3-container-workflow-tool > | python3-container-workflow-tool (rpmlib, GLIBC filtered): > | python(abi) > | python3.10dist(gitpython) > | python3.10dist(pyyaml) > | python3.10dist(requests-kerberos) > | Provides > | -------- > | container-workflow-tool: > | container-workflow-tool > | python3-container-workflow-tool: > | python-container-workflow-tool > | python3-container-workflow-tool > | python3.10-container-workflow-tool > | python3.10dist(container-workflow-tool) > | python3dist(container-workflow-tool) > [OK] "For any module intended to be used in Python 3 with import MODNAME, > the package that includes it SHOULD provide python3-MODNAME, > with underscores (_) replaced by dashes (-)." > [OK] "For any FOO, a package that provides python3-FOO SHOULD use > %py_provides > or an automatic generator to also provide python-FOO and python3.X-FOO, > where X is the minor version of the interpreter." > [OK] "The provide SHOULD NOT be added manually: if a generator or macro is > not used, > do not add the python-FOO / python3.X-FOO provides at all." > [OK] "Every Python package MUST provide python3dist(DISTNAME) and > python3.Xdist(DISTNAME), > where X is the minor version of the interpreter and > DISTNAME is the Canonical project name corresponding to the Dist-info > metadata." > [OK] "This is generated automatically from the dist-info metadata. > The provide SHOULD NOT be added manually: if the generator fails to add > it, the metadata MUST be fixed." > [FAIL] "Each Python package MUST explicitly BuildRequire python3-devel." > Reviewer note: > "BuildRequires: python%{python3_pkgversion}-devel" is used instead. That is fine, as said previously. > [OK] "Packages MUST NOT have dependencies (either build-time or runtime) > with the unversioned prefix python- if the corresponding python3- > dependency can be used instead." > [OK] "Packages SHOULD NOT have explicit dependencies (either build-time or > runtime) > with a minor-version prefix such as python3.8- or python3.8dist(" > [OK] "Packages SHOULD NOT have an explicit runtime dependency on python3." > [NEED-2ND-OPINION] "Packages MUST use the automatic Python run-time > dependency generator." > [OK] "Packages SHOULD use the opt-in build-dependency generator if possible." > [OK] "The packager MUST inspect the generated requires for correctness. > All dependencies MUST be resolvable within the targeted Fedora version." > [OK] "Dependencies covered by the generators SHOULD NOT be repeated in the > .spec file." > [NEED-2ND-OPINION] The project README.md states that: > | Requirements > | ------------ > | * python3 > | * python3-GitPython > | * python3-requests-kerberos > | * fedpkg > | * http://github.com/ficap/dhwebapi (installed using pip3 and > requirements.txt) > But I see those requirements neither generated nor explicitly specified. > Shouldn't they be listed? If the README is up to date, they should be listed, yes. > 8/ Interpreter invocation [NEED-2ND-OPINION] > [OK] "Shebang lines to invoke Python MUST use %{python3} as the interpreter." > [OK] "Shebang lines to invoke Python SHOULD be #!%{python3} > -%{py3_shebang_flags} and they MAY include extra flags." > Reviewer note: > macro "%pyproject_install" is used. > | # grep -i -e "^#!" -r review-container-workflow-tool/rpms-unpacked/* > | > review-container-workflow-tool/rpms-unpacked/container-workflow-tool-1.0.0-1. > fc36.noarch.rpm/usr/bin/cwt:#! /usr/bin/python3 -s > [NEED-2ND-OPINION] "Every executable TOOL for which the current version of > Python matters > SHOULD also be invokable by python3 -m TOOL." > "If the software doesn’t provide this functionality, > packagers SHOULD ask the upstream to add it." > Reviewer note: > I don't know how to check if the current version of Python matters. Does it matter for the user which Python version executes the tool? If the tool was written in Rust, it would need to be told (or detect/guess) what the Python version is to operate correctly? If the answers are yes and yes, the current version of Python doesn't matter. If the answers are no and no, the current version of Python doesn't matter. > 9/ Using Cython [OK] > [OK] "packages MUST NOT use files pre-generated by Cython" Ack, does not use Cython at all. > 10/ Tests [FAIL] > [FAIL] "If a test suite exists upstream, it SHOULD be run in the %check > section. > If that is not possible with reasonable effort, > at least a basic smoke test (such as importing the packaged module) > MUST be run in %check." > Reviewer note: > No test is currently run. > Even thought the justification why the upstream test are not run is OK, it > does NOT follow the guidelines. > You MUST run atleast a basic smoke test. Exactly. Since this is a CLI tool *and* an importable module, I suggest something like: %check PYTHONPATH=%{buildroot}%{python3_sitelib} %{buildroot}%{_bindir}/cwt --help %py3_check_import container_workflow_tool > [OK] "In %check, packages SHOULD NOT run “linters”: code style checkers, > test coverage checkers and other tools that check code quality rather > than functionality." Ack. > 11/ Source files from PyPI [OK] > Reviewer note: > None used. Ack. Extra notes by me: > %{?python_enable_dependency_generator} This is deprecated and not needed. > %global srcname container_workflow_tool I consider defining this macro overengineering. > # remove man pages from sitelib directory, where %%pyproject_install installed them > rm -r %{buildroot}%{python3_sitelib}/usr This should be fixed upstream. > 12/ LICENSE file > I do not see any usage of '%license' macro in the '%files' section, however > the License file is present in the package: > python3-container-workflow-tool-1.0.0-1.fc36.noarch.rpm/usr/lib/python3.10/site-packages/container_workflow_tool-1.0.0.dist-info/LICENSE Indeed. This is done by setuptools and on Fedora 35+ the file is detected by %pyproject_save_files and marked as %license. You can verify with: rpm -ql --licensefiles > 13/ I can see the YAML config files in [/usr] ... This depends on whether the user is supposed to edit those files. If no, everything is fine. If yes, they need to be in /etc and marked as configuration. -- 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=2003700 _______________________________________________ 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 on the list, report it: https://pagure.io/fedora-infrastructure
