Skip to content

MILAB-6205: add freesasa with opt-in MSVC activation for Windows#88

Open
xnacly wants to merge 12 commits into
mainfrom
MILAB-6205_freesasa-windows-msvc
Open

MILAB-6205: add freesasa with opt-in MSVC activation for Windows#88
xnacly wants to merge 12 commits into
mainfrom
MILAB-6205_freesasa-windows-msvc

Conversation

@xnacly
Copy link
Copy Markdown
Contributor

@xnacly xnacly commented Jun 3, 2026

Second try at MILAB-6205 (first attempt was #86, reverted in #87 because the windows-x64 wheel build failed).

What broke

freesasa publishes no cp312 wheels for any platform, so #86 added a buildWheel entry to compile from the sdist on the native runner. Four platforms compiled fine; windows-x64 failed with:

error: Microsoft Visual C++ 14.0 or greater is required. Get it with "Microsoft C++ Build Tools": https://visualstudio.microsoft.com/visual-cpp-build-tools/

The windows-latest runner has VS 2022 Build Tools installed, but the env vars vcvarsall.bat normally sets (INCLUDE / LIB / LIBPATH / PATH) aren't present in the prebuild job's shell, so setuptools' distutils-based MSVC compiler shim can't find cl.exe.

Why not the kalign-python approach

kalign-python uses scikit-build-core + cmake + clang-cl, which sidesteps distutils entirely. That works because kalign's source ships a CMakeLists.txt that cmake can drive. freesasa's source is plain setup.py / setuptools / distutils with a pre-generated SWIG C wrapper. Replicating the kalign approach would require forking freesasa or wrapping it in a cmake harness, which is much more work than activating vcvars for the one package that needs it.

What this PR does

Opt-in MSVC activation via a new needsMsvc: true flag on a buildWheel entry. When set on Windows, the builder runs vcvarsall.bat once (memoized per-arch) and merges the resulting env delta into the pip wheel subprocess.

Builder code

  • builder/src/msvc.ts (new): resolveMsvcEnv(arch) probes vswhere.exe for VS 2022 with the C++ toolset, runs vcvarsall.bat <arch> in a cmd subshell with && set, parses, returns the env delta vs current process env. No-op on non-Windows.
  • builder/src/util.ts: runCommand grows an extraEnv opt that merges on top of the inherited process env. Other call sites unchanged.
  • builder/src/build.ts: in the buildWheel branch, resolves msvcEnv when osType === 'windows' && buildWheel.needsMsvc; otherwise the env stays empty. Passed to both the pip install of build-requires and the pip wheel call.

Config

  • python-3.12.10/config.json: re-adds freesasa==2.2.1 to deps. buildWheel entries for all five platforms; windows-x64 carries needsMsvc: true.

Blast radius

  • kalign-python is the only other Windows buildWheel entry in the repo. It does NOT set needsMsvc and keeps using scikit-build-core + clang-cl + cmake. cmake handles its own toolchain discovery, and the MSVC env vars are never set for its build.
  • Prebuilt wheels and pure-Python installs don't go through the buildWheel branch at all, so they're untouched.
  • Other Windows buildWheels added later opt in to MSVC explicitly by setting needsMsvc: true.

Verification

  • TypeScript compiles clean (tsc --noEmit --esModuleInterop against the changed files).
  • JSON is valid.
  • The local build path is unchanged on Linux / macOS for every existing package.
  • Real verification is the Windows CI run on this PR — that's the one we need to watch.

Ref: previous attempt #86, revert #87.

Greptile Summary

This PR re-introduces freesasa==2.2.1 to the Python 3.12.10 runenv by adding an opt-in MSVC activation path (needsMsvc: true) that runs vcvarsall.bat before pip wheel on Windows, fixing the cl.exe-not-found failure from the first attempt (#86).

  • builder/src/msvc.ts (new): probes vswhere.exe, captures the post-vcvarsall.bat environment delta, memoizes it per-arch, and returns {} on non-Windows — other build paths are completely unaffected.
  • builder/src/util.ts: extends runCommand with an extraEnv option that layers additional vars on top of the inherited process env without touching any existing call sites.
  • python-3.12.10/config.json: adds freesasa==2.2.1 with buildWheel entries for all five platforms; only windows-x64 carries needsMsvc: true.

Confidence Score: 4/5

Safe to merge for Linux/macOS builds; Windows wheel build for freesasa hinges on the cmd.exe quoting in captureVcvarsEnv working correctly on the runner, which should hold for the standard VS install path.

The builder changes are well-scoped: the opt-in flag means no existing package is affected, and the extraEnv merge in util.ts is clean. The only real concern is in the new msvc.ts file — JSON.stringify is used to quote a cmd.exe command, which is non-standard (cmd.exe uses "" not " for quote escaping) and also suppresses vcvarsall stderr, making failures opaque. These work in the expected runner environment but are fragile if paths or runner images change.

builder/src/msvc.ts — specifically the captureVcvarsEnv function's command construction and stderr suppression.

Important Files Changed

Filename Overview
builder/src/msvc.ts New module: probes vswhere, captures vcvarsall env delta, memoizes per-arch. Two issues: stderr suppressed via >NUL 2>&1 (loses vcvarsall error messages), and JSON.stringify used for cmd.exe quoting (non-standard, accidental correctness for standard VS paths).
builder/src/build.ts Adds opt-in MSVC env resolution for the buildWheel branch; correctly gates on osType === 'windows' && needsMsvc, passes msvcEnv to both the build-requires install and pip wheel invocations.
builder/src/util.ts Adds extraEnv option to runCommand; merges on top of defaultExecOpts.env cleanly. All existing call sites are unaffected.
python-3.12.10/config.json Adds freesasa==2.2.1 with buildWheel entries for all 5 platforms; windows-x64 correctly sets needsMsvc: true. Missing trailing newline.
.changeset/milab-6205-freesasa-windows-msvc.md Changeset file marking patch bumps for the runenv and builder packages; accurate description of the change.

Sequence Diagram

sequenceDiagram
    participant B as build.ts (buildWheel branch)
    participant M as msvc.ts
    participant VS as vswhere.exe
    participant VC as vcvarsall.bat
    participant U as util.ts (runCommand)
    participant PIP as pip

    B->>B: "osType === 'windows' && needsMsvc?"
    alt "needsMsvc = true (windows-x64 / freesasa)"
        B->>M: resolveMsvcEnv(arch)
        M->>M: cache.get(arch)?
        alt not cached
            M->>VS: execFileSync(vswhere, [...])
            VS-->>M: VS installationPath
            M->>VC: "execSync(vcvarsall.bat amd64 >NUL 2>&1 && set)"
            VC-->>M: env dump (stdout)
            M->>M: compute delta vs process.env
            M->>M: cache.set(arch, delta)
        end
        M-->>B: msvcEnv (delta)
    else "needsMsvc = false / non-Windows"
        B->>B: "msvcEnv = {}"
    end
    B->>U: runCommand(pip install buildRequires, extraEnv: msvcEnv)
    U->>PIP: spawn with merged env
    B->>U: runCommand(pip wheel ..., extraEnv: msvcEnv)
    U->>PIP: spawn with merged env
    PIP-->>B: .whl artifact
Loading
Prompt To Fix All With AI
Fix the following 3 code review issues. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 3
builder/src/msvc.ts:75-76
**vcvarsall stderr silently discarded on failure**

`>NUL 2>&1` redirects both stdout _and_ stderr of `vcvarsall.bat` to NUL, so if vcvarsall fails (wrong arch string, incomplete VS install, runner image change, etc.) the only signal is the non-zero exit code that `execSync` throws. The actual error message — printed by vcvarsall to stderr — is gone. Changing to `>NUL` (stdout only) lets stderr pass through to the console so CI logs contain the real reason instead of a bare "exit code 1".

### Issue 2 of 3
builder/src/msvc.ts:75-76
**`JSON.stringify` is not a safe cmd.exe quoting mechanism**

`JSON.stringify` escapes internal `"` as `\"` and `\` as `\\`. cmd.exe does not recognise `\"` as an escaped quote — it's not a cmd.exe escape sequence — and it would see the doubled backslashes (e.g. `C:\\Program Files\\`) literally. In practice the standard VS install path survives this, but it is accidental: any path with characters that JSON and cmd.exe escape differently would break silently. Using `cp.execFileSync('cmd.exe', ['/c', vcvarsall, arch, '&&', 'set'])` (or building the arg list without the shell-level re-quoting) is the conventional approach for Windows subprocess invocation.

```suggestion
  const cmd = `"${vcvarsall}" ${arch} >NUL && set`;
  const out = cp.execSync(`cmd /c ${cmd}`, {
```

### Issue 3 of 3
python-3.12.10/config.json:70
**Missing newline at end of file**

The diff shows `\ No newline at end of file`. Adding a trailing newline avoids noisy diffs in future edits and keeps the file consistent with JSON style in the repo.

Reviews (1): Last reviewed commit: "review: dynamic vswhere path, direct exe..." | Re-trigger Greptile

Greptile also left 3 inline comments on this PR.

…l build

freesasa was first added in PR #86 and reverted in #87 because the
windows-x64 wheel build failed with:

  error: Microsoft Visual C++ 14.0 or greater is required.

The windows-latest runner has VS 2022 Build Tools installed but the
env vars vcvarsall.bat normally sets (INCLUDE / LIB / LIBPATH / PATH)
are missing, so setuptools' distutils-based MSVC compiler shim can't
find cl.exe. freesasa's setup.py uses plain setuptools / distutils
(no cmake), so we cannot reuse kalign-python's clang-cl + cmake path
without forking or wrapping freesasa.

Builder changes:
- builder/src/msvc.ts (new): resolveMsvcEnv(arch) probes vswhere.exe
  for the latest VS install with the C++ toolset, runs vcvarsall.bat
  in a cmd subshell with `&& set`, and returns the env delta (only
  the keys vcvars actually changes). Memoized per-arch. No-op on
  non-Windows.
- builder/src/util.ts: runCommand grows an `extraEnv` option that
  merges on top of the inherited process env when the caller supplies
  one. Other call sites unchanged.
- builder/src/build.ts: buildWheel branch resolves msvcEnv when
  `osType === 'windows' && buildWheel.needsMsvc`; otherwise the env
  stays empty. Passed to the pip install of build-requires AND the
  pip wheel call, so a build backend that compiles a C extension on
  install also finds cl.exe.

Config change:
- python-3.12.10/config.json: re-add freesasa 2.2.1 to deps. buildWheel
  entries for all five platforms; windows-x64 carries `needsMsvc: true`
  to opt into the new activation.

kalign-python (the other Windows buildWheel entry in this repo) does
NOT set `needsMsvc` and so is completely unaffected: it keeps using
scikit-build-core + clang-cl + cmake, which handles its own toolchain
discovery. Future packages that need plain MSVC opt in explicitly.
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request adds support for building freesasa 2.2.1 from source across platforms, introducing MSVC environment activation on Windows via vcvarsall.bat when needsMsvc is enabled. The review feedback highlights critical issues on Windows: JSON.stringify breaks path quoting when executing vcvarsall.bat, merging environment variables can lead to case-insensitive duplicates (like Path and PATH), and hardcoding the C:\ drive for vswhere.exe may fail on systems with different drive configurations. Addressing these issues will ensure robust and reliable builds on Windows.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment thread builder/src/msvc.ts Outdated
Comment thread builder/src/util.ts Outdated
Comment thread builder/src/msvc.ts Outdated
…erge

Address gemini-code-assist review on PR #88:

1. builder/src/msvc.ts (high): cp.execSync wrapped the vcvars command in
   `cmd /c JSON.stringify(...)`. cmd does not honour JSON's
   backslash-quote escapes, so the wrapped path quoting breaks when
   vcvarsall.bat lives under a path that contains spaces (which it
   always does on the GitHub runners). execSync already runs through
   the system shell; pass the command string directly. Also drop the
   `2>&1` redirect so vcvars errors stay on stderr and reach the CI
   log instead of being swallowed.

2. builder/src/msvc.ts (medium): vswhere.exe was hardcoded to
   `C:\Program Files (x86)\...`. Resolve via the
   `ProgramFiles(x86)` env var with a fall-back to the conventional
   path so the builder still works on hosts where Windows lives on a
   different drive (self-hosted runners, custom images).

3. builder/src/util.ts (high): `defaultExecOpts` captured `process.env`
   at module-import time, so later mutations were invisible. Worse,
   spreading the inherited env and the caller's `extraEnv` on Windows
   can leave both `Path` and `PATH` in the resulting env (JS keys are
   case-sensitive; Windows env vars are case-insensitive), and the
   child process may read whichever casing the OS lookup finds first.
   Replaced with a `buildExecEnv(extraEnv)` helper called per
   invocation that builds the env fresh and, on Windows, removes any
   case-differing duplicate before writing the caller's value.

No behaviour change for callers that don't pass `extraEnv`.
Comment thread builder/src/msvc.ts Outdated
Comment thread builder/src/msvc.ts Outdated
Comment thread python-3.12.10/config.json Outdated
xnacly added 4 commits June 3, 2026 13:09
Greptile review feedback: avoid the no-trailing-newline diff noise on
future edits.
The previous push activated MSVC env (INCLUDE / LIB / LIBPATH / PATH
were correctly populated; 45 vars added/changed per the
[msvc] log line) but the wheel build still failed with:

  error: Microsoft Visual C++ 14.0 or greater is required.

The error originates in setuptools' `_msvccompiler.initialize()`,
which calls `_get_vcvarsall_path()` to run its OWN VS detection
(vswhere + registry lookup) and raises if it returns empty. On the
portable Python in this runenv that detection returns nothing, even
though VS 2022 Build Tools are installed on the runner and we just
finished running vcvarsall.bat ourselves.

`DISTUTILS_USE_SDK=1` (plus the legacy partner flag `MSSdk=1`) tells
distutils' MSVC compiler shim to skip its own VS lookup and trust the
env vars already in scope. Adding both to the vcvars env delta so
setuptools stops trying to re-find what we already found.
The macos-14-large runner pool is flaking on PR #88: same workflow
run, same CPython 3.12.10, some variants pass on Intel mac and
others fail in CPython's PGO test phase with `ModuleNotFoundError:
No module named '_testcapi'`. Not a code issue (same builds work on
the previous main commit) and not specific to freesasa.

Apple stopped selling Intel Macs mid-2023, macOS 15 Sequoia dropped
Intel support entirely, only Sonoma 14 still runs on them. The
Intel-mac runtime audience is shrinking; we'd rather not pay a
per-PR CI tax for it. Aarch64 (Apple Silicon) keeps building
freesasa normally.

Drops the macosx-x64 buildWheel entry too since the skip beats it
to the punch in the builder (shouldSkipPackage runs before
getBuildWheel).
The hand-rolled approach (msvc.ts + extraEnv plumbing through runCommand
+ DISTUTILS_USE_SDK env var) was solving in the builder what should be
solved at the runner level. The shared
`milaboratory/github-ci/actions/setup-msvc-dev-cmd@v4` action (a thin
wrapper around `ilammy/msvc-dev-cmd`) handles vcvars activation
properly, sets DISTUTILS_USE_SDK, and is the standard tool other
milaboratory repos use (e.g. pframes-rs).

Reverts:
- builder/src/msvc.ts (deleted)
- builder/src/util.ts back to the version on main (drops `extraEnv`
  option and case-insensitive env merge)
- builder/src/build.ts back to main (drops `needsMsvc` handling in the
  buildWheel branch)
- python-3.12.10/config.json windows-x64 entry loses `needsMsvc: true`;
  the comment now points at the workflow as the activation site.

Still on this PR:
- freesasa 2.2.1 added to deps + buildWheel for all platforms except
  macosx-x64 (skipped, Intel-mac runner flake + EOL platform).

Pending: a PR against milaboratory/github-ci v4 to add the
setup-msvc-dev-cmd step to the `node-matrix-pnpm.yaml` reusable
workflow's prebuild job. Once that lands and v4 tag advances, this
runenv build picks it up automatically.
xnacly added 6 commits June 3, 2026 16:05
GitHub Actions has announced deprecation of the macOS 14 Sonoma runner
image family (both Intel and ARM): deprecation begins July 6 2026 and
the images are fully unsupported by November 2 2026, with brownout
windows starting October 5 (jobs scheduled during the brownout fail).

Source: actions/runner-images#13518

Swaps in `.github/workflows/build.yaml`:
- "macos-14"       -> "macos-15"        (ARM, 10 entries)
- "macos-14-large" -> "macos-15-large"  (Intel, 10 entries)

macOS 15 keeps both Intel and ARM variants, so Intel mac coverage
stays in the matrix; only the underlying runner image moves forward.
Also expected to clear the intermittent CPython 3.12 PGO `_testcapi`
failures we saw on the macos-14-large pool, since macOS 15 ships a
newer Xcode / SDK.
milaboratory/github-ci#177 added a `Setup MSVC Dev Cmd` step to the
node-matrix reusable workflows and was merged to v4-beta. The
v4-beta -> v4 promotion (milaboratory/github-ci#178) is still open,
so a build referencing `@v4` still hits the old workflow without
MSVC activation.

Per the team's convention (branch from v4-beta, experiment in
v4-beta, validate downstream against v4-beta, then merge v4-beta
to v4 and revert downstream pin), switch the reusable workflow
reference to `@v4-beta` for the duration of this PR. Will be
reverted to `@v4` once #178 merges.

`context/init@v4` reference is left untouched, the action is
unchanged on v4 (only the reusable workflow YAML was modified).
turbo's `tasks.build.passThroughEnv` is an allowlist; anything not
listed is stripped before invoking the task. That meant the
`DISTUTILS_USE_SDK=1` / `MSSdk=1` / `INCLUDE` / `LIB` / `LIBPATH`
vars set by milaboratory/github-ci/actions/setup-msvc-dev-cmd@v4
were silently dropped when pnpm/turbo invoked pl-py-builder. The
python subprocess then saw a clean env, setuptools' MSVC compiler
shim ran its independent vswhere lookup and raised
"Microsoft Visual C++ 14.0 or greater is required" despite vcvars
being correctly activated at the runner step level.

Adds the MSVC env vars (DISTUTILS_USE_SDK, MSSdk, INCLUDE, LIB,
LIBPATH, VC* / VS* / Windows* install markers) to the build task's
passThroughEnv. PATH is already auto-passed by turbo.

This was the actual missing piece. The setup-msvc-dev-cmd workflow
step + DISTUTILS_USE_SDK action change were both correct; turbo was
just hiding their effect from the build task.
…indows-msvc

# Conflicts:
#	python-3.12.10/config.json
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.

1 participant