Modernize RPM packaging for EL9+/Fedora 43+, drop EL7/python2, refresh Python syntax - #98
Open
mouchar wants to merge 6 commits into
Open
Modernize RPM packaging for EL9+/Fedora 43+, drop EL7/python2, refresh Python syntax#98mouchar wants to merge 6 commits into
mouchar wants to merge 6 commits into
Conversation
%py3_build and %py3_install are deprecated and slated for removal in Fedora 45. Switch the non-EL7 build path to %pyproject_wheel and %pyproject_install, driving %files via %pyproject_save_files instead of hardcoded python3_sitelib paths. EL7 keeps the python2 macros since pyproject-rpm-macros isn't available there. Also add a %changelog entry so %source_date_epoch_from_changelog has something to read.
Target EL9+ and Fedora 43+ only, where python3 is always >= 3.9. Removes the now-dead %py2_build/%py2_install/%python2 branches and the python2_sitelib %files entries left over from the EL7 era.
License: BSD-3-Clause matches setup.py's SPDX identifier instead of the bare "BSD" string. python3dist(mock) is no longer needed since tests/server/test_client.py now uses stdlib unittest.mock.
- Shebang: python -> python3, drop the now-unneeded coding declaration (Python 3 defaults source files to UTF-8). - Drop the dead "test_suite" distutils option; setuptools no longer recognizes it (it warned "Unknown distribution option") and tests already run via pytest per the Makefile/tox.ini. - Replace the "License :: OSI Approved :: BSD License" classifier and bare "BSD" license string with the SPDX identifier "BSD-3-Clause", avoiding setuptools' license-classifier deprecation warning. - Add python_requires=">=3.9" to declare the actual support floor.
Mechanical cleanup found while auditing the codebase for syntax that pre-dates the Python 3.9 floor: - Drop the "# -*- coding: utf-8 -*-" magic comment everywhere; Python 3 source files default to UTF-8 (PEP 3120), so it's dead weight. - Drop "#!/usr/bin/env python" shebangs from library modules that were never executable in the first place; the three actual entry points (bin/*.py) now say "python3" explicitly, matching hosts that have no bare "python" on PATH. - Drop the redundant explicit "(object)" base class (implicit since Python 3). - Replace old-style super(ClassName, self) calls with bare super(). Two real latent bugs surfaced along the way: - smoker/util/tap.py and smoker/util/progressbar.py had Python 2 `print` statements inside their usage-example docstrings; fixed to call syntax so anyone copy-pasting the example gets working code. - smoker/server/plugins/varnishparser.py built its regex from a non-raw string with backslash-space escapes, which is only valid today via CPython's SyntaxWarning-emitting fallback and will become a hard SyntaxError in a future Python. Made it a raw string. Also swap tests/server/test_client.py's third-party "mock" import for stdlib unittest.mock (available since Python 3.3), dropping the external test dependency (see requirements-test.txt). Verified: `pytest -q` still passes 115/115 before and after.
Smoker now targets Python 3.9+ on EL9+/Fedora 43+ (see recent spec and setup.py changes), so update the stale Python 2.6.6-era instructions: - Build deps: yum -> dnf, drop python2-setuptools, add the pyproject-rpm-macros toolchain smoker.spec now requires. - Point pip installs at requirements.txt/requirements-test.txt instead of a hand-maintained package list that had already drifted (missing PyYAML's real name, listing argparse which is stdlib, and mock/lockfile which aren't actual dependencies). - git:// clone URL -> git+https:// (git:// is no longer served by GitHub). - py.test -> pytest (the py.test alias is long gone).
📝 WalkthroughWalkthroughThis PR modernizes the codebase for Python 3, removing legacy shebangs, encoding declarations, explicit ChangesPython 3 modernization
Estimated code review effort: 2 (Simple) | ~15 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@README.md`:
- Around line 55-74: Fix the typo in the README wording by changing “bellow” to
“below” in the dependencies/install instructions text. Update the sentence in
the README section around the Smoker setup guidance so the phrasing is correct
and consistent with the rest of the documentation.
In `@smoker.spec`:
- Around line 44-58: The package manifest in smoker.spec is missing the license
entry in the %files section. Update the %files block alongside %pyproject_files
to include LICENSE.TXT using the proper %license tag, while keeping the existing
%doc etc/* entry unchanged.
🪄 Autofix (Beta)
❌ Autofix failed (check again to retry)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c79226c2-13b1-4215-b3fb-0cb57fcace5b
📒 Files selected for processing (45)
README.mdbin/check_smoker_plugin.pybin/smokercli.pybin/smokerd.pyrequirements-test.txtsetup.pysmoker.specsmoker/__init__.pysmoker/client/__init__.pysmoker/client/cli.pysmoker/client/out_junit/__init__.pysmoker/client/out_junit/default_config.pysmoker/client/out_junit/rows.pysmoker/client/out_junit/xml_builder.pysmoker/client/plugins/__init__.pysmoker/logger/__init__.pysmoker/logger/level_handler.pysmoker/server/__init__.pysmoker/server/daemon.pysmoker/server/exceptions.pysmoker/server/parser.pysmoker/server/plugins/__init__.pysmoker/server/plugins/fsmount.pysmoker/server/plugins/glusterfs.pysmoker/server/plugins/mongo.pysmoker/server/plugins/testconnection.pysmoker/server/plugins/uname.pysmoker/server/plugins/varnishparser.pysmoker/server/restserver.pysmoker/util/__init__.pysmoker/util/command.pysmoker/util/console.pysmoker/util/nagios.pysmoker/util/progressbar.pysmoker/util/tap.pytests/__init__.pytests/server/__init__.pytests/server/smoker_test_resources/__init__.pytests/server/smoker_test_resources/client_mock_result.pytests/server/smoker_test_resources/smokermodule.pytests/server/smoker_test_resources/smokerparser.pytests/server/test_client.pytests/server/test_daemon.pytests/server/test_plugins.pytests/server/test_restapi.py
💤 Files with no reviewable changes (24)
- smoker/server/plugins/glusterfs.py
- smoker/logger/level_handler.py
- smoker/server/plugins/mongo.py
- tests/server/smoker_test_resources/smokermodule.py
- smoker/client/out_junit/rows.py
- smoker/server/plugins/fsmount.py
- smoker/server/plugins/uname.py
- tests/server/init.py
- tests/init.py
- smoker/init.py
- smoker/client/out_junit/default_config.py
- requirements-test.txt
- smoker/util/nagios.py
- tests/server/smoker_test_resources/client_mock_result.py
- smoker/server/exceptions.py
- smoker/server/init.py
- smoker/client/out_junit/init.py
- tests/server/smoker_test_resources/smokerparser.py
- tests/server/smoker_test_resources/init.py
- smoker/logger/init.py
- smoker/util/console.py
- smoker/client/cli.py
- smoker/server/plugins/testconnection.py
- smoker/util/init.py
Comment on lines
+55
to
+74
| Smoker requires Python 3.9 or newer, and is built and tested on EL9+ and Fedora 43+. | ||
|
|
||
| To build smoker with 'make rpm' folowing packages required to be installed on the CentOS, RHEL or Fedora: | ||
| To build smoker with 'make rpm' following packages are required on EL9+ or Fedora: | ||
|
|
||
| yum install gcc rpm-build python-setproctitle python-flask-restful python2-setuptools | ||
| dnf install gcc rpm-build rpmdevtools redhat-rpm-config systemd-rpm-macros \ | ||
| python3-devel python3-setuptools python3-pip python3-wheel pyproject-rpm-macros | ||
|
|
||
| It doesn't have much dependencies, follow instructions bellow to install them: | ||
|
|
||
| With PIP: | ||
|
|
||
| pip install psutil PyAML argparse setproctitle Flask-RESTful | ||
| pip install -r requirements.txt | ||
|
|
||
| Or install packages from your distribution repository. | ||
|
|
||
| ### Smoker app | ||
|
|
||
| With PIP (from Github): | ||
|
|
||
| pip install -e 'git://github.com/gooddata/smoker.git#egg=smoker' | ||
| pip install -e 'git+https://github.com/gooddata/smoker.git#egg=smoker' |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Typo: "bellow" → "below".
Line 62: It doesn't have much dependencies, follow instructions bellow to install them: — "bellow" should be "below".
✏️ Proposed fix
-It doesn't have much dependencies, follow instructions bellow to install them:
+It doesn't have much dependencies, follow instructions below to install them:📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| Smoker requires Python 3.9 or newer, and is built and tested on EL9+ and Fedora 43+. | |
| To build smoker with 'make rpm' folowing packages required to be installed on the CentOS, RHEL or Fedora: | |
| To build smoker with 'make rpm' following packages are required on EL9+ or Fedora: | |
| yum install gcc rpm-build python-setproctitle python-flask-restful python2-setuptools | |
| dnf install gcc rpm-build rpmdevtools redhat-rpm-config systemd-rpm-macros \ | |
| python3-devel python3-setuptools python3-pip python3-wheel pyproject-rpm-macros | |
| It doesn't have much dependencies, follow instructions bellow to install them: | |
| With PIP: | |
| pip install psutil PyAML argparse setproctitle Flask-RESTful | |
| pip install -r requirements.txt | |
| Or install packages from your distribution repository. | |
| ### Smoker app | |
| With PIP (from Github): | |
| pip install -e 'git://github.com/gooddata/smoker.git#egg=smoker' | |
| pip install -e 'git+https://github.com/gooddata/smoker.git#egg=smoker' | |
| Smoker requires Python 3.9 or newer, and is built and tested on EL9+ and Fedora 43+. | |
| To build smoker with 'make rpm' following packages are required on EL9+ or Fedora: | |
| dnf install gcc rpm-build rpmdevtools redhat-rpm-config systemd-rpm-macros \ | |
| python3-devel python3-setuptools python3-pip python3-wheel pyproject-rpm-macros | |
| It doesn't have much dependencies, follow instructions below to install them: | |
| With PIP: | |
| pip install -r requirements.txt | |
| Or install packages from your distribution repository. | |
| ### Smoker app | |
| With PIP (from Github): | |
| pip install -e 'git+https://github.com/gooddata/smoker.git#egg=smoker' |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@README.md` around lines 55 - 74, Fix the typo in the README wording by
changing “bellow” to “below” in the dependencies/install instructions text.
Update the sentence in the README section around the Smoker setup guidance so
the phrasing is correct and consistent with the rest of the documentation.
Comment on lines
+44
to
58
| %pyproject_wheel | ||
|
|
||
| %install | ||
| %if 0%{?el7} | ||
| %py2_install | ||
| %else | ||
| %py3_install | ||
| %endif | ||
| %pyproject_install | ||
| %pyproject_save_files smoker | ||
| install -d %{buildroot}%{_unitdir} | ||
| install -pm644 -t %{buildroot}%{_unitdir} smokerd.service | ||
|
|
||
| %if %{with check} | ||
| %check | ||
| %if 0%{?el7} | ||
| %python2 -m pytest -vv | ||
| %else | ||
| %python3 -m pytest -vv | ||
| %endif | ||
| %endif | ||
|
|
||
| %files | ||
| %files -f %{pyproject_files} | ||
| %doc etc/* |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check for a LICENSE file at the repo root and whether it's referenced anywhere in packaging files
fd -i '^license' -d 2
rg -n '%license' smoker.spec
rg -n 'license' setup.py -iRepository: gooddata/smoker
Length of output: 199
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '== smoker.spec ==\n'
cat -n smoker.spec | sed -n '1,220p'
printf '\n== packaging files mentioning license ==\n'
rg -n -i 'license|LICENSE' -S pyproject.toml setup.py smoker.spec . || true
printf '\n== top-level files ==\n'
fd -d 1 . .Repository: gooddata/smoker
Length of output: 3878
Add %license LICENSE.TXT to %files — LICENSE.TXT is present at the repo root, but the spec only ships %doc etc/*. The BSD-3-Clause text should be tagged separately.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@smoker.spec` around lines 44 - 58, The package manifest in smoker.spec is
missing the license entry in the %files section. Update the %files block
alongside %pyproject_files to include LICENSE.TXT using the proper %license tag,
while keeping the existing %doc etc/* entry unchanged.
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. An unexpected error occurred while generating fixes: Not Found - https://docs.github.com/rest/git/refs#get-a-reference |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
%py3_build/%py3_installmacros to%pyproject_wheel/%pyproject_install, driving%filesvia%pyproject_save_files.%py2_build/%py2_install/%python2branches.BSD-3-Clauseidentifier forLicense(spec and setup.py) instead of the bare"BSD"string, and drop the now-deprecatedLicense ::classifier.setup.py:python3shebang, drop the unneeded# -*- coding: utf-8 -*-declaration, remove the deadtest_suiteoption (setuptools no longer recognizes it — it was emittingUnknown distribution optionwarnings), addpython_requires=">=3.9".(object)base classes, old-stylesuper(ClassName, self)calls.printstatements inside usage-example docstrings (smoker/util/tap.py,smoker/util/progressbar.py), and a non-raw regex string with invalid escape sequences insmoker/server/plugins/varnishparser.pythat only worked via CPython'sSyntaxWarning-emitting fallback.mocktest dependency with stdlibunittest.mock, droppingmockfromrequirements-test.txtandpython3dist(mock)from the spec's%checkBuildRequires.README.md's install/build/test instructions to match (Python 3.9+, EL9+/Fedora 43+,dnf, current pip requirements files,pytestinstead ofpy.test,git+https://clone URL).Test plan
pytest -qpasses 115/115 before and after the changes (no regressions)python -m py_compileon every tracked.pyfile with-Werror::SyntaxWarning(clean)make rpmbuilt successfully on a Fedora 44 host with zero rpmbuild warnings (previously two%py3_build/%py3_installdeprecation warnings plus a%source_date_epoch_from_changelogwarning)rpm -qlp) still ship the three/usr/bin/*.pyscripts alongside the newdist-infometadata, no duplicate file listings🤖 Generated with Claude Code
Summary by CodeRabbit
Documentation
Bug Fixes
pytest-based workflows.Chores