Change workflow to using uv and ruff.#133
Conversation
dgarske
left a comment
There was a problem hiding this comment.
Skoll Code Review
Scan type: reviewOverall recommendation: REQUEST_CHANGES
Findings: 7 total — 7 posted, 0 skipped
7 finding(s) posted as inline comments (see file-level comments below)
Posted findings
- [High] build-no-pqc 'Install uv' step has both uses and run (invalid workflow) —
.github/workflows/python-app.yml:79-87 - [High] 'Install wolfcrypt-py' step left with only env and loses USE_LOCAL_WOLFSSL —
.github/workflows/python-app.yml:76-78 - [High] uv sync --locked will fail: no uv.lock committed in the repo —
.github/workflows/python-app.yml:32 - [Medium] README documents invalid uv run sync --dev command —
README.rst:93 - [Medium] README uv pip install . requires an active virtualenv —
README.rst:67 - [Low] RST title underline too short for renamed 'pytest' heading —
README.rst:83-84 - [Low] --all-extras is a no-op (no optional-dependencies defined) —
.github/workflows/python-app.yml:32
Review generated by Skoll
adfa03e to
37e8f12
Compare
dgarske
left a comment
There was a problem hiding this comment.
Skoll Code Review
Scan type: reviewOverall recommendation: COMMENT
Findings: 6 total — 6 posted, 0 skipped
5 finding(s) posted as inline comments (see file-level comments below)
1 finding(s) not tied to a diff line (full detail below)
Posted findings
- [Medium] build-no-pqc mixes uv pip install with uv run, risking an extension rebuild without USE_LOCAL_WOLFSSL —
.github/workflows/python-app.yml:84-93 - [Medium] Malformed reStructuredText: code-block directive missing blank line —
README.rst:76-77 - [Medium] Enabling uv run ruff check may fail CI on previously-unlinted code —
.github/workflows/python-app.yml:33-34 - [Low] README tells users to uv sync to install against local wolfSSL, changing install semantics —
README.rst:67 - [Low] tox tested a built wheel (package = "wheel"); uv run pytest tests the editable/source install —
pyproject.toml:43-51 (removed [tool.tox])
Findings not tied to a diff line
Incomplete tox removal: Makefile test-all/check-all still invoke tox
File: Makefile:61-64
Function: test-all / check-all
Severity: Low
This PR removes tox from pyproject.toml and requirements/test.txt, but make test-all and make check-all still call tox. Because tox is no longer a declared test dependency, uv sync / pip install -r requirements/test.txt will not install it and those Makefile targets now break. This is a direct consequence of the diff (the Makefile is outside the changed-file set but the tox removal makes it dead/broken).
Recommendation: Update or remove the tox-based Makefile targets to keep the tox->uv migration consistent.
Referenced code: Makefile:61-64 (4 lines)
Review generated by Skoll
37e8f12 to
1db63dd
Compare
dgarske
left a comment
There was a problem hiding this comment.
Skoll Code Review
Scan type: reviewOverall recommendation: REQUEST_CHANGES
Findings: 5 total — 5 posted, 0 skipped
5 finding(s) posted as inline comments (see file-level comments below)
Posted findings
- [High] Makefile targets depend on removed install target —
Makefile:68,74,79 - [Medium] test-all loop hides per-version test failures and omits the test path —
Makefile:60-64 - [Medium] uv run auto-sync may rebuild wolfcrypt without USE_LOCAL_WOLFSSL in the no-PQC regression checks —
.github/workflows/python-app.yml:92-101 - [Medium] "Run tests using the locally built wheel" may not actually exercise the wheel —
.github/workflows/python-app.yml:31-40 - [Low] README grammar: 'from your OS repository or uv' —
README.rst:45
Review generated by Skoll
uv is used to manage virtual environments and building wheels and source distributions. This replaces tox. ruff is used to do static checking. README is updated to reflect this change.
* Update README.rst to fix remaining unclarities in using uv to install locally built version and various other smaller issues. * Pipeline changes: * Build wheel * Test against locally built wheel * Make sure no-pqc version doesn't have ML-DSA/ML-KEM enabled.
3a92dcc to
0d2e15f
Compare
dgarske
left a comment
There was a problem hiding this comment.
Skoll Code Review
Scan type: reviewOverall recommendation: COMMENT
Findings: 3 total — 3 posted, 0 skipped
3 finding(s) posted as inline comments (see file-level comments below)
Posted findings
- [Medium] no-pqc 'Import smoke' step omits --no-sync and USE_LOCAL_WOLFSSL, undermining the regression test —
.github/workflows/python-app.yml:96-97 - [Low] no-pqc 'Run tests' step omits --no-sync, causing a redundant project re-sync/rebuild —
.github/workflows/python-app.yml:98-101 - [Low] Makefile 'dist' target still uses setup.py while README migrates to 'uv build' —
Makefile:86-93
Review generated by Skoll
|
The last patch also removes the now obsolete |
All information in requirements is now present in the pyproject.toml file.
uv is used to manage virtual environments and building wheels and source distributions.
This replaces tox.
ruff is used to do static checking.
README is updated to reflect this change.