Add a windows build workflow.

[roberthdevries]

No description provided.

[dgarske]

"Windows fatal exception: access violation"?

[roberthdevries]

The crash appears to happen during this test: tests/test_asn.py::test_pem_der_conversion.
Are there any special flags/configuration options to set to compile on Windows?

[roberthdevries]

If the Python traceback can be trusted, it seems to crash in wc_InitRngNonce_ex. It is not clear to me what causes the crash, for that a local debugger is needed on a Windows development platform (that I do not have).

[dgarske]

CI is failing:

uv pip install --reinstall dist/*.whl
  uv run --no-sync pytest tests
  shell: C:\Program Files\PowerShell\7\pwsh.EXE -command ". '{0}'"
  env:
    pythonLocation: C:\hostedtoolcache\windows\Python\3.10.11\x64
    PKG_CONFIG_PATH: C:\hostedtoolcache\windows\Python\3.10.11\x64/lib/pkgconfig
    Python_ROOT_DIR: C:\hostedtoolcache\windows\Python\3.10.11\x64
    Python2_ROOT_DIR: C:\hostedtoolcache\windows\Python\3.10.11\x64
    Python3_ROOT_DIR: C:\hostedtoolcache\windows\Python\3.10.11\x64
    UV_TOOL_BIN_DIR: D:\a\_temp\uv-tool-bin-dir
    UV_TOOL_DIR: D:\a\_temp\uv-tool-dir
    UV_PYTHON_INSTALL_DIR: D:\a\_temp\uv-python-dir
    UV_CACHE_DIR: D:\a\_temp\setup-uv-cache
error: The wheel filename "*.whl" is invalid: Must have a version
============================= test session starts =============================
platform win32 -- Python 3.10.11, pytest-9.1.1, pluggy-1.6.0
rootdir: D:\a\wolfcrypt-py\wolfcrypt-py
configfile: pyproject.toml
plugins: cov-7.1.0
collected 196 items

tests\test_aesgcmstream.py .........                                     [  4%]
Windows fatal exception: access violation

Current thread 0x000003c0 (most recent call first):

[dgarske]

Skoll Code Review

Scan type: reviewOverall recommendation: REQUEST_CHANGES
Findings: 8 total — 8 posted, 0 skipped
7 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

• [High] PowerShell does not expand dist/*.whl, so the wheel install step fails on windows-latest — .github/workflows/python-app.yml:122-125
• [High] Multi-line pwsh run block does not fail-fast, so the broken wheel install produces a false-green job — .github/workflows/python-app.yml:122-125
• [Medium] word32 typedef change to uint32_t is a no-op on supported platforms and the stated rationale does not hold — scripts/build_ffi.py:563
• [Medium] build-windows is a verbatim copy of the build job instead of an OS matrix — .github/workflows/python-app.yml:101-125
• [Medium] Windows job gives no signal about which features were actually compiled in — .github/workflows/python-app.yml:101-125
• [Low] Duplicate ruff lint on a 2x-billed Windows runner — .github/workflows/python-app.yml:120-121
• [Low] No timeout-minutes on a job that builds wolfSSL from source with MSVC — .github/workflows/python-app.yml:101-103

Findings not tied to a diff line

call() uses a colon PATH separator, which corrupts PATH on Windows -- newly exercised by this workflow

File: scripts/build_ffi.py:106-109
Function: call
Severity: Medium

This is pre-existing code, not a diff hunk, but the PR is what puts it on the CI path for the first time, so it is in scope for whether this workflow can be relied on. call() joins the wolfSSL source path onto PATH with a hard-coded :. On Windows the PATH separator is ;, so the resulting variable begins with one malformed entry -- C:\...\lib\wolfssl:C:\hostedtoolcache\windows\Python\3.10.x\x64\Scripts -- which both fails to add the intended directory and destroys whatever was previously first in PATH (on GitHub runners that is typically the actions/setup-python directory that the job configured at lines 107-110). Because the new job omits USE_LOCAL_WOLFSSL, main() reaches generate_libwolfssl() at scripts/build_ffi.py:1420-1421 and every Windows build command flows through call(): cmake {flags} .., cmake --build . --config Release, cmake --install . --config Release (lines 264-266), plus git fetch / git checkout (lines 145-147). It will likely still work by luck, since cmake and git resolve from later PATH entries, but it is fragile and will produce a confusing tool-not-found failure the moment something the build needs lives in the first PATH entry.

Recommendation: Use os.pathsep instead of a literal :, and restore PATH in a finally block so a failed build does not leave the mutated PATH behind for subsequent calls in the same process. Cheap to fix now while the Windows path is being wired into CI.

Referenced code: scripts/build_ffi.py:106-109 (4 lines)

---

Review generated by Skoll

[dgarske]

🔴 [High] PowerShell does not expand dist/*.whl, so the wheel install step fails on windows-latest

The new build-windows job copies the Run tests using the locally built wheel step verbatim from the Linux build job, but the default shell on GitHub Actions windows-latest runners is pwsh, not bash. PowerShell does NOT perform wildcard/glob expansion on arguments passed to native (non-PowerShell) executables -- unlike bash, which expands dist/*.whl into the actual wheel filename before uv ever sees it. Under pwsh, uv receives the literal seven-character-glob string dist/*.whl. Neither uv pip install nor pip performs its own globbing, so uv attempts to resolve dist/*.whl as a path/requirement, finds no such file, and errors out. The step as written can never install the wheel it just built, which defeats the entire purpose of the job. Note this step is correct in the Linux build job at line 37-40 purely because bash is the default shell there.

Fix: Add shell: bash to this step. Git Bash is preinstalled on GitHub's Windows runners and GitHub invokes it with bash --noprofile --norc -eo pipefail {0}, so the glob expands correctly AND the script fails fast -- this single change also fixes the masked-failure defect reported separately. If you prefer to stay native to PowerShell, resolve the wheel explicitly instead: $wheel = Get-ChildItem dist/*.whl | Select-Object -First 1 then uv pip install --reinstall $wheel.FullName.

[dgarske]

🔴 [High] Multi-line pwsh run block does not fail-fast, so the broken wheel install produces a false-green job

GitHub Actions wraps a pwsh script by prepending $ErrorActionPreference = 'stop' and appending if ((Test-Path -LiteralPath variable:\LASTEXITCODE)) { exit $LASTEXITCODE }. $ErrorActionPreference governs PowerShell cmdlet errors only -- it does NOT abort on a non-zero exit code from a native command ($PSNativeCommandUseErrorActionPreference defaults to $false). The practical consequence is that only the LAST command in a multi-line pwsh run block determines step success. Combined with the glob defect above, this is worse than a plain failure: uv pip install --reinstall dist/*.whl exits non-zero, the script keeps going, and uv run --no-sync pytest tests then runs happily against the environment already populated by uv sync --dev at line 118-119. Tests pass, $LASTEXITCODE is 0, and the job reports success -- while the step named Run tests using the locally built wheel never installed or tested the wheel. A green check mark will assert wheel validity that was never verified, and any future wheel-packaging regression on Windows will ship undetected. This masking behavior applies to every multi-line pwsh step, so it is a hazard for this job generally, not just this one line.

Fix: Set shell: bash on the step (GitHub runs it with -eo pipefail, giving true fail-fast). If keeping pwsh, either split the two commands into separate run: steps so each exit code is checked, or add an explicit guard after each native call: if ($LASTEXITCODE -ne 0) { exit $LASTEXITCODE }. Whatever the fix, add a positive assertion that the wheel is actually the installed artifact (e.g. uv run --no-sync python -c "import wolfcrypt, sys; print(wolfcrypt.__file__)") so a silent fallback to the synced install cannot pass unnoticed.

[dgarske]

🟠 [Medium] word32 typedef change to uint32_t is a no-op on supported platforms and the stated rationale does not hold

The commit message justifies this with "'unsigned int' might be interpreted as a 64 bit unsigned int", but that does not describe any platform this project targets. In API mode, cffi expands a cdef typedef down to its primitive and emits that primitive directly into the generated C, which is then handed to the real wolfSSL function. The checked-in generated source confirms this -- build/temp.macosx-26.0-arm64-cpython-314/wolfcrypt._ffi.c:9782 reads static int _cffi_d_wc_RNG_GenerateBlock(WC_RNG * x0, unsigned char * x1, unsigned int x2) and :13945 reads _cffi_d_wc_ecc_export_x963(ecc_key * x0, unsigned char * x1, unsigned int * x2). So unsigned int is resolved by the host C compiler, and it is 32 bits on every platform in the CI matrix: Linux/macOS LP64 and Windows MSVC LLP64 alike (only long and pointers widen under LP64). uint32_t also resolves to unsigned int on all three, so the emitted C is byte-identical and the change fixes nothing. It is also a mild step backwards on portability: wolfSSL declares typedef unsigned int word32; in wolfssl/wolfcrypt/types.h, so the previous cdef matched the header type exactly. Because cffi does not verify primitive typedefs against the header, on any target where uint32_t is a distinct type from unsigned int (e.g. toolchains where uint32_t is unsigned long), the generated uint32_t * locals passed to word32 * parameters become an incompatible-pointer-type mismatch that only surfaces as a compiler diagnostic at build time. Additionally, this change is unrelated to the PR's stated purpose (a Windows build…

Fix: Please describe the concrete failure this fixes -- the exact compiler/linker error or wrong runtime value, and on which target. If there is no reproducer, revert the hunk and land the workflow on its own so the Windows CI addition can be evaluated independently. If a real width bug does exist, the fix likely belongs with the long-typed constants rather than word32, and either way it needs a regression test (e.g. a round-trip length assertion through a word32* out-parameter such as wc_ecc_export_x963).

[dgarske]

🟠 [Medium] build-windows is a verbatim copy of the build job instead of an OS matrix

build-windows (lines 101-125) is character-for-character identical to build (lines 16-40) except for runs-on. This creates a maintenance trap: the workflow now pins the same astral-sh/setup-uv commit SHA 08807647e7069bb48b6ef5acd8ec9567f424441b, the same uv version 0.11.26, the same actions/checkout@v7, and the same actions/setup-python@v6 in three separate places (lines 21-30, 52-63, 112-115). Every future dependency bump or step change has to be applied three times, and the two jobs will silently drift the first time someone forgets. A matrix also makes the Linux-vs-Windows delta explicit and reviewable rather than hidden in a duplicated block.

Fix: Collapse build and build-windows into a single job with strategy.matrix.os: [ubuntu-latest, windows-latest] and fail-fast: false so a Windows failure still yields Linux results. Setting shell: bash on the wheel-install step keeps one script working on both legs.

[dgarske]

🟠 [Medium] Windows job gives no signal about which features were actually compiled in

Without USE_LOCAL_WOLFSSL, this job builds wolfSSL from source using windows/non_fips/user_settings.h, which enables a materially narrower feature set than the Linux configure flags in make_flags() (scripts/build_ffi.py:184-246). That header omits HAVE_ED448, omits WOLFSSL_AES_SIV, and defines NO_ERROR_STRINGS. The test suite is correctly feature-gated for all of these (tests/test_ciphers.py:58 guards Ed448 on _lib.ED448_ENABLED, tests/test_ciphers.py:826 and five other sites skip on not _lib.AES_SIV_ENABLED, tests/test_error_string.py:27 guards on _lib.ERROR_STRINGS_ENABLED), so pytest will go green -- but green here attests to strictly less than the Linux job does, and nothing in the job output makes that visible. The sibling build-no-pqc job (lines 92-95) sets the right precedent by explicitly asserting _lib.ML_KEM_ENABLED == 0 and _lib.ML_DSA_ENABLED == 0 rather than trusting the build. Without an equivalent assertion, a Windows build that silently loses ML-KEM, ML-DSA, ECC, or RSA would still show a passing check because every corresponding test would simply skip.

Fix: Add a feature-assertion step mirroring build-no-pqc's pattern so a regression in the Windows user_settings.h build fails loudly instead of turning into a wall of skips, and pass -ra to pytest so the skip reasons are visible in the job log.

[dgarske]

🔵 [Low] Duplicate ruff lint on a 2x-billed Windows runner

uv run ruff check is already executed by the build job at lines 35-36. ruff's results are a pure function of the source tree and the [tool.ruff] config in pyproject.toml -- they do not vary by operating system. Running it again buys no additional signal, and GitHub bills Windows runner minutes at 2x the Linux rate. The one arguable exception is line-ending sensitivity via [tool.ruff.format] line-ending = "auto", but ruff check (as opposed to ruff format --check) does not flag that.

Fix: Drop the Perform static checks step from build-windows and let the Linux job own linting. If the jobs are merged into a matrix per the earlier suggestion, gate it with if: runner.os == 'Linux'.

[dgarske]

🔵 [Low] No timeout-minutes on a job that builds wolfSSL from source with MSVC

Because the job does not set USE_LOCAL_WOLFSSL, it runs the full cmake plus MSBuild wolfSSL build twice -- once during uv build --wheel (line 117) and again during uv sync --dev (line 119), since both trigger the cffi_modules hook in setup.py. That is a long, network- and toolchain-dependent path with no upper bound configured. GitHub's default job timeout is 360 minutes, so a hung MSBuild or a stalled git fetch inside checkout_version() will burn six hours of 2x-billed Windows minutes before the runner reclaims the job. The pre-existing jobs share this gap, but the Windows job is by far the slowest and the most likely to hit it.

Fix: Add timeout-minutes: 60 (tune once you have a few real run times to calibrate against) so a wedged native build fails in an hour rather than six.