Skip to content

Add a windows build workflow. #140

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
roberthdevries wants to merge 2 commits into
base: master
Choose a base branch
from
+27 −1
Open

Add a windows build workflow. #140

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7d887d5
Add a windows build workflow.
roberthdevries Jul 15, 2026
5947244
Change definition of word32 to uint32_t.
roberthdevries Jul 20, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 26 additions & 0 deletions .github/workflows/python-app.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,3 +97,29 @@ jobs:
run: uv run --no-sync python -c "from wolfcrypt.random import Random; Random()"
- name: Run tests
run: uv run --no-sync pytest tests

build-windows:

@dgarske dgarske Aug 6, 2026

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.

🟠 [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 dgarske Aug 6, 2026

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.

🟠 [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 dgarske Aug 6, 2026

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.

🔵 [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.


runs-on: windows-latest

steps:
- uses: actions/checkout@v7
- name: Set up Python 3.10
uses: actions/setup-python@v6
with:
python-version: "3.10"
- name: Install uv
uses: astral-sh/setup-uv@08807647e7069bb48b6ef5acd8ec9567f424441b # v8.1.0
with:
# Install a specific version of uv.
version: "0.11.26"
- name: Build the wheel
run: uv build --wheel
- name: Install the project
run: uv sync --dev
- name: Perform static checks

@dgarske dgarske Aug 6, 2026

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.

🔵 [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'.

run: uv run ruff check
- name: Run tests using the locally built wheel

@dgarske dgarske Aug 6, 2026

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.

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 dgarske Aug 6, 2026

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.

🔴 [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 dgarske Aug 6, 2026

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.

🔴 [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.

run: |
uv pip install --reinstall dist/*.whl
uv run --no-sync pytest tests
2 changes: 1 addition & 1 deletion scripts/build_ffi.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -560,7 +560,7 @@ def build_ffi(local_wolfssl, features):
extern int HASHDRBG_ENABLED;

typedef unsigned char byte;
typedef unsigned int word32;
typedef uint32_t word32;

@dgarske dgarske Aug 6, 2026

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.

🟠 [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).


typedef struct { ...; } WC_RNG;
typedef struct { ...; } OS_Seed;
Expand Down
Loading