[Bug 2176933] New: Review Request: python-accessible-pygments - Accessible pygments themes
https://bugzilla.redhat.com/show_bug.cgi?id=2176933
Bug ID: 2176933 Summary: Review Request: python-accessible-pygments - Accessible pygments themes Product: Fedora Version: rawhide Hardware: All OS: Linux Status: NEW Component: Package Review Severity: medium Priority: medium Assignee: nobody@fedoraproject.org Reporter: loganjerry@gmail.com QA Contact: extras-qa@fedoraproject.org CC: package-review@lists.fedoraproject.org Target Milestone: --- Classification: Fedora
Spec URL: https://jjames.fedorapeople.org/python-accessible-pygments/python-accessible... SRPM URL: https://jjames.fedorapeople.org/python-accessible-pygments/python-accessible... Fedora Account System Username: jjames Description: This package includes a collection of accessible themes for pygments based on different sources.
https://bugzilla.redhat.com/show_bug.cgi?id=2176933
Jerry James loganjerry@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Doc Type|--- |If docs needed, set a value
--- Comment #1 from Jerry James loganjerry@gmail.com --- The package has been updated to version 0.0.4. New URLs:
Spec URL: https://jjames.fedorapeople.org/python-accessible-pygments/python-accessible... SRPM URL: https://jjames.fedorapeople.org/python-accessible-pygments/python-accessible...
https://bugzilla.redhat.com/show_bug.cgi?id=2176933
--- Comment #2 from Jakub Kadlčík jkadlcik@redhat.com --- Created attachment 1954292 --> https://bugzilla.redhat.com/attachment.cgi?id=1954292&action=edit The .spec file difference from Copr build 5619344 to 5719189
https://bugzilla.redhat.com/show_bug.cgi?id=2176933
Miro Hrončok mhroncok@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |ASSIGNED CC| |mhroncok@redhat.com Flags| |fedora-review? Assignee|nobody@fedoraproject.org |mhroncok@redhat.com
https://bugzilla.redhat.com/show_bug.cgi?id=2176933
--- Comment #3 from Miro Hrončok mhroncok@redhat.com --- Spec sanity first. The spec is very tidy and sane. Suggestions bellow:
BuildRequires: %{py3_dist pygments}
This should probably come generated from upstream. The setup.py has:
install_requires=[ 'pygments >= 1.5' ],
So %pyproject_buildrequires probably already generates this (including >= 1.5).
Summary: Accessible pygments themes
This could be Summary: %{summary}
%pyproject_save_files a11y_pygments
If the importable module is called a11y_pygments, consider adding %py_provides python3-a11y-pygments as well. In the spirit of https://docs.fedoraproject.org/en-US/packaging-guidelines/Python/#_provides_...
export PYTHONPATH=%{buildroot}%{python3_sitelib}
You can use %{py3_test_envvars} instead, available on Fedora 38+ https://src.fedoraproject.org/rpms/python-rpm-macros/c/b6479253006cef572fb0c...
I can do backports if you want me to.
python3 test/run_tests.py
Please always use %{python3} instead of directly invoking python3. it makes no difference in Fedora, but it makes the spec portable to e.g. RHEL alternative Python version stacks.
%license LICENSE
This is most likely redundant, but I have not yet built the package to verify.
https://bugzilla.redhat.com/show_bug.cgi?id=2176933
--- Comment #4 from Jerry James loganjerry@gmail.com --- Thanks for the suggestions.
(In reply to Miro Hrončok from comment #3)
BuildRequires: %{py3_dist pygments}
This should probably come generated from upstream. The setup.py has:
install_requires=[ 'pygments >= 1.5' ],
So %pyproject_buildrequires probably already generates this (including >= 1.5).
It doesn't though. If I remove that, the build fails:
Requirement satisfied: setuptools >= 40.8 (installed: setuptools 65.5.1) Handling wheel from default build backend Requirement satisfied: wheel (installed: wheel 0.40.0) Traceback (most recent call last): File "/usr/lib/rpm/redhat/pyproject_buildrequires.py", line 532, in main generate_requires( File "/usr/lib/rpm/redhat/pyproject_buildrequires.py", line 439, in generate_requires generate_build_requirements(backend, requirements) File "/usr/lib/rpm/redhat/pyproject_buildrequires.py", line 289, in generate_build_requirements new_reqs = get_requires() ^^^^^^^^^^^^^^ File "/usr/lib/python3.11/site-packages/setuptools/build_meta.py", line 338, in get_requires_for_build_wheel return self._get_build_requires(config_settings, requirements=['wheel']) ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ File "/usr/lib/python3.11/site-packages/setuptools/build_meta.py", line 320, in _get_build_requires self.run_setup() File "/usr/lib/python3.11/site-packages/setuptools/build_meta.py", line 484, in run_setup self).run_setup(setup_script=setup_script) ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ File "/usr/lib/python3.11/site-packages/setuptools/build_meta.py", line 335, in run_setup exec(code, locals()) File "<string>", line 5, in <module> File "/builddir/build/BUILD/accessible-pygments-0.0.4/a11y_pygments/utils/utils.py", line 7, in <module> from pygments.formatters import HtmlFormatter ModuleNotFoundError: No module named 'pygments'
The binary package does have a Requires generated for pygments >= 1.5, on the other hand.
This could be Summary: %{summary}
Done.
If the importable module is called a11y_pygments, consider adding %py_provides python3-a11y-pygments as well. In the spirit of https://docs.fedoraproject.org/en-US/packaging-guidelines/Python/ #_provides_for_importable_modules
Done.
export PYTHONPATH=%{buildroot}%{python3_sitelib}
You can use %{py3_test_envvars} instead, available on Fedora 38+ https://src.fedoraproject.org/rpms/python-rpm-macros/c/ b6479253006cef572fb0cbe7ce001b3048f127b3?branch=rawhide
I can do backports if you want me to.
This package is only for F39+, so no backport needed. But using that fails:
+ PYTHONPATH=/builddir/build/BUILDROOT/python-accessible-pygments-0.0.4-1.fc39.x86_64/usr/lib64/python3.11/site-packages:/builddir/build/BUILDROOT/python-accessible-pygments-0.0.4-1.fc39.x86_64/usr/lib/python3.11/site-packages + PYTHONDONTWRITEBYTECODE=1 + PYTEST_ADDOPTS=' --ignore=/builddir/build/BUILD/accessible-pygments-0.0.4/.pyproject-builddir' + PYTEST_XDIST_AUTO_NUM_WORKERS=16 + /usr/bin/python3 test/run_tests.py Traceback (most recent call last): File "/builddir/build/BUILD/accessible-pygments-0.0.4/test/run_tests.py", line 9, in <module> from a11y_pygments.utils.utils import find_all_themes ModuleNotFoundError: No module named 'a11y_pygments'
That doesn't make any sense. The second element of PYTHONPATH is exactly what I was setting, isn't it?
Oh, I see. There are no export statements. In spite of the name of the macro, those are shell variables, not environment variables. If I simply add "export PYTHONPATH" after %py3_test_envvars, then it works.
Please always use %{python3} instead of directly invoking python3. it makes no difference in Fedora, but it makes the spec portable to e.g. RHEL alternative Python version stacks.
Done.
%license LICENSE
This is most likely redundant, but I have not yet built the package to verify.
You are correct; `rpm -qLp` lists the LICENSE file in the dist-info directory. I have removed this.
I have uploaded new versions. Due to %autorelease/%autochangelog, the URLs are the same. I wonder if I should use traditional Release numbering and changelogs while a package is going through review, then switch to %autorelease/%autochangelog before importing the package.
https://bugzilla.redhat.com/show_bug.cgi?id=2176933
--- Comment #5 from Miro Hrončok mhroncok@redhat.com ---
It doesn't though. If I remove that, the build fails: ...
I see. IMHO this is a bug in upstream packaging. The setup.py script has:
from a11y_pygments.utils.utils import find_all_themes_packages
Hence before we can execute setup.py, we already need to have install_requires installed.
A possible upstream fix (if you intend to persuade it) is to introduce a pyproject.toml file with pygments listed in [build-system].requires. Something like:
[build-system] requires = ["setuptools", "pygments >= 1.5"] build-backend = "setuptools.build_meta"
A better fix is not to import the package from setup.py, but that might be tricky.
(This might deserve a comment near the manual BR.)
There are no export statements. In spite of the name of the macro, those are shell variables, not environment variables.
The intended usage is:
%{py3_test_envvars} %{python3} test/run_tests.py
I had no idea that such variables are not considered "environment" when introduced the macro (and nobody who discussed or reviewed it had that idea neither). The macro will be documented in the guidelines. Do you think it needs to be renamed?
I wonder if I should use traditional Release numbering and changelogs while a package is going through review
No need. Updating the same link is fine for me.
https://bugzilla.redhat.com/show_bug.cgi?id=2176933
--- Comment #6 from Miro Hrončok mhroncok@redhat.com ---
The intended usage is:
%{py3_test_envvars} %{python3} test/run_tests.py
Or alternatively:
export %{py3_test_envvars}
https://bugzilla.redhat.com/show_bug.cgi?id=2176933
--- Comment #7 from Miro Hrončok mhroncok@redhat.com --- test/run_tests.py has no output. Is that expected?
https://bugzilla.redhat.com/show_bug.cgi?id=2176933
Miro Hrončok mhroncok@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |POST Flags|fedora-review? |fedora-review+
--- Comment #8 from Miro Hrončok mhroncok@redhat.com --- Package APPROVED.
Consider changing the use of %{py3_test_envvars} as described.
Package Review ==============
Legend: [x] = Pass, [!] = Fail, [-] = Not applicable, [?] = Not evaluated
===== 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. [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", "BSD 3-Clause License". [x]: Package contains no bundled libraries without FPC exception. [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]: Package is not known to require an ExcludeArch tag. [-]: Large documentation must go in a -doc subpackage. Large could be size (~1MB) or number of files. Note: Documentation size is 20480 bytes in 2 files. [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: No rpmlint messages. [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]: 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]: 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. [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. [-]: 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. [x]: %check is present and all tests pass. [?]: 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-accessible-pygments-0.0.4-1.fc39.noarch.rpm python-accessible-pygments-0.0.4-1.fc39.src.rpm ============================================= rpmlint session starts ============================================ rpmlint: 2.4.0 configuration: /usr/lib/python3.11/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/tmp_8y0iy0d')] checks: 31, packages: 2
============== 2 packages and 0 specfiles checked; 0 errors, 0 warnings, 0 badness; has taken 0.6 s =============
Rpmlint (installed packages) ---------------------------- ============================ rpmlint session starts ============================ rpmlint: 2.4.0 configuration: /usr/lib/python3.11/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: 31, packages: 1
1 packages and 0 specfiles checked; 0 errors, 0 warnings, 0 badness; has taken 0.0 s
Source checksums ---------------- https://github.com/Quansight-Labs/accessible-pygments/archive/v0.0.4/accessi... : CHECKSUM(SHA256) this package : c53231d81bf5baf9dde7e27e8894290c1012284543128f1d3c8c28ddd95dd2b4 CHECKSUM(SHA256) upstream package : c53231d81bf5baf9dde7e27e8894290c1012284543128f1d3c8c28ddd95dd2b4
BuildRequires ------------- pyproject-rpm-macros python3-devel python3dist(packaging) python3dist(pip) >= 19 python3dist(pygments) python3dist(pygments) >= 1.5 python3dist(setuptools) >= 40.8 python3dist(wheel)
Requires -------- python3-accessible-pygments (rpmlib, GLIBC filtered): python(abi) = 3.11 python3.11dist(pygments) >= 1.5
Provides -------- python3-accessible-pygments: python-a11y-pygments = 0.0.4-1.fc39 python-accessible-pygments = 0.0.4-1.fc39 python3-a11y-pygments = 0.0.4-1.fc39 python3-accessible-pygments = 0.0.4-1.fc39 python3.11-a11y-pygments = 0.0.4-1.fc39 python3.11-accessible-pygments = 0.0.4-1.fc39 python3.11dist(accessible-pygments) = 0.0.4 python3dist(accessible-pygments) = 0.0.4
Generated by fedora-review 0.7.0 (fed5495) last change: 2019-03-17 Command line :try-fedora-review -b 2176933 -m fedora-rawhide-x86_64 Buildroot used: fedora-rawhide-x86_64 Active plugins: Shell-api, Python, Generic Disabled plugins: PHP, R, Haskell, Ocaml, Ruby, Java, SugarActivity, Perl, C/C++, fonts Disabled flags: EPEL6, EPEL7, DISTTAG, BATCH, EXARCH
https://bugzilla.redhat.com/show_bug.cgi?id=2176933
--- Comment #9 from Jerry James loganjerry@gmail.com --- (In reply to Miro Hrončok from comment #5)
I see. IMHO this is a bug in upstream packaging. The setup.py script has:
from a11y_pygments.utils.utils import find_all_themes_packages
Hence before we can execute setup.py, we already need to have install_requires installed.
A possible upstream fix (if you intend to persuade it) is to introduce a pyproject.toml file with pygments listed in [build-system].requires. Something like:
[build-system] requires = ["setuptools", "pygments >= 1.5"] build-backend = "setuptools.build_meta"
A better fix is not to import the package from setup.py, but that might be tricky.
(This might deserve a comment near the manual BR.)
I will speak with upstream about this. Thanks for the diagnosis.
The intended usage is:
%{py3_test_envvars} %{python3} test/run_tests.py
I had no idea that such variables are not considered "environment" when introduced the macro (and nobody who discussed or reviewed it had that idea neither). The macro will be documented in the guidelines. Do you think it needs to be renamed?
Ah, I misunderstood. When used like that, they are environment variables. If the documentation says to use the macro that way, then the name is fine.
(In reply to Miro Hrončok from comment #7)
test/run_tests.py has no output. Is that expected?
The test regenerates the output in the test/results directory. We could save the original output, then compare the two. However, they are not the same, because we have a newer version of pygments than was used to generate the results packaged by upstream. So all we are testing is that this package can be imported and used to generate output. Whether the output is actually correct is not tested. I will make a comment about that in the spec file.
I have updated the URLs with the version I intend to import. Thank you for the review!
https://bugzilla.redhat.com/show_bug.cgi?id=2176933
--- Comment #10 from Fedora Admin user for bugzilla script actions fedora-admin-xmlrpc@fedoraproject.org --- The Pagure repository was created at https://src.fedoraproject.org/rpms/python-accessible-pygments
https://bugzilla.redhat.com/show_bug.cgi?id=2176933
Jerry James loganjerry@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Resolution|--- |RAWHIDE Status|POST |CLOSED Fixed In Version| |python-accessible-pygments- | |0.0.4-1.fc39 Last Closed| |2023-03-30 03:40:07
https://bugzilla.redhat.com/show_bug.cgi?id=2176933
--- Comment #11 from Miro Hrončok mhroncok@redhat.com --- FYI %{py3_test_envvars} documentation PR in https://pagure.io/packaging-committee/pull-request/1267
https://bugzilla.redhat.com/show_bug.cgi?id=2176933
--- Comment #12 from Jerry James loganjerry@gmail.com --- Thanks. That is well written and makes perfect sense. I'll check my spec files to see if I can use that elsewhere.
package-review@lists.fedoraproject.org
-
bugzilla＠redhat.com