Modernize to 3.0 - #150
Open
rosjat wants to merge 54 commits into
Open
Conversation
No .gitattributes with core.autocrlf=true left the worktree CRLF while blobs were LF, so a container could commit CRLF blobs. Renormalized 134 files, zero content change. Also extends .gitignore. Signed-off-by: rosjat <[email protected]>
mypy.ini pinned python_version 3.7, which current mypy rejects. Config now lives in pyproject.toml at 3.11, with pytest settings alongside it. Signed-off-by: rosjat <[email protected]>
isort moves off the dead timothycrosley URL to PyCQA; hooks go to v5.0.0. Adds check-toml and mixed-line-ending, which enforces the LF rule .gitattributes only declares. Signed-off-by: rosjat <[email protected]>
Covers the import-time metaprogramming and opcode table invariants the Enum migration will touch, plus ReadDiscInformation which had no test at all. 47 -> 67 passed. Signed-off-by: rosjat <[email protected]>
setup.py/setup.cfg replaced by a [project] table; build-backend was never declared and only worked via setup.py's presence. Adds py.typed with its SPDX header in REUSE.toml, and pins black/isort to the pre-commit revs. Signed-off-by: rosjat <[email protected]>
black infers target-version from requires-python, which only now exists, so it adds trailing commas after **kwargs. Also drops stale planning references from test comments. Signed-off-by: rosjat <[email protected]>
Nothing ran pytest or mypy in CI; adds a 3.11-3.14 matrix plus a mypy job on the floor version. Publish now triggers on tags only instead of every push, splits build from upload, and gains a TestPyPI OIDC smoke test. Signed-off-by: rosjat <[email protected]>
setuptools_scm appends a PEP 440 local version to untagged builds and PyPI rejects those, so the dispatch run got a 400. Also moves upload/download artifact off v4, which targets the deprecated Node 20. Signed-off-by: rosjat <[email protected]>
Its last release is Feb 2024 and it pulls in actions/cache@v4, which targets the deprecated Node 20. Also pins the Python version, which was unset and silently used whatever was on PATH. Signed-off-by: rosjat <[email protected]>
TestPyPI stays on an API token: that project is owned by another account, so trusted publishing cannot be configured there. PyPI moves to OIDC, which the org does own. TestPyPI now uploads the real artifact, and the PyPI job refuses any tag not merged into master. Signed-off-by: rosjat <[email protected]>
Introduced in 0de000b (2016), which converted upper-bound checks into explicit ranges and turned `opcode < 0x80` into a repeat of the group 0 test, so group 3 has never been rejected. Group 3 (0x60-0x7F) is the extended and variable length CDB range, which has no fixed size. Both rejection paths now carry a message instead of a bare exception. Signed-off-by: rosjat <[email protected]>
sbc had REDUNDANCY_GROUP_OT for 0xBB and VOLUME_SET_IN for 0xBF, while smc spells both per T10. OpCode.name is only read by __repr__, so the effect was a wrong command name in output, never a wrong CDB. Signed-off-by: rosjat <[email protected]>
SCSI and OpCode declared (object), which has no meaning in Python 3. Signed-off-by: rosjat <[email protected]>
The image carries Python, both SCSI transports, sg3_utils and tgt, with the verification harness built in as pyscsi-tgt-setup, pyscsi-verify-tools and pyscsi-verify-iscsi, so it depends on nothing outside itself. Builds on changes to the Containerfile only; pull requests build and smoke-test but do not push. Signed-off-by: rosjat <[email protected]>
setup-buildx, login and build-push all declared node20 and were being forced onto Node 24 by the runner. The newer majors declare node24. Signed-off-by: rosjat <[email protected]>
The image held one Python fixed at build time, so the matrix meant four images. It now ships 3.11 through 3.14, each in its own virtualenv, selected at run time by PYSCSI_PYTHON. Runtimes come from uv; no distribution packages all four. The build layer drops its && chain, whose trailing `|| true` masked a failed install and shipped an image with no runtimes at all. Signed-off-by: rosjat <[email protected]>
The released wheels lag their repositories -- cython-iscsi on PyPI is 1.0 and predates Context.lun_reset() -- so the image tested against a state both projects had moved past. SGIO_REF and ISCSI_REF default to master. CI resolves them to commit SHAs, without which the layer caches on the literal "master" and never sees an upstream commit. Both repositories move rarely, so picking up a new commit is a manual workflow_dispatch rather than a schedule. Signed-off-by: rosjat <[email protected]>
What is left is what the code cannot say: the build-arg caching behaviour, the single-layer purge, the `|| true` exit-code trap, git's dubious ownership check, the swp skip condition and the label ordering. Signed-off-by: rosjat <[email protected]>
The per-commit tag kept every superseded image alive. Untagged versions are swept first, then old tagged ones, keeping latest and release numbers. Signed-off-by: rosjat <[email protected]>
A vX.Y.Z tag publishes :vX.Y.Z next to :latest. Separate workflow because path filters are not applied to tag pushes. Signed-off-by: rosjat <[email protected]>
python_version pins the analysis target to 3.11, so --python-version is what makes each matrix entry check its own version. Folded into the pytest matrix, which already has the interpreters. Signed-off-by: rosjat <[email protected]>
Covers the published image, the multi-runtime layout and the verification harness. Fixes the standalone tgt command, which exited immediately, and the stale Makefile, Travis and tag-format references. Signed-off-by: rosjat <[email protected]>
Enum(dict) built its class at runtime, so mypy saw no members and every access was attr-defined. Table[T] keeps duplicate values and reverse lookup while resolving statically. pyscsi.utils.enum is removed. Signed-off-by: rosjat <[email protected]>
_cdb_bits and _cdb were assigned to SCSICommand, so the last command constructed decided how every live command decoded its CDB. marshall_cdb and unmarshall_cdb are classmethods again and take the length from the opcode. SCSICommand.marshall_cdb on the base class now returns zeros instead of reusing the last command's state. Signed-off-by: rosjat <[email protected]>
build_cdb lost its cdb_len parameter in 0de000b (2016); the call site kept passing it, so modeselect10 raised TypeError on every call. Signed-off-by: rosjat <[email protected]>
LUN LIST LENGTH counts the list alone (SPC-3 r23 table 149), but unmarshall sliced data[8 : length + 4] and marshall wrote len(result) - 4. The two errors cancelled, so the round-trip passed while the last LUN decoded from its high four bytes. Verified against scsi_debug with four LUNs. Signed-off-by: rosjat <[email protected]>
TransportID is defined once in SPC-5 7.6.4 but was implemented inside PersistentReserveInReadFullStatus, so a second command needing one had no way to reach it. The two classmethods stay as delegating wrappers. Signed-off-by: rosjat <[email protected]>
Neither direction had ever run. Both read cls._datain_bits, which the class does not define, and passed .copy uncalled; the TransportID entry used a str as a bitmask; ADDITIONAL DESCRIPTOR LENGTH is two bytes but only one was read; and PRIORITY PARAMETER DATA LENGTH counts the bytes that follow (SPC-3 r23 table 152), so the descriptor slice must end at 4 + length -- four short, the last TransportID was truncated. TransportID now decodes to the same structure the other commands return. Signed-off-by: rosjat <[email protected]>
marshall_datain iterated _tpgd['relative_target_port_id'], the per-port scalar, instead of 'target_ports', the list unmarshall_datain builds -- so it raised KeyError on anything. Both halves shipped in 4842f92 with a test covering only the CDB, so the two never had to agree. unmarshall was already right; the test pins it against a captured scsi_debug response that sg_rtpg reads identically. Signed-off-by: rosjat <[email protected]>
marshall_designator returned the key name in a list for SCSI name string. sat_product_identification was 20 bytes, overlapping sat_product_rev_lvl. Signed-off-by: rosjat <[email protected]>
154 of 160 cells match. The six that do not are marked expectedFailure: 00h/CD-DA, the 08h Form 1/Formless swap, 18h/Formless, and 38h missing from the invalid list. Signed-off-by: rosjat <[email protected]>
The check iterated every provided key and raised on the first one the set yielded, usually a valid one. Four sites across spc4 and spc5. Signed-off-by: rosjat <[email protected]>
CheckDict mixes lists and tuples, so the element type stays Any and the readers cast. No behaviour change. Signed-off-by: rosjat <[email protected]>
Only SCSICheckCondition needed annotating; the rest is data tables. Signed-off-by: rosjat <[email protected]>
Metaclasses kept as they are: the per-host-class exceptions are what separate SCSI from iSCSI errors. Signed-off-by: rosjat <[email protected]>
cdb, datain, result and opcode are never None, so they are not Optional -- that spares a cast in each of the 37 command modules. dataout stays Optional for WRITE SAME (16) with NDOB. Signed-off-by: rosjat <[email protected]>
execute() sized the transfer with len(cmd.dataout), but NDOB leaves it None. Both C extensions already handle that; only this layer did not. Signed-off-by: rosjat <[email protected]>
Metaclass-injected exceptions declared as ClassVar, as in scsi_command. Signed-off-by: rosjat <[email protected]>
close() casts rather than guarding, so a None file still raises AttributeError. Signed-off-by: rosjat <[email protected]>
Nineteen modules whose only untyped surface was __init__. readcd also needed its datain signature: the result is keyed by LBA, not by field name. Signed-off-by: rosjat <[email protected]>
Fourteen modules plus the shared TransportID helper. ModeSelect6/10 take a dict of mode pages, not a buffer, and their unmarshall_datain returns None. Signed-off-by: rosjat <[email protected]>
marshall_segment read _segment_descriptor_bits_stream_block; the layout is _segment_descriptor_bits_stream_to_block, so type codes 01h and 0Ch raised AttributeError instead of encoding a descriptor. Signed-off-by: rosjat <[email protected]>
CheckDict, FieldNotation and DecodedValue move out of converter, which still re-exports CheckDict; CodeTable was declared in both extended_copy modules. validate_check_dict enforces the notation contract the alias cannot express, and a test runs it over all 143 tables in the package. Inquiry.unmarshall_datain and marshall_designator fall through to None when no branch matches, so both are Optional. Drops _pad4_len and _transport_id_bits, left behind by the TransportID move. Signed-off-by: rosjat <[email protected]>
ExMETA injects both families into SCSICommand and the devices, but each declared only its own, so SCSICommand.CheckCondition and SCSIDevice.OpcodeException worked at runtime while mypy rejected them as Any. Signed-off-by: rosjat <[email protected]>
SCSIDevice and ISCSIDevice are duck-typed siblings with no common base, so init_device returns a structural type rather than a Union that would exclude MockDevice. open() is left out: the two signatures differ and it is only ever called by a device on itself. Signed-off-by: rosjat <[email protected]>
SCSIDeviceCommandExceptionMeta created the class, took its MRO as the new bases and built it again. That put plain Generic into bases, so no host class could be a typing.Generic, and set __class__ twice, which makes zero-arg super() raise TypeError at class creation. Signed-off-by: rosjat <[email protected]>
result is keyed by field name for every command except READ CD, which returns one entry per sector and keys by LBA. dict is invariant in its key, so neither a single type nor a Union describes both; SCSICommand binds ResultT instead and each command declares its own. Signed-off-by: rosjat <[email protected]>
Parameter types are taken from each command class's own __init__. The persistentreservein chain builds four different subclasses, so cmd is declared as the base rather than inferred from the first branch. Signed-off-by: rosjat <[email protected]>
strict now applies everywhere: no exemption list and no disabled error codes. The raw CDB gets its own name so cdb stays the decoded dict it is reassigned to, and dataout is narrowed where the tests rely on it being present. Signed-off-by: rosjat <[email protected]>
Only the files this branch changed; the rest keep their original year. Signed-off-by: rosjat <[email protected]>
README-v2.md keeps the old content and the new one links to it. The breaking changes were each checked against a master worktree rather than taken from the plan, which cost one entry: init_cdb already rejected group 3, 6 and 7 opcodes. Signed-off-by: rosjat <[email protected]>
Runs pytest and mypy on every runtime, pre-commit, the build and the emulated iSCSI targets, writes a transcript and exits non-zero if a stage failed. --device adds a real device. The transcript is gitignored: a bare SPDX header in a .txt does not satisfy the reuse hook. Output goes through a pipe rather than a process substitution, which bash does not wait for -- the tail was being lost. Signed-off-by: rosjat <[email protected]>
Feature branches build it locally, so a branch push no longer runs a workflow holding packages: write. The pull_request trigger goes with them -- a fork PR could reach the registry login. container-release.yml now refuses a tag that is not merged into master, the same guard pypi.yml already applies. Signed-off-by: rosjat <[email protected]>
containers/ ships the Containerfile alone; the PowerShell wrapper is local. Signed-off-by: rosjat <[email protected]>
|
Tick the box to add this pull request to the merge queue (same as
|
Collaborator
Author
|
@Flameeyes would be cool if you take a look at the CI stuff, I left the verbose comments in for now but on the final merge we can remove them first! Thats a breaking change end would, as the pr says raise the version to 3.0 |
Contributor
|
LGTM |
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
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.
Modernize to 3.0
Python 3.11 floor, the custom
Enummetaclass replaced by typed tables, thepackage annotated under strict mypy, and a dozen defects fixed — most of them in
marshall/unmarshall paths that had no test coverage.
Breaking changes are listed in the rewritten README; the 2.x one is kept as
README-v2.md.Fixed
Each checked against T10 or hardware, not inferred from reading the code:
parameter list. Confirmed against
scsi_debug max_luns=4andsg_luns.AttributeErrorinstead of encoding a descriptor, in both SPC-4 and SPC-5.TypeErroron every call.a data-out buffer the command deliberately does not have.
GROUPS never marshalled its port descriptors; INQUIRY had two VPD
defects.
how an earlier one decoded.
marshall_cdb/unmarshall_cdbare classmethodsnow.
result's MRO. That made
typing.Genericand zero-argsuper()unusable inany
SCSICommandsubclass.Typing
strict = trueover the package and the tests, no per-module exemptions, nodisabled error codes. Ships
py.typed.SCSICommandis generic in its result type: most commands keyresultby fieldname, READ CD keys by LBA, and
dictis invariant in its key — so no singleannotation covers both. Subclassing without a parameter still works at runtime.
CI
pytest and mypy on 3.11–3.14. A dev container with all four runtimes, both
transports,
sg3_utilsandtgt;pyscsi-reportruns the whole battery andwrites a transcript. The image builds on
masterand release tags only, and atag not merged into
masteris refused. Packaging moved to PEP 621.Verified
coverage
ssc,mmcandsmcgetNeeds a maintainer
pypi.ymlis written for trusted publishing (nopassword:,id-token: write)but the publisher must be registered on pypi.org by a project Owner, and the
first release tag is what proves it. Registering one does not disable the
existing token, so it can stay as a fallback — and
PYPI_API_TOKENmust not berevoked, other projects share it. TestPyPI stays on a token deliberately: that
project has a different owner.
Suggest tagging
v3.0.0rc1first. The container image needs nothing — mergingpublishes the first one.