Skip to content

Modernize to 3.0 - #150

Open
rosjat wants to merge 54 commits into
python-scsi:masterfrom
rosjat:modernize
Open

Modernize to 3.0#150
rosjat wants to merge 54 commits into
python-scsi:masterfrom
rosjat:modernize

Conversation

@rosjat

@rosjat rosjat commented Aug 2, 2026

Copy link
Copy Markdown
Collaborator

Modernize to 3.0

Python 3.11 floor, the custom Enum metaclass replaced by typed tables, the
package 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:

  • REPORT LUNS dropped the last LUN's low four bytes from any conformant
    parameter list. Confirmed against scsi_debug max_luns=4 and sg_luns.
  • EXTENDED COPY segment type codes 01h/0Ch (stream to block) raised
    AttributeError instead of encoding a descriptor, in both SPC-4 and SPC-5.
  • MODE SELECT (10) raised TypeError on every call.
  • WRITE SAME (16) with NDOB failed over iSCSI, which sized the transfer from
    a data-out buffer the command deliberately does not have.
  • REPORT PRIORITY truncated the last TransportID; REPORT TARGET PORT
    GROUPS
    never marshalled its port descriptors; INQUIRY had two VPD
    defects.
  • CDB field maps lived on the class, so building a second command changed
    how an earlier one decoded. marshall_cdb/unmarshall_cdb are classmethods
    now.
  • The exception metaclass built every host class twice, from the first
    result's MRO. That made typing.Generic and zero-arg super() unusable in
    any SCSICommand subclass.

Typing

strict = true over the package and the tests, no per-module exemptions, no
disabled error codes. Ships py.typed.

SCSICommand is generic in its result type: most commands key result by field
name, READ CD keys by LBA, and dict is invariant in its key — so no single
annotation 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_utils and tgt; pyscsi-report runs the whole battery and
writes a transcript. The image builds on master and release tags only, and a
tag not merged into master is refused. Packaging moved to PEP 621.

Verified

  • 130 tests / 655 subtests plus mypy, on all four runtimes
  • 15 tools and examples against a real device over SGIO
  • 11 against emulated disk, changer, tape and CD targets over iSCSI — the only
    coverage ssc, mmc and smc get
  • every commit imports, so the history bisects

Needs a maintainer

pypi.yml is written for trusted publishing (no password:, 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_TOKEN must not be
revoked, other projects share it. TestPyPI stays on a token deliberately: that
project has a different owner.

Suggest tagging v3.0.0rc1 first. The container image needs nothing — merging
publishes the first one.

rosjat added 30 commits July 30, 2026 19:11
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]>
rosjat added 24 commits August 1, 2026 21:30
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]>
@mergify

mergify Bot commented Aug 2, 2026

Copy link
Copy Markdown

Tick the box to add this pull request to the merge queue (same as @mergifyio queue).

  • Queue this pull request

@rosjat

rosjat commented Aug 2, 2026

Copy link
Copy Markdown
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!
@sahlberg if you can take a look at the scsi fixes :)

Thats a breaking change end would, as the pr says raise the version to 3.0

@sahlberg

sahlberg commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants