setup.py -> pyproject.toml #159
Conversation
|
@PaulYuuu since you are the expert in this matter, could you please review my approach? Note I haven't tested the deb/rpm targets, only updated them to be +- compliant. If anyone uses them, please verify and update them if needed. |
MANIFEST.in
Outdated
| prune tests | ||
| prune docs |
There was a problem hiding this comment.
This file is to control the sdist contents. For a source tarball, including tests and docs, also Makefile would be helpful for developers IIUC.
There was a problem hiding this comment.
As part of this I moved away from tarring sources manually, do you think I should tweak the MANIFEST to include those? The were included before but I thought they are not really useful and unnecessary increase the package size. I'm open to suggestions as I don't know how other packages handle this.
There was a problem hiding this comment.
Actually my bad, I tried using build but there were some issues so I kept the git archive. So IIUC this change is fine by you?
There was a problem hiding this comment.
Indeed, this is an open topic, it based on how we define the source archive. I checked the https://github.com/psf/black/archive/refs/tags/25.1.0.tar.gz, it includes all of files, but I don't think something like .github is useful in source.
Maybe we can keep the current MANIFEST, when someone requires more files in the source archive, we can modify the MANIFEST.in to include them.
There was a problem hiding this comment.
setuptools_scm has the best practices of this file: https://github.com/pypa/setuptools-scm/blob/main/MANIFEST.in
| @@ -0,0 +1,54 @@ | |||
| [build-system] | |||
| requires = ["setuptools>=61.0", "setuptools-scm>=8"] | |||
There was a problem hiding this comment.
With setuptools-scm, all files will be included in the sdist build, nice that we have a MANIFEST.in to control it.
Hello @ldoktor, I have a question: avocado/avocado-vt use a VERSION file to control version, aexpect use the fixed version string in setup.py in the past. Can you share more info regarding this change? I like the scm's behavior, but just want to confirm with you we would like to pick dynamic version vs fixed version string.
There was a problem hiding this comment.
You are right, previously it was simply hardcoded, with the move I think it makes sense to use a better alternative, which is the SCM. For releases nothing changes as we simply tag the version and scm uses the tag. The only difference is for locally deployed versions where the version will be different, but still based on the latest tag + suffix that tells how distant it is and which actual commit the user uses. This should actually improve our life in case of failures of locally deployed versions.
There was a problem hiding this comment.
Agree, to release a new tag, we add a new git tag, then build aexpect, it should a clear format like 1.9.0, for the main branch and local development, we can see how many commits are added after the latest tag, like 1.8.0.post11+gd671f35f0, that is useful for debug.
pyproject.toml
Outdated
| description = "Python library used to control interactive programs" | ||
| readme = "README.rst" | ||
| dynamic = ["version"] | ||
| license = {text = "GPL-2.0-or-later"} |
There was a problem hiding this comment.
I got a warning when trying to build aexpect.
/tmp/build-env-43arckc7/lib64/python3.12/site-packages/setuptools/config/_apply_pyprojecttoml.py:82: SetuptoolsDeprecationWarning: `project.license` as a TOML table is deprecated
!!
********************************************************************************
Please use a simple string containing a SPDX expression for `project.license`. You can also use `project.license-files`. (Both options available on setuptools>=77.0.0).
By 2026-Feb-18, you need to update your project and remove deprecated calls
or your builds will no longer be supported.
See https://packaging.python.org/en/latest/guides/writing-pyproject-toml/#license for details.
********************************************************************************
!!
corresp(dist, value, root_dir)
/tmp/build-env-43arckc7/lib64/python3.12/site-packages/setuptools/config/_apply_pyprojecttoml.py:61: SetuptoolsDeprecationWarning: License classifiers are deprecated.
!!
********************************************************************************
Please consider removing the following classifiers in favor of a SPDX license expression:
License :: OSI Approved :: GNU General Public License v2 or later (GPLv2+)
See https://packaging.python.org/en/latest/guides/writing-pyproject-toml/#license for details.
********************************************************************************
!!
Let's fix it by
| license = {text = "GPL-2.0-or-later"} | |
| license = "GPL-2.0-or-later" |
and remove "License :: OSI Approved :: GNU General Public License v2 or later (GPLv2+)",
pyproject.toml
Outdated
| aexpect_helper = "aexpect.scripts.aexpect_helper:main" | ||
|
|
||
| [tool.setuptools.packages.find] | ||
| where = ["."] |
There was a problem hiding this comment.
It should be the default one, but I am okay with this.
There was a problem hiding this comment.
I struggled with this of a while and you are right, it's probably redundant in this version so let me remove it.
pyproject.toml
Outdated
| [tool.setuptools.packages.find] | ||
| where = ["."] | ||
| include = ["aexpect*"] | ||
| exclude = ["debian*", "docs*", "tests*", ".github*"] |
There was a problem hiding this comment.
tool.setuptools.packages.find is to tell the build system what kinds of files should in the wheel, as include already defined, exclude is not need. I have tested and the result is expected.
$ grep -A 4 'find' pyproject.toml
[tool.setuptools.packages.find]
where = ["."]
include = ["aexpect*"]
[tool.setuptools_scm]
$ python3 -m build --wheel > /dev/null 2>&1
$ unzip -l dist/*.whl
Archive: dist/aexpect-1.8.0.post11+gdd032d41c.d20250825-py3-none-any.whl
Length Date Time Name
--------- ---------- ----- ----
949 08-25-2025 08:07 aexpect/__init__.py
70090 08-25-2025 08:07 aexpect/client.py
3583 08-25-2025 08:06 aexpect/exceptions.py
18866 08-25-2025 08:06 aexpect/ops_linux.py
20891 08-25-2025 08:07 aexpect/ops_windows.py
51814 08-25-2025 08:07 aexpect/remote.py
41164 08-25-2025 08:07 aexpect/remote_door.py
21044 08-25-2025 08:07 aexpect/rss_client.py
3541 08-25-2025 08:07 aexpect/shared.py
0 08-25-2025 08:07 aexpect/scripts/__init__.py
8198 08-25-2025 08:07 aexpect/scripts/aexpect_helper.py
0 08-25-2025 08:06 aexpect/utils/__init__.py
2491 08-25-2025 08:06 aexpect/utils/astring.py
1433 08-25-2025 08:06 aexpect/utils/data_factory.py
2420 08-25-2025 08:06 aexpect/utils/path.py
3233 08-25-2025 08:07 aexpect/utils/process.py
1420 08-25-2025 08:07 aexpect/utils/wait.py
18082 08-25-2025 11:13 aexpect-1.8.0.post11+gdd032d41c.d20250825.dist-info/licenses/LICENSE
6743 08-25-2025 11:13 aexpect-1.8.0.post11+gdd032d41c.d20250825.dist-info/METADATA
91 08-25-2025 11:13 aexpect-1.8.0.post11+gdd032d41c.d20250825.dist-info/WHEEL
71 08-25-2025 11:13 aexpect-1.8.0.post11+gdd032d41c.d20250825.dist-info/entry_points.txt
8 08-25-2025 11:13 aexpect-1.8.0.post11+gdd032d41c.d20250825.dist-info/top_level.txt
2006 08-25-2025 11:13 aexpect-1.8.0.post11+gdd032d41c.d20250825.dist-info/RECORD
--------- -------
278138 23 filesThere was a problem hiding this comment.
TBH it only works because of MANIFEST, without it the exclude did nothing, but I kept it just to be more explicit. I'm fine with removing it completely and rely on MANIFEST (this actually took me most of the time, I really wanted to avoid creating the MANIFEST but for some reason the tests and debian and other files were always included, no matter what I put here).
There was a problem hiding this comment.
Not sure if [tool.setuptools.packages.find] only for wheel and MANIFEST.in for sdist. as the exclude will not introduce regression, I am okay to keep it here.
| testpaths = ["tests"] | ||
|
|
||
| [tool.isort] | ||
| profile = "black" |
There was a problem hiding this comment.
The default line length of isort is 79, but after switching the profile to black, I am not sure if it will change to 88.
There was a problem hiding this comment.
I don't know that either. I simply recorded what was there. Do you want me to specify the line-length explicitly or should we just keep it as is and worst-case wait for a black failure after successful isort fix?
There was a problem hiding this comment.
Black does not have rules for sorting import IIRC. Well, I searched the isort doc, it will change the line length if using black profile. https://pycqa.github.io/isort/docs/configuration/profiles.html
Black is for code style, I don't think it will sort the import order. With the current configuration, there are no big problem, just only tiny gap I think.
pyproject.toml
Outdated
|
|
||
| [tool.black] | ||
| line-length = 79 | ||
| target-version = ["py38"] |
There was a problem hiding this comment.
Do we need to consider multiple version like in black doc target-version = ["py311", "py312", "py313"]?
There was a problem hiding this comment.
I went with the oldest to be safe-ish, but I'm open to adding 313 to be even safer, what do you think? (tbh I'm not sure what's the result then, does it run 2 checks with each version or does it do something smarter?)
There was a problem hiding this comment.
From
Python versions that should be supported by Black’s output. You can run black --help and look for the --target-version option to see the full list of supported versions. You should include all versions that your code supports. If you support Python 3.11 through 3.13, you should write:
we should include all aexpect supported python versions, black should detect all of them and adapt which output is safely for all.
Makefile
Outdated
| BUILDIR=$(CURDIR)/debian/aexpect | ||
| PROJECT=aexpect | ||
| VERSION="1.8.0" | ||
| VERSION=$(shell $(PYTHON) -c "import setuptools_scm; print(setuptools_scm.get_version())") |
There was a problem hiding this comment.
I found the simple command line:
$ python -m setuptools_scm
1.8.0.post11+gdd032d41c
|
Hello @ldoktor, I will take long vacation(almost 3 weeks) starting from 08-28, may delay for review this one. Most issues we already found and know how to fix. So if you plan to push this PR to be done, please go ahead, or invite other stakeholders, thanks. |
ldoktor
force-pushed
the
project.toml
branch
from
5979974 to
79157e0
Compare
📝 WalkthroughWalkthroughReplaces legacy packaging with PEP 517 Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Pre-merge checks✅ Passed checks (3 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
🧰 Additional context used🪛 checkmake (0.2.2)Makefile[warning] 9-9: Target body for "all" exceeds allowed length of 5 (10). (maxbodylength) [warning] 39-39: Target body for "pypi" exceeds allowed length of 5 (6). (maxbodylength) 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 |
|
Changes:
|
ldoktor
force-pushed
the
project.toml
branch
from
79157e0 to
ca25de7
Compare
|
Changes:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
MANIFEST.in (1)
5-5: Consider including tests in source distribution.The
global-excludepattern is comprehensive but might be too aggressive. Consider preserving*.txtfiles that could include important documentation like requirements files, while excluding only specific patterns.-global-exclude *.py[cod] __pycache__ *.so *.spec *.txt .gitignore MANIFEST.in Makefile +global-exclude *.py[cod] __pycache__ *.so *.spec .gitignoreNote: You may want to keep
requirements*.txtfiles and theMakefilein the source distribution as they can be helpful for developers working from source, as mentioned in past reviews.pyproject.toml (2)
46-46: Consider including all supported Python versions.Based on past review comments, Black's target-version should include all Python versions that your code supports.
If aexpect supports Python 3.8 through 3.13, consider:
-target-version = ["py38", "py312", "py313"] +target-version = ["py38", "py39", "py310", "py311", "py312", "py313"]Based on past review comments.
38-38: Consider simplifying package discovery configuration.The
excludedirective might be redundant since MANIFEST.in already controls source distribution contents andincludeproperly defines wheel contents.[tool.setuptools.packages.find] include = ["aexpect*"] -exclude = ["debian*", "docs*", "tests*", ".github*"]Based on past review comments.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
.github/workflows/ci.yml(1 hunks)MANIFEST.in(1 hunks)Makefile(1 hunks)aexpect/__init__.py(1 hunks)aexpect/client.py(1 hunks)aexpect/ops_windows.py(1 hunks)aexpect/remote.py(1 hunks)aexpect/remote_door.py(2 hunks)aexpect/rss_client.py(1 hunks)aexpect/scripts/aexpect_helper.py(5 hunks)aexpect/shared.py(1 hunks)aexpect/utils/process.py(1 hunks)aexpect/utils/wait.py(1 hunks)pyproject.toml(1 hunks)setup.py(0 hunks)tests/test_remote_door.py(1 hunks)
💤 Files with no reviewable changes (1)
- setup.py
🧰 Additional context used
🧬 Code graph analysis (4)
aexpect/scripts/aexpect_helper.py (2)
aexpect/shared.py (7)
get_filenames(120-132)get_lock_fd(22-41)get_reader_filename(135-137)makeraw(80-103)makestandard(106-117)unlock_fd(44-47)wait_for_lock(66-77)aexpect/client.py (1)
close(432-479)
aexpect/remote.py (2)
aexpect/client.py (2)
Expect(804-1147)RemoteSession(1612-1706)aexpect/exceptions.py (2)
ExpectProcessTerminatedError(45-57)ExpectTimeoutError(35-42)
aexpect/__init__.py (2)
aexpect/client.py (8)
Expect(804-1147)ShellSession(1150-1609)Spawn(74-558)Tail(561-801)kill_tail_threads(60-71)run_bg(1763-1814)run_fg(1817-1864)run_tail(1709-1760)aexpect/exceptions.py (7)
ExpectError(15-32)ExpectProcessTerminatedError(45-57)ExpectTimeoutError(35-42)ShellCmdError(103-117)ShellError(60-72)ShellProcessTerminatedError(85-100)ShellStatusError(120-129)
aexpect/client.py (2)
aexpect/exceptions.py (8)
ExpectError(15-32)ExpectProcessTerminatedError(45-57)ExpectTimeoutError(35-42)ShellCmdError(103-117)ShellError(60-72)ShellProcessTerminatedError(85-100)ShellStatusError(120-129)ShellTimeoutError(75-82)aexpect/shared.py (6)
get_filenames(120-132)get_lock_fd(22-41)get_reader_filename(135-137)is_file_locked(50-63)unlock_fd(44-47)wait_for_lock(66-77)
🪛 Ruff (0.13.1)
aexpect/scripts/aexpect_helper.py
100-100: Starting a process without a shell
(S606)
104-104: Starting a process without a shell
(S606)
🪛 checkmake (0.2.2)
Makefile
[warning] 9-9: Target body for "all" exceeds allowed length of 5 (10).
(maxbodylength)
[warning] 89-89: Missing required phony target "test"
(minphony)
🔇 Additional comments (24)
aexpect/utils/process.py (1)
14-16: Import grouping now consistentThe standard-library imports are now grouped and ordered consistently. No behavior change—just cleaner organization.
aexpect/shared.py (1)
14-16: Import ordering tidy-upThe reordered imports keep stdlib modules together without altering functionality. Looks good.
aexpect/utils/wait.py (1)
14-16: Import placement aligns with logging setupMoving
timebeneath the logging import maintains clarity without changing behavior.aexpect/rss_client.py (1)
29-36: Consolidated stdlib importsBringing the argparse/glob/os imports up front removes duplication and follows the repository’s import style. No functional impact.
aexpect/ops_windows.py (1)
36-42: Logging import added where neededImporting
loggingexplicitly resolves the implicit dependency forLOG = logging.getLogger(__name__). All good here.aexpect/remote_door.py (2)
60-67: LGTM! Import reorganization follows Python conventions.The reorganization groups standard library imports together at the top, following PEP 8 conventions for import ordering.
77-78: LGTM! Cleaner Pyro5 import structure.The consolidated import statement is more concise and maintains the same functionality.
MANIFEST.in (1)
1-5: Appropriate pruning for distribution packages.The exclusions align with modern packaging practices by removing development-specific files from the source distribution. This follows similar patterns used in other projects like setuptools_scm.
Based on past review comments.
.github/workflows/ci.yml (1)
23-23: Good addition for packaging verification.Adding
make installto CI ensures the new pyproject.toml-based packaging workflow is tested before running checks.tests/test_remote_door.py (1)
18-19: LGTM! Import ordering follows conventions.Standard library imports are properly grouped together at the top.
pyproject.toml (2)
1-43: LGTM! Well-structured modern Python packaging configuration.The migration from setup.py to pyproject.toml follows PEP 517/518 standards. The setuptools-scm integration provides automatic versioning based on git tags, which is a significant improvement over hardcoded versions.
16-16: Remove deprecated license classifier.The License classifier is deprecated when using the SPDX expression format in the
licensefield (line 10).Based on past review comments.
classifiers = [ "Development Status :: 6 - Mature", - "License :: OSI Approved :: GNU General Public License v2 or later (GPLv2+)", "Natural Language :: English", "Operating System :: POSIX", "Programming Language :: Python :: 3", ]aexpect/client.py (1)
20-53: Import consolidation improves maintainability.The consolidation of imports from
aexpect.exceptionsandaexpect.sharedinto grouped multi-line import blocks follows Python best practices and aligns with the project-wide import organization changes. The addition of new top-level imports (locale,logging,threading,time) provides explicit visibility into module dependencies.aexpect/remote.py (1)
46-54: Import consolidation follows consistent pattern.The consolidation of imports from
aexpect.clientandaexpect.exceptionsinto grouped multi-line blocks, along with the addition of explicittimeimport, aligns with the broader project-wide import reorganization without changing runtime behavior.aexpect/scripts/aexpect_helper.py (3)
19-34: Import consolidation and formatting improvements.The consolidation of imports from
aexpect.sharedinto a grouped multi-line import block improves readability and follows the project-wide import organization pattern.
37-102: Formatting improvements enhance readability.The conversion of single-line statements to multi-line formats and the standardization of string quotes improve code readability without changing functionality.
100-102: Security warning about shell execution is a false positive.The static analysis warning about starting processes without a shell is a false positive in this context. The code uses
os.execv("/bin/bash", ["/bin/bash", "-c", command])which is the appropriate way to execute shell commands securely with proper argument separation.aexpect/__init__.py (1)
17-37: Import consolidation maintains public API while improving organization.The consolidation of individual imports from
.clientand.exceptionsinto grouped multi-line import blocks improves maintainability without changing the public API. All previously exported entities remain accessible to users of the module.Makefile (6)
3-3: Modern version detection using setuptools_scm.The use of
python -m setuptools_scmis the recommended approach for version detection when using setuptools_scm, which extracts Python package versions from git metadata instead of declaring them statically. This aligns with the modernization from setup.py to pyproject.toml.
10-19: Expanded help text reflects new build capabilities.The updated help text appropriately documents the new
develop,pypi, andchecktargets while maintaining clarity about available packaging options.
22-44: Modern Python packaging workflow implementation.The new
install,develop, andpypitargets use modern Python packaging tools (python -m build,pip install --editable) instead of legacy setup.py commands, which aligns with the migration to pyproject.toml-based packaging.
46-51: Comprehensive check target combines multiple quality tools.The
checktarget appropriately combines linting (inspekt), formatting checks (black, isort), and testing (pytest) into a single comprehensive quality verification step.
54-55: Modern sdist generation aligns with new build system.The use of
python -m build --sdistreplaces the legacypython setup.py sdistapproach, consistent with the pyproject.toml-based build system.
85-87: Enhanced clean target covers modern build artifacts.The addition of
.venv*,.mypy_cache, and__pycache__cleanup ensures removal of common Python development artifacts that weren't covered previously.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (12)
aexpect/remote_door.py (1)
104-110: Clarify missing-backend warningIf both Pyro5 and Pyro4 are unavailable, the current message mentions only Pyro4. Consider mentioning both backends to aid troubleshooting. No behavior change.
- logging.warning( - "Remote object backend (Pyro4) not found, some functionality" - " of the remote door will not be available" - ) + logging.warning( + "Remote object backends (Pyro5/Pyro4) not found; remote door " + "functionality will be limited" + )aexpect/scripts/aexpect_helper.py (2)
95-103: Avoid leaking temp file and drop unnecessary shell -c/sourceSwitch to executing the temp script directly, add a trap to delete it on exit, and avoid shell -c. This reduces shell usage and cleans up the file.
- with tempfile.NamedTemporaryFile( - suffix=".sh", prefix="aexpect-", dir=base_dir - ) as tmp_file: - tmp_file.write(command.encode("utf-8")) - tmp_file.flush() - os.execv( - "/bin/bash", ["/bin/bash", "-c", f"source {tmp_file.name}"] - ) + with tempfile.NamedTemporaryFile( + suffix=".sh", prefix="aexpect-", dir=base_dir, delete=False + ) as tmp_file: + tmp_path = tmp_file.name + script = f'trap "rm -f {tmp_path}" EXIT\n{command}' + tmp_file.write(script.encode("utf-8")) + tmp_file.flush() + os.execv("/bin/bash", ["/bin/bash", tmp_path])
163-169: Fix bytes/str mismatch when handling control pipe commandsos.read returns bytes; comparisons with "raw"/"standard" (str) always fail, disabling terminal mode switching. Decode first.
- data = os.read(ctrlpipe_fd, cmd_len) - if data == "raw": + data = os.read(ctrlpipe_fd, cmd_len).decode("utf-8", "ignore") + if data == "raw": makeraw(shell_fd) - elif data == "standard": + elif data == "standard": makestandard(shell_fd, echo)aexpect/client.py (1)
252-260: Prefer shell=False for Popen (security and clarity)helper_cmd is already resolved via utils_path.find_command, so a shell isn’t required. Using shell=False avoids invoking a shell unnecessarily.
- sub = subprocess.Popen( # pylint: disable=R1732 - [helper_cmd], - shell=True, + sub = subprocess.Popen( # pylint: disable=R1732 + [helper_cmd], + shell=False, stdin=subprocess.PIPE, stdout=subprocess.PIPE, stderr=subprocess.STDOUT, pass_fds=pass_fds, ).github/workflows/ci.yml (1)
23-23: Install target exists and is safe
The Makefile’sinstalltarget builds and installs the wheel viapython -m pipwith no sudo required—no change needed. Optional: upgrade actions/checkout to v4 and setup-python to v4 for improved performance and security.pyproject.toml (2)
43-46: List all supported target versions for BlackBlack should include all versions you support. Current list skips 3.9–3.11.
-target-version = ["py38", "py312", "py313"] +target-version = ["py38", "py39", "py310", "py311", "py312", "py313"]
51-52: Keep isort and Black line lengths consistentWith
profile = "black", isort defaults to 88. Either change Black to 88 or pin isort to 79 to avoid churn. Given you set Black to 79, pin isort too.[tool.isort] profile = "black" +line_length = 79Based on past review comments.
aexpect/__init__.py (1)
17-37: Make the public API explicit with allDefining all prevents accidental export drift and helps tooling.
from .exceptions import ( ExpectError, ExpectProcessTerminatedError, ExpectTimeoutError, ShellCmdError, ShellError, ShellProcessTerminatedError, ShellStatusError, ShellTimeoutError, ) + +__all__ = [ + "remote", + "rss_client", + "Expect", + "ShellSession", + "Spawn", + "Tail", + "kill_tail_threads", + "run_bg", + "run_fg", + "run_tail", + "ExpectError", + "ExpectProcessTerminatedError", + "ExpectTimeoutError", + "ShellCmdError", + "ShellError", + "ShellProcessTerminatedError", + "ShellStatusError", + "ShellTimeoutError", +]Makefile (4)
3-3: Provide a fallback for VERSION when setuptools_scm isn’t installedCurrent expansion fails if setuptools_scm isn’t available in the environment parsing the Makefile. Add a git-based fallback and make it overridable.
-VERSION=$(shell $(PYTHON) -m setuptools_scm) +VERSION ?= $(shell $(PYTHON) -m setuptools_scm 2>/dev/null || git describe --tags --dirty --always 2>/dev/null || echo unknown)
9-19: Move help text to a dedicatedhelptarget and shortenallThis addresses the checkmake max body length warning and improves ergonomics.
-all: - @echo "make check - Run lint and tests" - @echo "make clean - Remove build artifacts" - @echo "make source - Create source package (commit snapshot)" - @echo "make source-release - Create source package (versioned tag)" - @echo "make install - Install from built wheel" - @echo "make develop - Editable install" - @echo "make pypi - Build wheel+sdist (ready for upload)" - @echo - @echo "Debian targets: build-deb-src, build-deb-bin, build-deb-all" - @echo "RPM targets: srpm, rpm, srpm-release, rpm-release" +all: help + +help: + @echo "make check - Run lint and tests" + @echo "make test - Run tests only" + @echo "make clean - Remove build artifacts" + @echo "make source - Create source package (commit snapshot)" + @echo "make source-release - Create source package (versioned tag)" + @echo "make install - Install from built wheel" + @echo "make develop - Editable install" + @echo "make pypi - Build wheel+sdist (ready for upload)" + @echo + @echo "Debian targets: build-deb-src, build-deb-bin, build-deb-all" + @echo "RPM targets: srpm, rpm, srpm-release, rpm-release"
45-51: Add atesttarget (checkmake requires it)Expose tests separately from lint to satisfy tooling and allow faster local runs.
check: clean inspekt checkall --disable-lint R0917,R0205,R0801,W4901,W0703,W0511 --disable-style E203,E501,E265,W601,E402 --exclude .venv* $(PYTHON) -m black --check -- $(shell git ls-files -- "*.py") $(PYTHON) -m isort --check-only -- $(shell git ls-files -- "*.py") $(PYTHON) -m pytest + +test: + $(PYTHON) -m pytest
89-91: Keep PHONY up to dateAdd the new
helpandtesttargets to PHONY.-.PHONY: all source source-release install develop pypi check clean \ - build-deb-src build-deb-bin build-deb-all srpm rpm srpm-release rpm-release prepare-source +.PHONY: all help test source source-release install develop pypi check clean \ + build-deb-src build-deb-bin build-deb-all srpm rpm srpm-release rpm-release prepare-source
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
.github/workflows/ci.yml(1 hunks)MANIFEST.in(1 hunks)Makefile(1 hunks)aexpect/__init__.py(1 hunks)aexpect/client.py(1 hunks)aexpect/ops_windows.py(1 hunks)aexpect/remote.py(1 hunks)aexpect/remote_door.py(2 hunks)aexpect/rss_client.py(1 hunks)aexpect/scripts/aexpect_helper.py(5 hunks)aexpect/shared.py(1 hunks)aexpect/utils/process.py(1 hunks)aexpect/utils/wait.py(1 hunks)pyproject.toml(1 hunks)setup.py(0 hunks)tests/test_remote_door.py(1 hunks)
💤 Files with no reviewable changes (1)
- setup.py
🚧 Files skipped from review as they are similar to previous changes (7)
- aexpect/remote.py
- tests/test_remote_door.py
- aexpect/rss_client.py
- aexpect/utils/process.py
- aexpect/ops_windows.py
- MANIFEST.in
- aexpect/shared.py
🧰 Additional context used
🧬 Code graph analysis (3)
aexpect/scripts/aexpect_helper.py (1)
aexpect/shared.py (7)
get_filenames(120-132)get_lock_fd(22-41)get_reader_filename(135-137)makeraw(80-103)makestandard(106-117)unlock_fd(44-47)wait_for_lock(66-77)
aexpect/client.py (2)
aexpect/exceptions.py (8)
ExpectError(15-32)ExpectProcessTerminatedError(45-57)ExpectTimeoutError(35-42)ShellCmdError(103-117)ShellError(60-72)ShellProcessTerminatedError(85-100)ShellStatusError(120-129)ShellTimeoutError(75-82)aexpect/shared.py (6)
get_filenames(120-132)get_lock_fd(22-41)get_reader_filename(135-137)is_file_locked(50-63)unlock_fd(44-47)wait_for_lock(66-77)
aexpect/__init__.py (2)
aexpect/client.py (8)
Expect(804-1147)ShellSession(1150-1609)Spawn(74-558)Tail(561-801)kill_tail_threads(60-71)run_bg(1763-1814)run_fg(1817-1864)run_tail(1709-1760)aexpect/exceptions.py (8)
ExpectError(15-32)ExpectProcessTerminatedError(45-57)ExpectTimeoutError(35-42)ShellCmdError(103-117)ShellError(60-72)ShellProcessTerminatedError(85-100)ShellStatusError(120-129)ShellTimeoutError(75-82)
🪛 Ruff (0.13.1)
aexpect/scripts/aexpect_helper.py
100-100: Starting a process without a shell
(S606)
104-104: Starting a process without a shell
(S606)
🪛 checkmake (0.2.2)
Makefile
[warning] 9-9: Target body for "all" exceeds allowed length of 5 (10).
(maxbodylength)
[warning] 89-89: Missing required phony target "test"
(minphony)
🔇 Additional comments (8)
aexpect/utils/wait.py (1)
15-15: LGTM on import reorderingNo behavioral impact. wait_for remains correct.
aexpect/scripts/aexpect_helper.py (1)
149-156: LGTM on main loop readability tweaksList-comprehension and select usage remain functionally identical.
aexpect/client.py (1)
20-53: LGTM on import consolidationImports are grouped and consistent; no behavior change.
pyproject.toml (3)
10-10: License switched to SPDX string — LGTMThis addresses setuptools deprecations and aligns with current packaging guidance.
35-38: Exclude list is redundant for wheels; fine to keepPackage discovery via include/exclude is okay. Note MANIFEST.in governs sdist; this block mainly affects wheels. No change required.
32-33: Console script entry point verified
aexpect.scripts.aexpect_helper:mainfound inscripts/aexpect_helper.pywith a top-levelmain()defined.aexpect/__init__.py (1)
17-37: Public exports consolidated — LGTMRe-exports are clear and backward compatible.
Makefile (1)
55-57: Guard the rename in prepare-source
rename -f '../*'may affect unrelated files in the parent directory. Narrow the scope.Consider replacing with a safer mv, e.g., identify the freshly built tarball explicitly:
- Build to a known path (e.g.,
--outdir $(CURDIR)), then:
mv "$(PROJECT)-$(VERSION).tar.gz" "../$(PROJECT)_$(VERSION).orig.tar.gz"
Would you like a patch adjusting the outdir and mv?
| from Pyro5 import nameserver, server | ||
| from Pyro5.compatibility import Pyro4 | ||
|
|
||
| NS_MODULE = "Pyro5.nameserver" | ||
| except ImportError: |
There was a problem hiding this comment.
Fix: Pyro5 path may raise AttributeError due to missing server.is_private_attribute
In the Pyro5 branch, server.is_private_attribute may not exist, but it’s used later (e.g., in share_local_object). This will raise AttributeError at runtime when Pyro5 is available. Define a compatible predicate in the Pyro5 path.
Apply this diff after the Pyro5 imports:
from Pyro5 import nameserver, server
from Pyro5.compatibility import Pyro4
NS_MODULE = "Pyro5.nameserver"
+ # Ensure attribute-filter API used below exists under Pyro5 as well
+ if not hasattr(server, "is_private_attribute"):
+ server.is_private_attribute = lambda name: name.startswith("_")📝 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.
| from Pyro5 import nameserver, server | |
| from Pyro5.compatibility import Pyro4 | |
| NS_MODULE = "Pyro5.nameserver" | |
| except ImportError: | |
| from Pyro5 import nameserver, server | |
| from Pyro5.compatibility import Pyro4 | |
| NS_MODULE = "Pyro5.nameserver" | |
| # Ensure attribute-filter API used below exists under Pyro5 as well | |
| if not hasattr(server, "is_private_attribute"): | |
| server.is_private_attribute = lambda name: name.startswith("_") | |
| except ImportError: |
🤖 Prompt for AI Agents
In aexpect/remote_door.py around lines 77 to 81, the Pyro5 branch may import
server that lacks server.is_private_attribute but later code expects it; add a
compatibility definition immediately after the Pyro5 imports that defines
server.is_private_attribute when missing (e.g., assign it to an appropriate
predicate from nameserver or implement an equivalent function that matches the
expected signature) so later calls like share_local_object won't raise
AttributeError.
Use pyproject.toml to configure our project. This required to move the aexpect_helper and a few updates to Makefile to avoid using setup.py. Signed-off-by: Lukáš Doktor <[email protected]>
no functional changes, just black formatter. Signed-off-by: Lukáš Doktor <[email protected]>
no changes, just reshuffling via isort. Signed-off-by: Lukáš Doktor <[email protected]>
ldoktor
force-pushed
the
project.toml
branch
from
ca25de7 to
cb91e0e
Compare
|
Changes:
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (7)
Makefile (7)
10-17: Trim verbose default target output; add a dedicated help targetcheckmake flags this as too long. Consider a separate help target and making all depend on it to keep the body short.
Based on static analysis hints
31-33: Harden install target: clean dist, avoid dependency resolution, force reinstallPrevents stale wheels and network dependency changes.
Apply this diff:
+ $(RM) -r dist 2>/dev/null || true - $(PYTHON) -m pip install --upgrade build + $(PYTHON) -m pip install --upgrade pip build wheel - $(PYTHON) -m build - $(PYTHON) -m pip install dist/*.whl + $(PYTHON) -m build + $(PYTHON) -m pip install --no-deps --force-reinstall dist/*.whl
38-44: Add 'twine check' to validate built artifactsCatches long descriptions and metadata issues before upload.
Apply this diff:
pypi: clean $(PYTHON) -m build + $(PYTHON) -m twine check dist/* @echo @echo @echo "Use 'python3 -m twine upload dist/*'" @echo "to upload this release"Ensure twine is included in your dev extras.
46-51: Add a 'test' alias for checkmake and developer ergonomicsExpose a test target alias to satisfy the minphony rule and common muscle memory.
Apply this diff to PHONY (Line 89) and append a target at EOF:
-.PHONY: all source source-release install develop pypi check clean \ +.PHONY: all source source-release install develop pypi check test clean \ build-deb-src build-deb-bin build-deb-all srpm rpm srpm-release rpm-release prepare-sourceAnd append:
test: checkBased on static analysis hints
85-85: Avoid nuking developer virtualenvs on cleanRemoving .venv* in clean is surprising. Split into a separate target.
Apply this diff:
-rm -rf .venv* .mypy_cache *.egg-info MANIFEST BUILD BUILDROOT SPECS RPMS SRPMS SOURCES dist +rm -rf .mypy_cache *.egg-info MANIFEST BUILD BUILDROOT SPECS RPMS SRPMS SOURCES distAnd append:
clean-venv: rm -rf .venv*
89-90: Add 'test' to .PHONY (and define target) to satisfy checkmakePairs with the earlier suggestion to add a test alias.
Apply this diff:
-.PHONY: all source source-release install develop pypi check clean \ +.PHONY: all source source-release install develop pypi check test clean \ build-deb-src build-deb-bin build-deb-all srpm rpm srpm-release rpm-release prepare-sourceThen add:
test: checkBased on static analysis hints
3-3: Ensure setuptools-scm is installed and tag_format matches git tags
Add a prereq or Makefile step to install setuptools-scm so$(PYTHON) -m setuptools_scmnever fails, and configure[tool.setuptools_scm].tag_format = "v{version}"if your repository tags include avprefix.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
.github/workflows/ci.yml(1 hunks)MANIFEST.in(1 hunks)Makefile(1 hunks)aexpect/__init__.py(1 hunks)aexpect/client.py(1 hunks)aexpect/ops_windows.py(1 hunks)aexpect/remote.py(1 hunks)aexpect/remote_door.py(2 hunks)aexpect/rss_client.py(1 hunks)aexpect/scripts/aexpect_helper.py(5 hunks)aexpect/shared.py(1 hunks)aexpect/utils/process.py(1 hunks)aexpect/utils/wait.py(1 hunks)pyproject.toml(1 hunks)setup.py(0 hunks)tests/test_remote_door.py(1 hunks)
💤 Files with no reviewable changes (1)
- setup.py
✅ Files skipped from review due to trivial changes (2)
- aexpect/utils/process.py
- aexpect/utils/wait.py
🚧 Files skipped from review as they are similar to previous changes (7)
- pyproject.toml
- .github/workflows/ci.yml
- aexpect/client.py
- aexpect/ops_windows.py
- aexpect/rss_client.py
- MANIFEST.in
- aexpect/shared.py
🧰 Additional context used
🧬 Code graph analysis (3)
aexpect/scripts/aexpect_helper.py (1)
aexpect/shared.py (7)
get_filenames(120-132)get_lock_fd(22-41)get_reader_filename(135-137)makeraw(80-103)makestandard(106-117)unlock_fd(44-47)wait_for_lock(66-77)
aexpect/__init__.py (2)
aexpect/client.py (8)
Expect(804-1147)ShellSession(1150-1609)Spawn(74-558)Tail(561-801)kill_tail_threads(60-71)run_bg(1763-1814)run_fg(1817-1864)run_tail(1709-1760)aexpect/exceptions.py (8)
ExpectError(15-32)ExpectProcessTerminatedError(45-57)ExpectTimeoutError(35-42)ShellCmdError(103-117)ShellError(60-72)ShellProcessTerminatedError(85-100)ShellStatusError(120-129)ShellTimeoutError(75-82)
aexpect/remote.py (2)
aexpect/client.py (2)
Expect(804-1147)RemoteSession(1612-1706)aexpect/exceptions.py (2)
ExpectProcessTerminatedError(45-57)ExpectTimeoutError(35-42)
🪛 Ruff (0.13.1)
aexpect/scripts/aexpect_helper.py
100-100: Starting a process without a shell
(S606)
104-104: Starting a process without a shell
(S606)
🪛 checkmake (0.2.2)
Makefile
[warning] 9-9: Target body for "all" exceeds allowed length of 5 (10).
(maxbodylength)
[warning] 89-89: Missing required phony target "test"
(minphony)
🔇 Additional comments (16)
tests/test_remote_door.py (1)
18-20: LGTM on import reordering.Moving html into the stdlib import block is correct and keeps style consistent; usage later in tests remains valid.
aexpect/remote_door.py (2)
60-67: LGTM on added stdlib imports and ordering.importlib/inspect/logging and threading placement look good and are used below.
77-81: Fix missing server.is_private_attribute in Pyro5 path to avoid runtime AttributeError.Later code calls server.is_private_attribute(..) when exposing functions/classes. Pyro5.server may not define this attribute, causing failures when Pyro5 is present.
Apply this diff right after setting NS_MODULE:
from Pyro5.compatibility import Pyro4 NS_MODULE = "Pyro5.nameserver" + # Ensure predicate exists in the Pyro5 path; used to filter private attrs + if not hasattr(server, "is_private_attribute"): + server.is_private_attribute = lambda name: name.startswith("_")aexpect/scripts/aexpect_helper.py (2)
25-34: LGTM on grouped imports from aexpect.shared.Consolidation improves readability without behavior change.
95-103: Temporary script execution flow looks fine.Switch to NamedTemporaryFile with explicit base_dir and execv is preserved; no functional change.
If you’ve seen flakiness on some platforms, consider delete=False to avoid edge cases with source reading while the FD is still open. Otherwise this is fine.
aexpect/__init__.py (1)
17-37: LGTM on public import consolidation.Export surface unchanged; grouping improves maintainability.
aexpect/remote.py (1)
45-55: LGTM on grouped imports and stdlib ordering.No behavioral impact; consistency with package-wide style.
Makefile (9)
21-21: No action neededComment-only cosmetic change.
23-24: Prefix vs archive name mismatch (COMMIT vs SHORT_COMMIT)Prefix uses full COMMIT; filename uses SHORT_COMMIT. Likely fine, but verify downstream (spec/packaging) doesn’t assume they match.
Would you like me to scan the spec for expected Source0 naming?
52-52: No action neededComment-only cosmetic change.
55-55: Debian sdist version format sanity checksetuptools_scm may emit local versions (e.g., 1.8.0+gHASH); confirm dch/dpkg accept the resulting ../$(PROJECT)-$(VERSION).tar.gz -> orig tarball rename.
Want a quick script to simulate the rename and lintian expectations?
72-74: Verify src.rpm naming matches VERSION formattingIf VERSION contains '+', rpm may normalize. Ensure the glob BUILD/SRPM/python-$(PROJECT)-$(VERSION)-*.src.rpm matches actual output.
76-81: Same rpm-release considerations as aboveRelease builds should be validated for tag/NAME-VERSION alignment.
87-87: LGTMCache cleanup is fine.
35-37: No changes needed — ‘dev’ extra is defined in pyproject.toml
68-69: No action needed: source tarball naming aligns with spec. Spec’s%global gittarfor commit builds (aexpect-%{shortcommit}.tar.gz) matches the Makefile’s$(PROJECT)-$(SHORT_COMMIT).tar.gz.
| mkdir -p SOURCES | ||
| git archive --prefix="$(PROJECT)-$(VERSION)/" -o "SOURCES/$(PROJECT)-$(VERSION).tar.gz" $(VERSION) |
There was a problem hiding this comment.
source-release may fail if setuptools_scm strips a 'v' tag prefix
If your git tag is v1.8.0, setuptools_scm yields 1.8.0, and git archive $(VERSION) will fail. Resolve by mapping version to the tag commit.
Apply this diff to resolve within the current line:
- git archive --prefix="$(PROJECT)-$(VERSION)/" -o "SOURCES/$(PROJECT)-$(VERSION).tar.gz" $(VERSION)
+ git archive --prefix="$(PROJECT)-$(VERSION)/" -o "SOURCES/$(PROJECT)-$(VERSION).tar.gz" $(shell git rev-list -n1 "refs/tags/$(VERSION)" 2>/dev/null || git rev-list -n1 "refs/tags/v$(VERSION)")Run a quick check:
#!/bin/bash
# Expect at least one to resolve to a commit
git rev-parse -q --verify "refs/tags/$(python3 -m setuptools_scm)" || git rev-parse -q --verify "refs/tags/v$(python3 -m setuptools_scm)"🤖 Prompt for AI Agents
In Makefile around lines 27-28, git archive uses $(VERSION) directly which
breaks when setuptools_scm strips a leading "v" from tags (e.g. tag v1.8.0 ->
VERSION 1.8.0); update the target to resolve the tag to a commit by checking
refs/tags/$(python3 -m setuptools_scm) and then refs/tags/v$(python3 -m
setuptools_scm) (falling back to $(VERSION) if neither exists) and use that
resolved ref in the git archive invocation so the archive refers to the actual
tag commit.
PaulYuuu
left a comment
PaulYuuu
left a comment
There was a problem hiding this comment.
Hello @ldoktor, with the current change, it seems LGTM. only 1 place I would suggest that the isort config seting line_length = 79, to avoid the black profile override it from 79 to 88.
https://pycqa.github.io/isort/docs/configuration/options.html#line-length
https://pycqa.github.io/isort/docs/configuration/profiles.html
rather then specifying it in Makefile. Signed-off-by: Lukáš Doktor <[email protected]>
Signed-off-by: Lukáš Doktor <[email protected]>
Signed-off-by: Lukáš Doktor <[email protected]>
reorder by purpose and simplify descriptions. Signed-off-by: Lukáš Doktor <[email protected]>
Signed-off-by: Lukáš Doktor <[email protected]>
functional tests require the scripts to be deployed. Signed-off-by: Lukáš Doktor <[email protected]>
Signed-off-by: Lukáš Doktor <[email protected]>
|
Changes:
|
|
Thanks @PaulYuuu, let's wait a few days for @Andrei-Stepanov or @clebergnu but this change should be transparent and work with the same setup like before. Anyway, folks, if you see any issues with this change, please let us know. |
2 participants
Modernize the setup.py -> pyproject.toml.
Summary by CodeRabbit
New Features
Chores
Refactor
Tests