RFC: Move package configuration from setup.py to pyproject.toml - #3924
RFC: Move package configuration from setup.py to pyproject.toml#3924YvanY0 wants to merge 5 commits into
Conversation
YvanY0
changed the title
cef6d5e to
2084547
Compare
YvanY0
changed the title
60f8949 to
c0dd58a
Compare
|
Hello @luckyh @clebergnu @richtja, as I can see directly invoke @clebergnu I also saw the avocado issue avocado-framework/avocado#5754, as VT is a plugin of avocado, so I am happy to listen to your voice, not sure what's the plan for avocado. I also tried to migrate avocado to hatch, but the setup.py has some complex custom targets, so I only modified some basic settings. |
b99746d to
eafc079
Compare
75e987f to
8a2f61c
Compare
2b65a86 to
b205883
Compare
YvanY0
changed the title
| "packaging", | ||
| "six", | ||
| "aexpect", | ||
| "avocado-framework>=82.1", |
There was a problem hiding this comment.
Let's check if that would require a higher version of Avocado due to the python compatibility change.
There was a problem hiding this comment.
There was a problem hiding this comment.
@richtja What is the situation with a formal pyproject.toml support on the Avocado side? @PaulYuuu I already have one avocado plugin that has been fully migrated and had no problem installing it and using it with avocado on an as-is basis. So I suspect Avocado is not a blocker for this migration in any way.
There was a problem hiding this comment.
@pevogam Yes, that should be right. but considering this is a big milestone, so let's consider it a little more, we should also clarify which version we want to support after switching to pyproject.
About avocado, it's not a easy task. the main setup.py contains many logic to support optional plugins, we need to decouple them. I've thought optional-dependencies probably a right implementation and I have asked deepwiki to check if this is possible. You can have a look of this: https://deepwiki.com/search/avocado-is-using-setuppy-i-hop_4fd8c6a1-b483-462b-ac49-e3e25367ef40
There was a problem hiding this comment.
Hi @PaulYuuu and @pevogam, currently on the avocado side we haven't started with the pyproject.toml support. The main issue is that for spawners, we need a mechanism of bootstrapping itself into the spawner environment. We currently use python eggs and the python wheels a currently not sufficient for our usecase. Therefore, we can't move to pyproject.toml until we will resolve this. More info in avocado-framework/avocado#6108
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
WalkthroughThis pull request migrates avocado-vt from legacy setup.py packaging to modern tooling: adds pyproject.toml with setuptools/setuptools-scm and declares project metadata, dependencies, package-data, console script, and multiple Avocado plugin entry points. Build and CI scripts (Makefile, Makefile.include, spec, Debian packaging, and GitHub Actions) switch to pip/build-based workflows and SCM-derived versioning. MANIFEST.in pruning and minor syntactic edits (trailing commas) appear across Python files; test helper code switches setup.py installs to pip installs. Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Key changes observed
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (3)
🔇 Additional comments (1)
✏️ Tip: You can disable this entire section by setting Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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
🔭 Outside diff range comments (2)
virttest/qemu_monitor.py (2)
2527-2535: SyntaxError – trailing comma after**kwargsmakes the file uncompilable
def block_stream(..., **kwargs,):is not valid Python syntax.
Unlike*args,**kwargscannot be followed by a comma in a function signature. Importing this module under any supported Python version will raise aSyntaxError, breaking the entire package.- **kwargs, + **kwargs)Apply the same fix to the closing parenthesis that follows.
3616-3622: Same SyntaxError introduced hereThe identical issue exists in
def block_export_add(..., **kwargs,):. This also prevents the module from loading.- **kwargs, + **kwargs)Please run
python -m py_compile virttest/qemu_monitor.pyafter fixing to ensure the file parses correctly.
🧹 Nitpick comments (5)
virttest/migration.py (2)
331-345: Trailing comma after**argsis syntactically fine, but consider renaming to**kwargsfor clarityPython 3 allows a trailing comma after
**kwargs, so the change is safe.
However, using the generic name**argsfor keyword-only arguments is atypical and can confuse readers who expect*args/**kwargssemantics.No functional change required, but you may want to align with common conventions:
- **args, + **kwargs,(Same remark applies to the inner helper below.)
556-569: Same naming / style remark as aboveEverything compiles, but
**args→**kwargswould make the intent consistent with idiomatic Python and with the outer wrapper.Makefile (2)
57-59: Re-evaluate the hard-coded--userflag
python -m pip install --user -e .installs into the per-user site-packages directory.
Inside a virtual-env or a build container this is unnecessary (and may even break isolation, e.g. with--system-site-packages=false). Consider:
- Delegating to the active environment and omitting
--user, or- Detecting a venv (
$VIRTUAL_ENV,sys.prefix) before appending--user.- $(PYTHON) -m pip install --user -e . + $(PYTHON) -m pip install -e . # rely on active env; add --user only when appropriate
71-75: Pin build-time tooling and avoid re-installing it every runInstalling
buildon every invocation is slow and gives nondeterministic results when a new major version lands.- $(PYTHON) -m pip install build + $(PYTHON) -m pip install --upgrade --require-hashes build==0.11.0 wheel==0.43.0• Pins versions → reproducible builds
• Addswheel, whichpython -m buildexpects when creating wheels.
At minimum add the--upgradeflag to prevent multiple copies of the same package in different site-dirs.pyproject.toml (1)
28-34: Consider version pinning (or lower bounds) for runtime dependenciesNone of the listed packages has a minimum version. When users install this plugin in an environment that already provides, say,
avocado-framework 84.x, the resolver will happily keep that older version and break the ≥89.0 assumptions encoded elsewhere (see comment thread).At a minimum:
dependencies = [ "netifaces>=0.11", "packaging>=21.0", "six>=1.16", "aexpect>=1.0", "avocado-framework>=89.0", ]Pinning major versions (or adding
~=,^) keeps the resolver honest without being overly strict.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
.github/workflows/ci.yml(0 hunks)MANIFEST.in(0 hunks)Makefile(4 hunks)Makefile.include(1 hunks)avocado-plugins-vt.spec(2 hunks)contrib/packages/debian/Makefile(1 hunks)pyproject.toml(1 hunks)requirements.txt(0 hunks)setup.py(0 hunks)virttest/gluster.py(1 hunks)virttest/libvirt_xml/accessors.py(1 hunks)virttest/migration.py(3 hunks)virttest/qemu_monitor.py(2 hunks)virttest/utils_test/__init__.py(2 hunks)virttest/virt_admin.py(1 hunks)
💤 Files with no reviewable changes (4)
- MANIFEST.in
- .github/workflows/ci.yml
- requirements.txt
- setup.py
🧰 Additional context used
🪛 checkmake (0.2.2)
Makefile
[warning] 20-20: Target body for "all" exceeds allowed length of 5 (21).
(maxbodylength)
[warning] 62-62: Target body for "unlink" exceeds allowed length of 5 (7).
(maxbodylength)
[warning] 60-60: Target "link" should be declared PHONY.
(phonydeclared)
🔇 Additional comments (10)
virttest/libvirt_xml/accessors.py (1)
239-245: Trailing-comma addition is harmless – change looks fine
The extra comma after**self.dargsis syntactically valid for all supported Python versions when used in a call expression, so no behavioural impact.virttest/gluster.py (1)
505-512: No change needed: trailing comma is supported (Python ≥3.9)
The project’srequires-python = ">=3.9"in pyproject.toml means you’re already on Python 3.9+, where a trailing comma after**kwargsis valid. You can safely keep the comma.virttest/virt_admin.py (1)
904-911: No changes needed for trailing comma after**dargs
The project’spyproject.tomlspecifiesrequires-python = ">=3.9", and CI runs on Python 3.9–3.11. Trailing commas after**kwargsare supported in these versions, so you can leave the signature as-is.virttest/migration.py (1)
642-655: Call-site comma is harmless and matches Black/PEP 8 multi-line styleNothing to change here. Good catch keeping the signature and call site in sync.
avocado-plugins-vt.spec (2)
46-46: LGTM! Correct addition of wheel dependency.Adding
python3-wheelto BuildRequires is necessary for modern Python packaging with pip-based installation.
91-91: LGTM! Proper modernization to pip-based installation.The change from
setup.py installtopip installwith--prefixand--no-build-isolationflags is correct for RPM packaging. The--no-build-isolationflag is particularly important in RPM builds to use the managed build environment rather than pip creating its own.contrib/packages/debian/Makefile (1)
15-15: LGTM! Correct modernization to PEP 517 build.The change from
setup.py sdisttopython -m build --sdist --outdir=../properly modernizes the source distribution creation using the standard Python build backend interface.Makefile.include (1)
10-10: LGTM! Simplified and modernized installation.The change removes conditional Python version logic in favor of a single, modern pip-based installation command. Using
--prefix $(DESTDIR) --upgrade .is correct for staged installations with the latest version.virttest/utils_test/__init__.py (2)
1197-1197: LGTM: Modernized plugin installation to use pip.This change correctly replaces the deprecated
python setup.py installcommand withpip install ., aligning with modern Python packaging standards and the PR's objective to eliminate setup.py usage.
1227-1227: LGTM: Consistent modernization of git installation method.This change maintains consistency with the plugin installation modernization by replacing
python setup.py installwithpip install .in the git installation workflow. The use ofself.pip_binensures proper pip binary selection.
| $(PYTHON) setup.py develop $(PYTHON_DEVELOP_ARGS) | ||
| $(PYTHON) -m pip install --user -e . | ||
|
|
||
| link: develop |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Declare link (and other meta targets) as .PHONY
make will look for a filesystem entry called link and may skip the recipe if such a file ever appears (e.g. after touch link).
Add the usual .PHONY stanza to guarantee the target always runs.
+# Targets that do not create an output file
+.PHONY: all check clean develop link unlink pypi🧰 Tools
🪛 checkmake (0.2.2)
[warning] 60-60: Target "link" should be declared PHONY.
(phonydeclared)
🤖 Prompt for AI Agents
In the Makefile at line 60, the target 'link' is not declared as .PHONY, which
can cause make to skip running it if a file named 'link' exists. Add 'link' to a
.PHONY declaration along with any other meta targets to ensure these targets
always execute regardless of file presence.
|
Hello guys, do you have any new concerns/comments? I’m planning to move forward with this patch to be merged, modernize this project, and to see if it will introduce other regressions. As from my personal view, it only affect the installation/bootstrap stage. |
|
Hi @PaulYuuu I think the comment in #3924 (comment) is a blocker :/ |
Indeed, for avocado, @richtja, do you think we need to support pyproject.toml in avocado first, or avocado-vt can move forward to this modern change first. Also for aexpect, it is an independent project, move it to pyproject.toml should not a complex task. If @ldoktor agree, I can help to do that. |
IMO, if avocado-vt will move forward with these changes, we will lose possibility of running avocado-vt in different avocado spawners. We will be able to use only process spawner. Which is a possible solution, but I am not sure what we will get in return by this change. IIRC @pevogam is running avocado-vt tests with LXC spawner, and he would lose this ability by this change. @pevogam am I right? |
I am definitely running VT tests in containers and Avocado VT also has a container creation script which makes parallel VT tests possible for everyone. I am not sure how the internal setup works in order to run these tests in container spawners right now, I thought the problem you were referring to was more general as in not being able to use Avocado VT as plugin for Avocado? If the problem is indeed restricted to particularities of deployment then perhaps something can be done so that VT moves to pyproject and the code is still runnable inside a better isolated environment? |
so far I did the bare minimum (like the pip vs. setup) in aexpect but since all projects are moving I'll try to reserve some time to update it to comply. I'll ping you for reviews, though... |
2417588 to
be43eeb
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
Makefile (1)
58-58: Declare PHONY targets (repeat from earlier review)link (and other meta targets) should be declared PHONY to avoid accidental no-ops when files with the same names exist.
Apply:
+# Targets that do not create an output file +.PHONY: all check clean develop link unlink pypi source source-release srpm rpm srpm-release rpm-release install
🧹 Nitpick comments (4)
docs/source/conf.py (2)
20-20: Make Sphinx conf resilient when setuptools_scm is unavailableImporting setuptools_scm at import time can break doc builds in environments without build-time deps (e.g., RTD PR previews, distro builders). Guard the import and defer to a fallback.
Apply:
-from setuptools_scm import get_version +try: + from setuptools_scm import get_version as _scm_get_version +except Exception: + _scm_get_version = None
64-66: Implement a layered version fallback indocs/source/conf.pyOur quick test confirmed that without
setuptools_scm, the current configuration errors out:ModuleNotFoundError: No module named 'setuptools_scm'To prevent build failures in environments lacking VCS metadata or
setuptools_scm, please replace the existing single-call logic with a multi-tiered fallback. Specifically:
- Environment variables (
RELEASE_VERSIONorVERSION)- Installed package metadata via
importlib.metadata/importlib_metadata- SCM lookup if
setuptools_scmis present- Default "0+unknown" as a last resort
Suggested patch in
docs/source/conf.py(around the currentVERSION = get_version(...)block):-import os -from setuptools_scm import get_version - -VERSION = get_version(root="../..", relative_to=__file__) +import os +try: + # if setuptools_scm is installed, alias for later use + from setuptools_scm import get_version as _scm_get_version +except ImportError: + _scm_get_version = None + +def _infer_version(): + # 1) Environment override + v = os.environ.get("RELEASE_VERSION") or os.environ.get("VERSION") + if v: + return v + + # 2) Installed distribution metadata + try: + from importlib.metadata import version as _dist_version + except ImportError: + try: + from importlib_metadata import version as _dist_version # for Python <3.8 + except ImportError: + _dist_version = None + + if _dist_version: + try: + return _dist_version("avocado-framework-plugin-vt") + except Exception: + pass + + # 3) SCM fallback + if _scm_get_version: + try: + return _scm_get_version(root="../..", relative_to=__file__) + except Exception: + pass + + # 4) Default + return "0+unknown" + +VERSION = _infer_version() version = VERSION release = VERSIONKey benefits:
- Resilience when building from source tarballs or in CI/CD without a Git repo
- Flexibility for packagers to inject a version via environment
- Graceful fallback to a safe default instead of build errors
Please apply this change before merging.
Makefile.include (1)
10-10: Use pip’s --root for staged installs instead of DESTDIR as the prefixUsing
--prefix $(DESTDIR)causes files to be placed directly under the staging directory (e.g._staging/bin,_staging/lib, …), which doesn’t mirror the final layout under/usr. For proper relocatable/packageable installs, invoke pip with a fixed prefix (e.g./usr) and stage everything underDESTDIRvia--root.Location:
- File: Makefile.include
- Target:
install(around line 10)Apply this optional refactor:
--- a/Makefile.include +++ b/Makefile.include @@ -9,7 +9,7 @@ install: - $(PYTHON) -m pip install --prefix $(DESTDIR) --upgrade . + $(PYTHON) -m pip install --root $(DESTDIR) --prefix /usr --upgrade .Quick manual verification:
DESTDIR="$(pwd)/_staging" make -f Makefile.include install # You should now see files under _staging/usr/…, not directly under _staging/ find _staging -type d | sed -e 's|^_staging/||' | headPlease confirm that this change produces the expected directory structure in your staging workflow.
Makefile (1)
2-2: Resolve Python interpreter more predictablyCurrent shell lookup may pick python from unexpected environments. Consider a simpler default with override: PYTHON ?= python3.
Apply:
-PYTHON=$(shell which python3 2>/dev/null || which python 2>/dev/null) +PYTHON ?= python3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (16)
.github/workflows/ci.yml(0 hunks)MANIFEST.in(1 hunks)Makefile(3 hunks)Makefile.include(2 hunks)avocado-plugins-vt.spec(2 hunks)contrib/packages/debian/Makefile(1 hunks)docs/source/conf.py(2 hunks)pyproject.toml(1 hunks)requirements.txt(0 hunks)setup.py(0 hunks)virttest/gluster.py(1 hunks)virttest/libvirt_xml/accessors.py(1 hunks)virttest/migration.py(3 hunks)virttest/qemu_monitor.py(2 hunks)virttest/utils_test/__init__.py(2 hunks)virttest/virt_admin.py(1 hunks)
💤 Files with no reviewable changes (3)
- setup.py
- requirements.txt
- .github/workflows/ci.yml
✅ Files skipped from review due to trivial changes (1)
- virttest/virt_admin.py
🚧 Files skipped from review as they are similar to previous changes (8)
- pyproject.toml
- virttest/libvirt_xml/accessors.py
- virttest/gluster.py
- virttest/utils_test/init.py
- virttest/migration.py
- virttest/qemu_monitor.py
- avocado-plugins-vt.spec
- contrib/packages/debian/Makefile
🧰 Additional context used
🪛 checkmake (0.2.2)
Makefile
[warning] 18-18: Target body for "all" exceeds allowed length of 5 (21).
(maxbodylength)
[warning] 60-60: Target body for "unlink" exceeds allowed length of 5 (7).
(maxbodylength)
[warning] 58-58: Target "link" should be declared PHONY.
(phonydeclared)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: fedora_40 AVOCADO_SRC:avocado-framework<104.0 SETUP:-m pip install PYPI_UPLOAD/*.whl VT_TYPE:qemu
- GitHub Check: fedora_40 AVOCADO_SRC: SETUP:-m pip install PYPI_UPLOAD/*.whl VT_TYPE:qemu
- GitHub Check: fedora_40 AVOCADO_SRC:avocado-framework<104.0 SETUP:-m pip install . VT_TYPE:qemu
- GitHub Check: fedora_40 AVOCADO_SRC: SETUP:-m pip install . VT_TYPE:qemu
🔇 Additional comments (9)
MANIFEST.in (2)
1-3: Confirm pruning docs/tests is intentional for both sdist and wheelPruning docs/ and tests/ keeps artifacts slim, but removes them from source distributions too. Some downstreams expect tests in the sdist.
Would you like to:
- keep them pruned (current behavior), or
- ship them in sdists only:
-prune tests -prune docs +# Keep in sdists, exclude from wheels only (setuptools_scm respects MANIFEST.in +# for sdists; for wheels, prefer pyproject package-data config) +# If you do want to prune only for wheels, handle via tool.setuptools in pyprojectIf you prefer shipping tests in sdist, I can adjust pyproject to exclude them from wheels while keeping them in sdists.
5-5: All non-Python assets are correctly packagedI’ve verified the contents of both the sdist and wheel. All required non-Python files under
• virttest/backends/*/cfg
• virttest/shared/{autoit,blkdebug,cfg,control,deps,downloads,keymaps,scripts,steps,unattended}
• virttest/test-providers.d
and the avocado_vt/conf.d directory are present in the built distributions. No runtime assets are missing, so the current MANIFEST.in exclusions are safe.Makefile.include (2)
7-7: Nice: tag-named source-release tarballUsing RELEASE_VERSION for both prefix and filename makes provenance clear and reproducible.
26-26: Good: rpm-release now consumes SRPM with RELEASE_VERSIONAligning rpm-release with the tag-based SRPM path mirrors source-release semantics and avoids mismatches.
Makefile (5)
22-24: Help text tweaks look goodUpdated messages match the pip/pyproject flow.
55-57: Switch to pip editable installs: LGTMThis aligns with PEP 517 workflows and removes setup.py dependence.
61-61: Uninstall via pip: LGTMClean uninstall for the declared package name improves the developer workflow.
71-73: Build via PEP 517: LGTMUsing python -m build is the recommended path for wheels/sdists.
4-5: Add robust fallbacks for VERSION and RELEASE_VERSION with explicit SHORT_COMMITTo avoid empty or failing values when setuptools_scm isn’t installed or there are no Git tags, update your Makefile as follows:
• Define a short-commit fallback early (if you don’t already have one):
# fallback for untagged repos SHORT_COMMIT := $(shell git rev-parse --short HEAD 2>/dev/null || echo "unknown")• VERSION: try setuptools_scm, then
git describe --dirty --always, else default to “0+unknown”-VERSION := $(shell $(PYTHON) -m setuptools_scm 2>/dev/null) +VERSION := $(shell { $(PYTHON) -m setuptools_scm 2>/dev/null \ + || git describe --tags --dirty --always 2>/dev/null; } \ + || echo "0+unknown")• RELEASE_VERSION: use the most recent tag, else “0+untagged-”
-RELEASE_VERSION := $(shell git describe --tags --abbrev=0) +RELEASE_VERSION := $(shell git describe --tags --abbrev=0 2>/dev/null \ + || echo "0+untagged-$(SHORT_COMMIT)")Next steps (please verify manually):
- Ensure
SHORT_COMMITis defined before use.- Confirm
$(PYTHON)points to a valid interpreter (e.g.python3).- Run
make VERSIONandmake RELEASE_VERSIONin both tagged and untagged repos to verify sensible outputs.
Modernize a setup.py based project by adding pyproject.toml, move package configuration to pyproject.toml. Upgrade avocado-framework must be greater than version 89.0, which is the first version that supports Python3.9. Refs: https://packaging.python.org/en/latest/guides/modernize-setup-py-project Signed-off-by: Yihuang Yu <yihyu@redhat.com>
Config file of black, isort and pylint can be pyproject.toml, so add some basic settings in pyproject.toml, so users can use the simple command to format. Signed-off-by: Yihuang Yu <yihyu@redhat.com>
Migrate deprecated "setup.py install/develop" with "pip install", also some "setup.py build" to "python -m build". For the custom clean target, convert it to shell in the Makefile. Refs: https://packaging.python.org/en/latest/discussions/setup-py-deprecated Signed-off-by: Yihuang Yu <yihyu@redhat.com>
egg had been replaced with wheel format, and should not be used anymore. Refs: https://packaging.python.org/en/latest/discussions/package-formats/#what-about-eggs Signed-off-by: Yihuang Yu <yihyu@redhat.com>
The changes standardize trailing commas in function parameters across multiple files to maintain consistent code formatting as per black's style guide. This improves readability and makes future diffs cleaner when adding new parameters. Signed-off-by: Yihuang Yu <yihyu@redhat.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@avocado-plugins-vt.spec`:
- Line 90: The pip install line uses --prefix=%{buildroot} which installs files
under the buildroot path instead of into the buildroot at the system prefix;
change the invocation to use --root=%{buildroot} and set --prefix to the target
system prefix (e.g. --prefix=%{_prefix} or --prefix=/usr) so pip installs into
%{buildroot}%{_prefix} instead of directly into %{buildroot}.
In `@docs/source/conf.py`:
- Around line 20-21: When calling setuptools_scm.get_version() in conf.py (and
where you assign the Sphinx variables version/release), wrap the call in a
try/except that catches LookupError and supplies a sensible fallback (e.g.,
"0+unknown" or read from an environment variable) so docs builds from sdists or
outside a git repo don't fail; update both places where get_version() is used
(the import/use around get_version and the assignments to version/release) and
log or warn about using the fallback so it’s obvious in build output.
In `@Makefile.include`:
- Around line 9-10: Replace the incorrect pip staging flag in the install
target: change the pip invocation in the install recipe (the line invoking
$(PYTHON) -m pip install --prefix $(DESTDIR) --upgrade .) to use --root
$(DESTDIR) instead of --prefix so pip performs a proper DESTDIR-style staging
install; also scan related packaging/spec code for any other uses of --prefix
with DESTDIR/BUILDROOT and update them to --root similarly to keep semantics
consistent.
♻️ Duplicate comments (1)
Makefile (1)
56-56: Declarelinkas.PHONY.The
linktarget should be declared.PHONYto ensure it always runs, even if a file namedlinkexists.
🧹 Nitpick comments (3)
avocado-plugins-vt.spec (1)
85-86: Consider removing the explicitsetup.py buildstep.With the move to
pip install, the build is handled internally by pip. The explicitsetup.py buildin the%buildsection is now redundant and inconsistent with the modern packaging approach. You could replace it with a no-op or use%py3_buildmacro if available.Makefile (2)
4-5: VERSION may silently fail if setuptools_scm is unavailable.If
setuptools_scmis not installed,VERSIONwill be empty (stderr is suppressed). This could cause silent failures in targets that depend onVERSION. Consider adding a fallback or validation.💡 Optional fallback pattern
-VERSION=$(shell $(PYTHON) -m setuptools_scm 2>/dev/null) +VERSION=$(shell $(PYTHON) -m setuptools_scm 2>/dev/null || echo "unknown")
67-70: Consider installingbuildas a documented prerequisite rather than inline.Installing
buildduring thepypitarget works but may unexpectedly modify the user's environment. A minor improvement would be to document it as a prerequisite or add it to development dependencies.💡 Alternative: check for build package first
pypi: clean if test ! -d PYPI_UPLOAD; then mkdir PYPI_UPLOAD; fi - $(PYTHON) -m pip install build + @$(PYTHON) -c "import build" 2>/dev/null || (echo "Installing 'build' package..." && $(PYTHON) -m pip install build) $(PYTHON) -m build -o PYPI_UPLOADOtherwise, the migration to
python -m buildis the correct modern approach for building distributions frompyproject.toml.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
.github/workflows/ci.ymlMANIFEST.inMakefileMakefile.includeavocado-plugins-vt.speccontrib/packages/debian/Makefiledocs/source/conf.pypyproject.tomlvirttest/gluster.pyvirttest/libvirt_xml/accessors.pyvirttest/qemu_monitor.py
💤 Files with no reviewable changes (1)
- .github/workflows/ci.yml
🚧 Files skipped from review as they are similar to previous changes (4)
- contrib/packages/debian/Makefile
- virttest/qemu_monitor.py
- virttest/libvirt_xml/accessors.py
- virttest/gluster.py
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-25T15:08:02.212Z
Learnt from: pevogam
Repo: avocado-framework/avocado-vt PR: 4101
File: .github/workflows/ci.yml:36-39
Timestamp: 2025-08-25T15:08:02.212Z
Learning: For GitHub Actions workflows in avocado-vt, pevogam prefers using YAML anchors (&py_versions and *py_versions) over fromJSON(env.PY_VERSIONS) for centralizing Python version matrices. Do not repeatedly suggest the fromJSON approach when YAML anchors are being used.
Applied to files:
pyproject.toml
🪛 checkmake (0.2.2)
Makefile
[warning] 18-18: Target body for "all" exceeds allowed length of 5 (21).
(maxbodylength)
[warning] 56-56: Target "link" should be declared PHONY.
(phonydeclared)
[warning] 58-58: Target body for "unlink" exceeds allowed length of 5 (7).
(maxbodylength)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: fedora_40 AVOCADO_SRC: SETUP:-m pip install PYPI_UPLOAD/*.whl VT_TYPE:qemu
🔇 Additional comments (12)
avocado-plugins-vt.spec (1)
46-46: LGTM!Adding
python3-wheelto BuildRequires is appropriate for pip-based installation workflows.MANIFEST.in (1)
1-5: LGTM!The simplified MANIFEST.in correctly excludes non-distribution files and aligns with the SCM-based versioning approach where a static VERSION file is no longer needed.
Makefile.include (1)
5-7: LGTM!The use of
RELEASE_VERSIONfor source tarball and RPM paths aligns correctly with the new SCM-based versioning approach.Also applies to: 24-26
pyproject.toml (5)
75-101: LGTM!The entry point registrations are well-structured and correctly use the pyproject.toml syntax for declaring Avocado plugin hooks.
47-56: LGTM!Package discovery and data inclusion are correctly configured with appropriate glob patterns for recursive directory inclusion.
12-12: Python version and avocado dependency alignment looks correct.The
requires-python = ">=3.9"aligns with theavocado-framework>=89.0dependency, which is noted in past discussions as the first version supporting Python 3.9.Also applies to: 35-35
58-70: LGTM!Tool configurations for Black, isort, and Pylint are properly set up with compatible settings.
1-3: The suggested version 61.0 is too low and would break the build.While
setuptools>=77.0.0(released March 19, 2025) is indeed stricter than necessary, the proposed fallback tosetuptools>=61.0is not viable. The project useslicense-files = ["LICENSE"]on line 11, which requiressetuptools>=66.1.0or later for proper support.The setuptools version requirement can be lowered, but to approximately
setuptools>=66.1.0(to ensurelicense-filessupport) rather than61.0.Likely an incorrect or invalid review comment.
Makefile (4)
14-14: Good addition of PKG_NAME for consistent uninstall.The
PKG_NAMEvariable correctly matches the package name defined inpyproject.tomland ensurespip uninstalltargets the correct package.
22-23: Help text accurately reflects the new pip-based workflow.
53-54: Modern editable install approach looks good.Using
pip install -e .is the correct modern approach that works withpyproject.toml.
58-59: Unlink correctly uses PKG_NAME for pip uninstall.The
-yflag avoids interactive prompts, which is appropriate for a Makefile target.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| %install | ||
| %{__mkdir} -p %{buildroot}%{_sysconfdir}/avocado/conf.d | ||
| %{__python3} setup.py install --root %{buildroot} --skip-build | ||
| %{__python3} -m pip install --prefix=%{buildroot} --no-build-isolation . |
There was a problem hiding this comment.
Incorrect pip install prefix handling for RPM packaging.
The --prefix=%{buildroot} flag sets the installation prefix to the buildroot path itself, rather than installing into the buildroot at the system prefix. This will result in incorrect installation paths.
For RPM packaging with pip, use --root to specify the buildroot and --prefix for the actual system prefix:
Proposed fix
-%{__python3} -m pip install --prefix=%{buildroot} --no-build-isolation .
+%{__python3} -m pip install --root=%{buildroot} --prefix=%{_prefix} --no-build-isolation .📝 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.
| %{__python3} -m pip install --prefix=%{buildroot} --no-build-isolation . | |
| %{__python3} -m pip install --root=%{buildroot} --prefix=%{_prefix} --no-build-isolation . |
🤖 Prompt for AI Agents
In `@avocado-plugins-vt.spec` at line 90, The pip install line uses
--prefix=%{buildroot} which installs files under the buildroot path instead of
into the buildroot at the system prefix; change the invocation to use
--root=%{buildroot} and set --prefix to the target system prefix (e.g.
--prefix=%{_prefix} or --prefix=/usr) so pip installs into
%{buildroot}%{_prefix} instead of directly into %{buildroot}.
| from setuptools_scm import get_version | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
setuptools_scm get_version behavior outside git repository sdist LookupError
💡 Result:
Cause
- get_version() raises LookupError when setuptools_scm cannot find SCM metadata (no .git, no supported archive metadata, and no fallback configured). This happens when building from a git checkout missing .git or from generic tar/zip archives (GitHub zip, unchecked sdist) that lack the metadata setuptools_scm expects. [1][2]
How to fix (pick one)
- Build from a real git repo (or use pip with a git URL: git+https://...#egg=...). This gives setuptools_scm the git data it needs. [3]
- Write the version into a file at build time (use_scm_version 'write_to' / tool.setuptools_scm.version_file) so installed/sdist packages have a concrete version. [1]
- Configure a fallback in pyproject.toml / setup: parentdir_prefix_version (for GitHub-style release tarballs) or fallback_version to supply a version when metadata is absent. [2]
- Ensure setuptools-scm is present in build-system.requires (or use the [simple] extra) so activation and file-finder/sdist metadata work reliably. Also for git archives, provide .git_archival.txt if you rely on archive expansion. [1][2]
References
- setuptools_scm usage (activation, write_to, fallback_root examples). [1]
- setuptools_scm configuration (fallback_version, parentdir_prefix_version). [2]
- Example reports / Q&A showing LookupError when .git/metadata missing and recommending git+ URL or write_to. [3]
🌐 Web query:
Read the Docs build process git clone or source distribution sdist
💡 Result:
Short answer: Read the Docs checks out your repository with a shallow git clone by default (it does not build from a PyPI sdist automatically). You can unshallow or change that behavior with build job commands in .readthedocs.yaml (e.g. git fetch --unshallow) or fully override the checkout step via build.jobs. [1][2]
Sources:
- Read the Docs — Build process customization (shallow clone / unshallow example). [1]
- Read the Docs — Tutorial / import & initial build behavior (projects built from your VCS repo). [2]
🏁 Script executed:
# Search for existing error handling around setuptools_scm or version retrieval in the codebase
rg -i "setuptools_scm|get_version|LookupError|exception|fallback" --type py -B 2 -A 2Repository: avocado-framework/avocado-vt
Length of output: 50385
🏁 Script executed:
# Check if there's any existing version handling or fallback mechanism in the project
fd -type f -name "*.py" | xargs grep -l "VERSION\|version" | head -10Repository: avocado-framework/avocado-vt
Length of output: 242
🏁 Script executed:
# Find and examine docs/source/conf.py
fd -type f -name "conf.py" | head -5Repository: avocado-framework/avocado-vt
Length of output: 242
🏁 Script executed:
# Look at the specific lines in conf.py around the get_version() call
cat -n docs/source/conf.py | head -80Repository: avocado-framework/avocado-vt
Length of output: 3033
🏁 Script executed:
# Check if there's any project configuration (pyproject.toml, setup.cfg) with setuptools_scm settings
fd -type f \( -name "pyproject.toml" -o -name "setup.cfg" -o -name "setup.py" \) | xargs -I {} sh -c 'echo "=== {} ===" && head -50 {}'Repository: avocado-framework/avocado-vt
Length of output: 242
🏁 Script executed:
# Search for configuration files properly
find . -maxdepth 2 -name "pyproject.toml" -o -name "setup.cfg" -o -name "setup.py" 2>/dev/null | head -5Repository: avocado-framework/avocado-vt
Length of output: 99
🏁 Script executed:
# Check if there's a .readthedocs.yaml configuration
cat .readthedocs.yaml 2>/dev/null || echo "No .readthedocs.yaml found"Repository: avocado-framework/avocado-vt
Length of output: 98
🏁 Script executed:
# Check if setuptools_scm is configured in pyproject.toml or setup.cfg
grep -r "setuptools_scm\|fallback" --include="*.toml" --include="*.cfg" --include="*.py" 2>/dev/null | grep -i version | head -20Repository: avocado-framework/avocado-vt
Length of output: 130
🏁 Script executed:
# Examine setup.py for setuptools_scm configuration
cat setup.py | head -60Repository: avocado-framework/avocado-vt
Length of output: 2371
🏁 Script executed:
# Examine pyproject.toml for setuptools_scm configuration
cat pyproject.tomlRepository: avocado-framework/avocado-vt
Length of output: 3073
🏁 Script executed:
# Search for any try-except blocks or error handling around VERSION assignment in conf.py
sed -n '58,75p' docs/source/conf.pyRepository: avocado-framework/avocado-vt
Length of output: 520
Add fallback handling for documentation builds outside git repositories.
When building documentation from a source distribution or outside a git repository, get_version() raises LookupError because setuptools_scm cannot find SCM metadata. While Read the Docs clones from git by default, this fails for local development using extracted tar/zip files, custom RTD configurations, or installations from sdist.
Proposed fallback handling
from setuptools_scm import get_version
+try:
+ VERSION = get_version(root="../..", relative_to=__file__)
+except LookupError:
+ # Fallback for builds outside git (e.g., from sdist or extracted archives)
+ VERSION = "0.0.0+unknown"
-VERSION = get_version(root="../..", relative_to=__file__)
version = VERSION
release = VERSIONAlso applies to: 65-66
🤖 Prompt for AI Agents
In `@docs/source/conf.py` around lines 20 - 21, When calling
setuptools_scm.get_version() in conf.py (and where you assign the Sphinx
variables version/release), wrap the call in a try/except that catches
LookupError and supplies a sensible fallback (e.g., "0+unknown" or read from an
environment variable) so docs builds from sdists or outside a git repo don't
fail; update both places where get_version() is used (the import/use around
get_version and the assignments to version/release) and log or warn about using
the fallback so it’s obvious in build output.
| install: | ||
| if [ "$$($(PYTHON) --version 2>&1 | cut -d' ' -f2 | cut -d'.' -f1)" == "2" ]; then \ | ||
| $(PYTHON) setup.py install --root $(DESTDIR) $(COMPILE); \ | ||
| else \ | ||
| $(PYTHON) -m pip install --prefix $(DESTDIR) --upgrade .; \ | ||
| fi | ||
| $(PYTHON) -m pip install --prefix $(DESTDIR) --upgrade . |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n "DESTDIR" Makefile Makefile.includeRepository: avocado-framework/avocado-vt
Length of output: 170
🏁 Script executed:
# Check for install instructions or README
find . -maxdepth 2 -type f \( -name "README*" -o -name "INSTALL*" -o -name "setup.py" -o -name "setup.cfg" \) | head -10
# Check for any references to DESTDIR override in the codebase
rg -n "make install.*DESTDIR" --type sh --type py --type txt
# Check the context around the install target
sed -n '8,15p' Makefile.includeRepository: avocado-framework/avocado-vt
Length of output: 410
🏁 Script executed:
# Check README for installation instructions
head -100 README.rst
# Also check setup.py to see if there's an install command
head -50 setup.pyRepository: avocado-framework/avocado-vt
Length of output: 4584
🏁 Script executed:
# Find RPM spec file
find . -name "*.spec" -type f
# Check how make install is called in CI or build scripts
rg -n "make install" --type sh --type yaml --type yml
# Check if there's any PREFIX definition
rg -n "PREFIX" Makefile Makefile.includeRepository: avocado-framework/avocado-vt
Length of output: 142
🏁 Script executed:
# Check the RPM spec file for how install is called
cat -n avocado-plugins-vt.spec | grep -A 5 -B 5 "make install"
# Check for YAML files with different approach
find . -name "*.yaml" -o -name "*.yml" | head -5
# Check how the make install command is invoked in spec file
sed -n '60,100p' avocado-plugins-vt.specRepository: avocado-framework/avocado-vt
Length of output: 1442
🏁 Script executed:
# Let's see the full context of the %install section in the spec file
sed -n '56,75p' avocado-plugins-vt.spec
# Also check if there are any other references to the Makefile install target
rg -n "Makefile.*install\|install.*Makefile" --type sh --type yamlRepository: avocado-framework/avocado-vt
Length of output: 712
🏁 Script executed:
# Check the full %install section to understand the context better
sed -n '70,90p' avocado-plugins-vt.spec
# Also verify pip install documentation behavior by checking if there are any comments or notes
rg -n "pip install" avocado-plugins-vt.spec Makefile Makefile.includeRepository: avocado-framework/avocado-vt
Length of output: 1139
🏁 Script executed:
# Let me check pip's actual behavior by looking at documentation or checking what the intended pattern is
# Also check if there are any comments or issues in the repository about this
# Search for any discussion about DESTDIR or installation in issues/docs
find . -name "*.md" -o -name "*.txt" -o -name "*.rst" | xargs grep -l "DESTDIR\|install" 2>/dev/null | head -5
# Check the full Makefile to see if there are any other install-related comments
cat MakefileRepository: avocado-framework/avocado-vt
Length of output: 3104
🏁 Script executed:
# Check pip documentation/behavior regarding --prefix vs --root
python3 << 'EOF'
import subprocess
import sys
# Show pip install help to understand --prefix vs --root behavior
result = subprocess.run([sys.executable, "-m", "pip", "install", "--help"],
capture_output=True, text=True)
# Extract relevant sections
lines = result.stdout.split('\n')
in_prefix = False
in_root = False
prefix_section = []
root_section = []
for i, line in enumerate(lines):
if '--prefix' in line and 'PREFIX' in line:
in_prefix = True
prefix_section = [line]
elif in_prefix:
if line.startswith(' --') or line.strip() == '':
prefix_section.append(line)
if line.startswith(' --'):
in_prefix = False
else:
prefix_section.append(line)
if '--root' in line and 'ROOT' in line:
in_root = True
root_section = [line]
elif in_root:
if line.startswith(' --') or line.strip() == '':
root_section.append(line)
if line.startswith(' --'):
in_root = False
else:
root_section.append(line)
print("=== --prefix ===")
for line in prefix_section[:10]:
print(line)
print("\n=== --root ===")
for line in root_section[:10]:
print(line)
EOFRepository: avocado-framework/avocado-vt
Length of output: 104
🏁 Script executed:
# Try a different approach to extract pip help
python3 -m pip install --help | grep -A 3 "prefix\|root"Repository: avocado-framework/avocado-vt
Length of output: 1070
Use --root instead of --prefix for staging installations.
The pip install --prefix $(DESTDIR) command is semantically incorrect for staging. The --prefix flag sets the installation prefix (for final paths), while --root should be used for staging installations. The spec file has the same pattern (--prefix=%{buildroot}), confirming this is a systematic issue.
For proper DESTDIR/staging semantics in both Makefile.include and the spec file:
Correct usage for staging
install:
- $(PYTHON) -m pip install --prefix $(DESTDIR) --upgrade .
+ $(PYTHON) -m pip install --root $(DESTDIR) --prefix /usr --upgrade .And in avocado-plugins-vt.spec:
-%{__python3} -m pip install --prefix=%{buildroot} --no-build-isolation .
+%{__python3} -m pip install --root=%{buildroot} --prefix /usr --no-build-isolation .📝 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.
| install: | |
| if [ "$$($(PYTHON) --version 2>&1 | cut -d' ' -f2 | cut -d'.' -f1)" == "2" ]; then \ | |
| $(PYTHON) setup.py install --root $(DESTDIR) $(COMPILE); \ | |
| else \ | |
| $(PYTHON) -m pip install --prefix $(DESTDIR) --upgrade .; \ | |
| fi | |
| $(PYTHON) -m pip install --prefix $(DESTDIR) --upgrade . | |
| install: | |
| $(PYTHON) -m pip install --root $(DESTDIR) --prefix /usr --upgrade . |
🤖 Prompt for AI Agents
In `@Makefile.include` around lines 9 - 10, Replace the incorrect pip staging flag
in the install target: change the pip invocation in the install recipe (the line
invoking $(PYTHON) -m pip install --prefix $(DESTDIR) --upgrade .) to use --root
$(DESTDIR) instead of --prefix so pip performs a proper DESTDIR-style staging
install; also scan related packaging/spec code for any other uses of --prefix
with DESTDIR/BUILDROOT and update them to --root similarly to keep semantics
consistent.
|
No one seems to be interested in this, I am closing this PR first. |
|
Hi @PaulYuuu, as this is more or less the inevitable future of python packaging I think it makes sense to keep it open. |
|
Hi @PaulYuuu, I agree with @pevogam. IMO this is very important, I am currently working on this in Avocado side and when it will be done I believe we can add this on top of that. |
|
Thank you @pevogam @richtja for the check back, I agree that this is important, but I don't think this implement in avocado-vt is depends on avocado. IMO, aexpect already switch to pyproject, vt can also do this, for avocado, it has some history debt, like egg build, spawners and so on, so hard to go this. I also opened a PR avocado-framework/avocado#5962, but it cannot address all, and must seprecate to individual task for better track. I closed this is because it live in my dashboard almost 2 years, without other acitvities, I have to rebase to reslove conflicts even if at that time it's prefect to go next, but I don't have much time to track this in the future. Well, I will reopen and draft this at this moment. |
|
The difference I see here is that aexpect is not an avocado plugin but an avocado dependency and thus has greater autonomy when it comes to deployment. Avocado VT in comparison (just like our plugin Avocado I2N) is a plugin of the Avocado framework and such has to be packaged and installed on top (e.g. discovered and integrated into the avocado core packages). As the Avocado framework itself is moving towards pyprojects I think it is only a matter until we make this easy to merge. Alternatively if you believe you should and can freely merge beforehand but not break the way VT is installed feel free to propose a way to achieve this if you like (unless you claim you already did so?). |
5 participants
Modernize a setup.py based project by adding pyproject.toml, move
package configuration to pyproject.toml.
Refs: https://packaging.python.org/en/latest/guides/modernize-setup-py-project
Depends on: #4162
Signed-off-by: Yihuang Yu yihyu@redhat.com
Summary by CodeRabbit
Chores
Style
✏️ Tip: You can customize this high-level summary in your review settings.