Fedora Account System
Red Hat Associate
Red Hat Customer
Spec URL: https://jjelen.fedorapeople.org/python-proxy-py.spec SRPM URL: https://jjelen.fedorapeople.org/python-proxy-py-2.4.10-1.fc46.src.rpm Description: proxy.py is a fast, lightweight, pluggable proxy server written in Python with no external dependencies. Fedora Account System Username: jjelen
Hey Jakub, Proxy.py is a tox-centric project that's also packaged with PEP 517. Could you use https://src.fedoraproject.org/rpms/pyproject-rpm-macros and change the direct pytest invocation with %tox per its docs? Modern guidelines recommend it — it's more standardized/universal, and it's my understanding that new things following accepted packaging standards should use them. The guidelines also have this example as well: https://docs.fedoraproject.org/en-US/packaging-guidelines/Python/#_example_spec_file. Also, I imagine, `python-aiohttp` should add a dep on `python3dist(proxy-py)` once it's packaged, since it references it in this comment: https://src.fedoraproject.org/rpms/python-aiohttp/blob/rawhide/f/python-aiohttp.spec#_131. But it's patched out above. Perhaps, Miro has a better context on the Python packaging preferences, though so I'll CC him.
The spec uses pyproject-rpm-macros (maybe it was already updated, not sure). Using %tox would hit the same testign extra problem documented in the spec near the test BuildRequires.
This is the part I don't fully understand: -e "/; python_version < '3.11'/d" \ -e "/; python_version <= '3.6'/d" \ -e "/; python_version < '3.8'/d" \ Such requirements are not relevant to our build on Python 3.12+, why sed them out? Other than that and some possibly-upstreamable patches/seds, the spec file is very sane.
> The spec uses pyproject-rpm-macros (maybe it was already updated, not sure). Using %tox would hit the same testign extra problem documented in the spec near the test BuildRequires. I did not change it. This was the initial submission. > This is the part I don't fully understand: Thank you for double-checking. Yes, you are right, these are not needed. They were added by claude and I did not double-check they make sense.
Updated both spec file and srpm: Spec URL: https://jjelen.fedorapeople.org/python-proxy-py.spec SRPM URL: https://jjelen.fedorapeople.org/python-proxy-py-2.4.10-1.fc46.src.rpm
(In reply to Miro Hrončok from comment #2) > The spec uses pyproject-rpm-macros (maybe it was already updated, not sure). > Using %tox would hit the same testign extra problem documented in the spec > near the test BuildRequires. Okay, then. But I see there's a `sed` invocation that replaces the `==` entries. Wouldn't `%tox` work after that's done? ------- LGTM, otherwise, I guess. Could you use the long argument names for `sed -i -e`, though?
The comment literally says: > Test dependencies; the upstream "testing" extra pins exact versions and pulls in linters and release tooling, so it is not used here The sed invocation that replaces the `==` entries only solves half of the problem. The testing extra lists stuff like tox itself, mccabe, pylint, pre-commit, autopep8... total 36 packages. Manually listing 6 (a sixth of the total) is more sane than filtering a mojority of them them away.
$ repoquery -q --repo=rawhide -f /usr/bin/proxy libproxy-bin-0:0.5.12-3.fc45.x86_64 You need an explicit RPM Conflict with this ^ See https://docs.fedoraproject.org/en-US/packaging-guidelines/Conflicts/#_implicit_conflicts (Or rename/move it if applicable.) I coexistence with libproxy-bin is anticipated on one system and renaming is not desired, splitting the binary to python3-proxy-py-bin subpackage and keeping that conflict only there is also an option. -------------------------------------- Other than that, all looks good: python3-proxy-py-2.4.10-1.fc46.noarch.rpm: Provides: python-proxy-py = 2.4.10-1.fc46 python3-proxy-py = 2.4.10-1.fc46 python3.15-proxy-py = 2.4.10-1.fc46 python3.15dist(proxy-py) = 2.4.10 python3dist(proxy-py) = 2.4.10 Requires: /usr/bin/python3 python(abi) = 3.15 Recommends: openssl python3-proxy-py+metrics-2.4.10-1.fc46.noarch.rpm: Provides: python-proxy-py+metrics = 2.4.10-1.fc46 python3-proxy-py+metrics = 2.4.10-1.fc46 python3.15-proxy-py+metrics = 2.4.10-1.fc46 python3.15dist(proxy-py[metrics]) = 2.4.10 python3dist(proxy-py[metrics]) = 2.4.10 Requires: python(abi) = 3.15 python3-proxy-py = 2.4.10-1.fc46 python3.15dist(prometheus-client) >= 0.20 python3-proxy-py+tunnel-2.4.10-1.fc46.noarch.rpm: Provides: python-proxy-py+tunnel = 2.4.10-1.fc46 python3-proxy-py+tunnel = 2.4.10-1.fc46 python3.15-proxy-py+tunnel = 2.4.10-1.fc46 python3.15dist(proxy-py[tunnel]) = 2.4.10 python3dist(proxy-py[tunnel]) = 2.4.10 Requires: python(abi) = 3.15 python3-proxy-py = 2.4.10-1.fc46 python3.15dist(cryptography) >= 39.0.1 python3.15dist(paramiko) >= 3.4 ------------------------------------ rpmlint: python-proxy-py.src: E: spelling-error ('proxying', '%description -l en_US proxying -> Procyon, peroxiding, prophesying') python-proxy-py.src: E: spelling-error ('acceptor', '%description -l en_US acceptor -> accept, accept or, accept-or') python3-proxy-py.noarch: E: spelling-error ('proxying', '%description -l en_US proxying -> Procyon, peroxiding, prophesying') python3-proxy-py.noarch: W: no-manual-page-for-binary proxy 4 packages and 1 specfiles checked; 3 errors, 1 warnings, 29 filtered, 3 badness; has taken 0.9 s The spelling errors are false positives - both are valid technical terms used correctly in context. "Proxying" refers to HTTP proxying functionality, and "acceptor" refers to the work acceptor framework pattern. no-manual-page-for-binary is meh unless upstream has one (I don't see any). Tests executed successfully: ====================== 226 passed, 6 deselected in 11.41s ====================== No bundled libraries, minified JavaScript, or vendored dependencies found in source tree. Python source files include BSD license reference (e.g., `proxy/__init__.py` header states `:license: BSD, see LICENSE for more details.`) License field declares `license = 'BSD'` and points to `LICENSE.md` file (corrected to `LICENSE` by spec file patch), this is BSD-3-Clause.
> LGTM, otherwise, I guess. Could you use the long argument names for `sed -i -e`, though? Thank you! Done that, though not sure if it helps much with the readability. > I coexistence with libproxy-bin is anticipated on one system and renaming is not desired, splitting the binary to python3-proxy-py-bin subpackage and keeping that conflict only there is also an option. Thank you. I decided to split the binary as I would expect this should get used a standalone python development module by default and if somebody needs the binary, they will have to resolve the conflicts on their own (decide if they need libproxy-bin or this one). Spec URL: https://jjelen.fedorapeople.org/python-proxy-py.spec SRPM URL: https://jjelen.fedorapeople.org/python-proxy-py-2.4.10-6.fc46.src.rpm
Consider adding `Recommends: python3-proxy-py-bin` (without version) to the python3-proxy-py package, so users would get it by default. Also note that due to the split, if other packages BuildRequire this via %pyproject_buildrequires, they won't get the CLI. This can eventually bite some dependent package (see e.g. https://src.fedoraproject.org/rpms/python-docs-theme/pull-request/22#comment-340631 where exactly that happened with similarly split /usr/bin/babel). ---- >> LGTM, otherwise, I guess. Could you use the long argument names for `sed -i -e`, though? > > Thank you! Done that, though not sure if it helps much with the readability. If you like that, you may also use long options for %pyproject_buildrequires -x/--extras and %pyproject_save_files -l/--assert-license (not yet documented in the Python packaging guidelines). ---- Anyway, package APPROVED.
The Pagure repository was created at https://src.fedoraproject.org/rpms/python-proxy-py Monitoring: Wasn't able to create project in Anitya. You can create it manually on: `https://release-monitoring.org`
Upstream release monitoring added to https://release-monitoring.org/project/26988/
And thank you for the review!
(In reply to Jakub Jelen from comment #9) > > LGTM, otherwise, I guess. Could you use the long argument names for `sed -i -e`, though? > > Thank you! Done that, though not sure if it helps much with the readability. This is sort of a tribal knowledge thing, but in general, using long flag names allows lowering the cognitive burden for the reader (especially someone from a different bubble) having to look up the meaning/use search/manpages. Here's an example of this argument being made for scripts: https://changelog.com/posts/use-long-flags-when-scripting. In general, short flags are for interactive use. One can remember what they mean having just looked them up, but other people would need to learn that context, so designing scripts for the reviewers/readers is a nice gesture in general. This is similar to using appropriate variable names in "adult" programming languages to label data instead of "a", "b", "c". In many contexts, using long arguments helps improve scripts not only in Bash but also in Python, because `f'--flag={value}'` allows fighting formatters splitting strings in inconvenient places. So it's more universal than just spec files and pure bash. I've seen this being practiced in some of the ecosystem and this spec file, for example: https://src.fedoraproject.org/rpms/python-aiohttp/blob/rawhide/f/python-aiohttp.spec#_95. I can say that before seeing it I never knew what `-e` stands for. It definitely helps with readability in more ways than one. ------- (In reply to Miro Hrončok from comment #10) > If you like that, you may also use long options for %pyproject_buildrequires > -x/--extras and %pyproject_save_files -l/--assert-license (not yet > documented in the Python packaging guidelines). Nice! Thanks for sharing, I may end up using these elsewhere..