Skip to content

Clean up setup.py - #137

Open
roberthdevries wants to merge 1 commit into
wolfSSL:masterfrom
roberthdevries:cleanup-setup-py
Open

Clean up setup.py#137
roberthdevries wants to merge 1 commit into
wolfSSL:masterfrom
roberthdevries:cleanup-setup-py

Conversation

@roberthdevries

Copy link
Copy Markdown
Contributor
  • remove obsolete merging of LICENSING.rst in description as the include is not there anymore
  • put setuptools configuration in pyproject.toml
  • the setup_requires setting in setup.py is deprecated and is covered by pyproject.toml [build-system] - requires

@dgarske
dgarske self-requested a review July 8, 2026 15:28

@dgarske dgarske left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please resolve merge conflicts. Thanks!

- remove obsolete merging of LICENSING.rst in description as the
  include is not there anymore
- put setuptools configuration in pyproject.toml
- the setup_requires setting in setup.py is deprecated and is covered
  by pyproject.toml [build-system] - requires
@roberthdevries

Copy link
Copy Markdown
Contributor Author

Rebased on master, conflicts are resolved.

Comment thread setup.py

@sebastian-carpenter sebastian-carpenter left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These changes seem to strip the portion that copies the .dll. The .dll is only copied on FIPS builds and non-FIPS builds will statically link the library.

Before approving I would like to see the Windows workflow in #140 validate the existence of the .dll in the build.

@dgarske dgarske left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Skoll Code Review

Scan type: reviewOverall recommendation: REQUEST_CHANGES
Findings: 4 total — 1 posted, 3 skipped
1 finding(s) not tied to a diff line (full detail below)

Findings not tied to a diff line

package_data removed with no pyproject.toml replacement - py.typed, .pyi stubs and wolfssl-fips.dll are dropped from…

File: setup.py:56 (removed line) / pyproject.toml:49-50
Function: setup
Severity: High

The PR deletes package_data={"wolfcrypt": ["*.dll", "**/*.pyi", "py.typed"]} from setup.py but adds only [tool.setuptools.packages.find] to pyproject.toml. There is no [tool.setuptools.package-data] (or include-package-data + MANIFEST.in) replacement anywhere in the repo - I grepped the whole tree and the only remaining packaging config is the 2-line packages.find block.

The consequences are concrete, because all three glob patterns match real files that exist in the tree:

  1. wolfcrypt/py.typed (0-byte PEP 561 marker, added by commit fda2869 "Add py.typed file to indicate that this library has typing annotations") will no longer be installed. Every downstream consumer running mypy/pyright/ty against an installed wolfcrypt silently loses all type information - the package reverts to being untyped. This directly undoes fda2869.

  2. wolfcrypt/_ffi/__init__.pyi and wolfcrypt/_ffi/lib.pyi (the hand-written CFFI stubs, 913 B and 23 KB) were matched by **/*.pyi and will no longer ship. Note the new include = [..., "wolfcrypt._ffi"] line does not save them: wolfcrypt/_ffi/ contains no .py files at all, so declaring it as a (PEP 420 namespace) package produces an empty directory, and wheels do not archive empty directories. Without package-data the include entry is a no-op.

  3. wolfcrypt/wolfssl-fips.dll - scripts/build_ffi.py:419-423 copies this DLL into wolfcrypt/ at build time specifically so it lands "alongside the .pyd created by CFFI". *.dll was the pattern that carried it into the wheel. Windows FIPS wheels will now…

Recommendation: Add a [tool.setuptools.package-data] table to pyproject.toml before merging. The two-key form above is clearest now that wolfcrypt._ffi is an explicitly declared package; the single-key equivalent wolfcrypt = ["*.dll", "**/*.pyi", "py.typed"] also works and is a byte-for-byte port of the deleted setup.py value. Verify with uv build --wheel && unzip -l dist/*.whl | grep -E 'py.typed|\.pyi' and confirm all three stub/marker files are present.

Referenced code: setup.py:56 (removed line) / pyproject.toml:49-60 (12 lines)


Skipped findings

  • [Medium] long_description block in setup.py is now dead code - pyproject.toml's [project] readme takes precedence
  • [Medium] packages.find include list uses exact names, so any future wolfcrypt subpackage is silently omitted from the wheel
  • [Medium] No test or CI check verifies that built wheels contain py.typed, the .pyi stubs, or the FIPS DLL

Review generated by Skoll

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.

4 participants