Enable standalone mode on Windows#2178
Draft
odaneau-astro wants to merge 4 commits into
Draft
Conversation
Standalone mode was disabled on Windows (PR #2034) because the implementation used Unix-only syscalls inline. This change abstracts those into platform-specific files so the same standalone.go works on all platforms. Changes: - Extract process group ops into standalone_process_{unix,windows}.go: setProcGroup, terminateProcessGroup, killProcessGroup, interruptSignals - Extract venv bin dir into venvBinDir() (Unix: "bin", Windows: "Scripts") - Use os.PathListSeparator for PATH/PYTHONPATH joining - Remove the Windows stub (standalone_windows.go) that returned errStandaloneWindows for every operation - Remove //go:build !windows from standalone.go and standalone_test.go On Windows, process groups use CREATE_NEW_PROCESS_GROUP and tree termination uses taskkill /T. Signal handling uses os.Interrupt (Ctrl+C) since SIGTERM doesn't exist on Windows. Closes #2177 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Coverage Report for CI Build 27689190148Coverage increased (+0.01%) to 45.101%Details
Uncovered Changes
Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
…iles
Tests that use shell scripts (#!/bin/sh) as fake binaries, Unix PATH
separators, or process group management are moved to
standalone_unix_test.go with a //go:build !windows constraint.
The remaining tests in standalone_test.go run on all platforms.
Platform-specific test helpers (longSleepCommand, failCommand) are
in standalone_process_{unix,windows}_test.go.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Rather than splitting tests by heuristic, keep the entire existing test suite behind //go:build !windows — these tests were written and validated on Unix and may have subtle platform assumptions beyond shell scripts. New Windows-specific tests can be added and validated on a real Windows runner as the feature matures. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
odaneau-astro
changed the title
Three issues found during review of the Windows enablement PR: 1. Bash() and standaloneExecAirflowCommand() hardcoded "bash" which doesn't exist on Windows. Extract shellCommand() and interactiveShellArgs() into the platform-specific files so Unix uses bash and Windows uses cmd.exe. 2. terminateProcessGroup used taskkill without /F, which sends WM_CLOSE — ineffective for console processes like Python/Airflow. Replace with GenerateConsoleCtrlEvent(CTRL_BREAK_EVENT), the proper Windows equivalent of SIGTERM. CREATE_NEW_PROCESS_GROUP (already set in setProcGroup) enables this. 3. NO_PROXY=* was injected unconditionally as a macOS _scproxy workaround but now ran on Windows too, where it could override registry-configured corporate proxy settings with no benefit. Guard with runtime.GOOS == darwin. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
|
This might fix the |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes #2177
Standalone mode was disabled on Windows since PR #2034 because
standalone.goused Unix-only syscalls inline (syscall.SysProcAttr{Setpgid},syscall.Kill(-pid, SIGTERM/SIGKILL),signal.Notify(SIGINT, SIGTERM)). This change abstracts those into platform-specific files so the samestandalone.goworks on all platforms.What changed
Setpgid: trueCREATE_NEW_PROCESS_GROUPkill(-pid, SIGTERM)taskkill /T /PIDkill(-pid, SIGKILL)taskkill /F /T /PIDSIGINT,SIGTERMos.InterruptbinScripts:;(os.PathListSeparator)Files
standalone_process_unix.go/standalone_process_windows.go— platform-specific helpers:setProcGroup,terminateProcessGroup,killProcessGroup,interruptSignals,venvBinDirstandalone_process_{unix,windows}_test.go— platform-specific test helpers (longSleepCommand,failCommand)standalone.go— removed//go:build !windows, replaced allsyscall.*calls with cross-platform helpers, useos.PathListSeparatorfor PATH/PYTHONPATHstandalone_test.go— kept//go:build !windows(existing tests validated on Unix only); replaced inlinesyscallusage with cross-platform helpersstandalone_windows.go— the stub that returnederrStandaloneWindowsfor every operationTest strategy
The existing test suite stays behind
//go:build !windows— these tests were written and validated on Unix and may have subtle platform assumptions beyond shell scripts (file permissions, path conventions, etc.). New Windows-specific tests should be added and validated on a real Windows CI runner as the feature matures.Test plan
GOOS=windows GOARCH=amd64 go build ./...GOOS=linux GOARCH=amd64 go build ./...🤖 Generated with Claude Code