From a1e550f0f014d86c19a1ed79311e80437d50897c Mon Sep 17 00:00:00 2001 From: fchengyan <3181088318@qq.com> Date: Thu, 3 Sep 2026 10:18:12 +0800 Subject: [PATCH] fix(skills): retry pip installs without --break-system-packages when unsupported pip >= 23.0 added the PEP 668 --break-system-packages flag. On older pip builds (e.g. macOS Command Line Tools Python) and on the pip `list` subcommand of every version, the flag is rejected with "no such option: --break-system-packages", breaking skill dependency install/check paths entirely. - dep_installer: route pip installs through pipRunInstall, which retries once without the flag when pip rejects it (issue #956) - pip_update_executor: same retry for the upgrade path - pip_update_checker: drop the flag from `pip3 list --outdated` - list is read-only, PEP 668 never applies, and no pip accepts the flag there - add pip_flags.go helpers + unit tests, and legacy-pip integration tests reproducing the original failure Fixes #956 --- internal/skills/dep_installer.go | 10 +- internal/skills/pip_flags.go | 49 ++++++++ internal/skills/pip_flags_test.go | 166 +++++++++++++++++++++++++ internal/skills/pip_update_checker.go | 6 +- internal/skills/pip_update_executor.go | 33 +++-- 5 files changed, 248 insertions(+), 16 deletions(-) create mode 100644 internal/skills/pip_flags.go create mode 100644 internal/skills/pip_flags_test.go diff --git a/internal/skills/dep_installer.go b/internal/skills/dep_installer.go index 42d781c9..ba5e4fae 100644 --- a/internal/skills/dep_installer.go +++ b/internal/skills/dep_installer.go @@ -106,8 +106,9 @@ func InstallSingleDep(ctx context.Context, dep string) (bool, string) { } defer release() } - cmd := exec.CommandContext(ctx, "pip3", "install", "--no-cache-dir", "--break-system-packages", pkg) - out, err := cmd.CombinedOutput() + // pipRunInstall retries without the PEP 668 flag when pip rejects it + // (pip < 23.0, see issue #956). + out, err := pipRunInstall(ctx, []string{"install", "--no-cache-dir", pipBreakSystemPackagesFlag, pkg}) if err != nil { msg := fmt.Sprintf("%s: %v", strings.TrimSpace(string(out)), err) slog.Error("skills: dep install failed", "dep", dep, "error", msg) @@ -183,8 +184,9 @@ func InstallDeps(ctx context.Context, manifest *SkillManifest, missing []string) slog.Info("skills: installing pip packages", "pkgs", pipPkgs) var successful []string for _, pkg := range pipPkgs { - cmd := exec.CommandContext(ctx, "pip3", "install", "--no-cache-dir", "--break-system-packages", pkg) - if out, err := cmd.CombinedOutput(); err != nil { + // pipRunInstall retries without the PEP 668 flag when pip rejects it. + out, err := pipRunInstall(ctx, []string{"install", "--no-cache-dir", pipBreakSystemPackagesFlag, pkg}) + if err != nil { result.Errors = append(result.Errors, fmt.Sprintf("pip %s: %s (%v)", pkg, strings.TrimSpace(string(out)), err)) if hint := pipBuildFailHint(pkg, string(out)); hint != "" { slog.Warn("skills: dep install hint", "pkg", pkg, "hint", hint) diff --git a/internal/skills/pip_flags.go b/internal/skills/pip_flags.go new file mode 100644 index 00000000..7501a378 --- /dev/null +++ b/internal/skills/pip_flags.go @@ -0,0 +1,49 @@ +package skills + +import ( + "bytes" + "context" + "os/exec" +) + +// pipBreakSystemPackagesFlag is the PEP 668 opt-out flag accepted by pip >= 23.0. +// Older pip builds (and the `list` subcommand on every pip) reject it with +// "no such option: --break-system-packages", which breaks skill dependency +// installs on macOS Command Line Tools Python and other legacy environments. +const pipBreakSystemPackagesFlag = "--break-system-packages" + +// pipRejectsBSPFlag reports whether pip's combined output indicates the +// --break-system-packages flag was rejected as an unknown option. pip < 23.0 +// predates PEP 668 and emits exactly this error (see issue #956). +func pipRejectsBSPFlag(combinedOut []byte) bool { + return bytes.Contains(combinedOut, []byte("no such option: "+pipBreakSystemPackagesFlag)) +} + +// dropPipBSPFlag returns args with every --break-system-packages occurrence +// removed, preserving order of the remaining tokens. Used to retry an install +// on pip builds that do not support the flag. +func dropPipBSPFlag(args []string) []string { + out := make([]string, 0, len(args)) + for _, a := range args { + if a == pipBreakSystemPackagesFlag { + continue + } + out = append(out, a) + } + return out +} + +// pipRunInstall runs `pip3 ` where args includes the PEP 668 +// --break-system-packages flag. If pip rejects the flag as an unknown option +// (pip < 23.0, issue #956), the command is retried once without it. Returns +// the combined output of the final attempt. Success-path overhead is zero +// extra subprocesses. +func pipRunInstall(ctx context.Context, args []string) ([]byte, error) { + cmd := exec.CommandContext(ctx, pipBinary, args...) + out, err := cmd.CombinedOutput() + if err != nil && pipRejectsBSPFlag(out) { + cmd = exec.CommandContext(ctx, pipBinary, dropPipBSPFlag(args)...) + out, err = cmd.CombinedOutput() + } + return out, err +} diff --git a/internal/skills/pip_flags_test.go b/internal/skills/pip_flags_test.go new file mode 100644 index 00000000..983b4113 --- /dev/null +++ b/internal/skills/pip_flags_test.go @@ -0,0 +1,166 @@ +package skills + +import ( + "context" + "os" + "path/filepath" + "runtime" + "strings" + "testing" +) + +func TestPipRejectsBSPFlag(t *testing.T) { + cases := []struct { + name string + out string + want bool + }{ + {"exact error", "no such option: --break-system-packages", true}, + {"error with context", "Usage: pip3 install\n\nno such option: --break-system-packages", true}, + {"unrelated error", "ERROR: Could not find a version that satisfies the requirement", false}, + {"empty", "", false}, + } + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + if got := pipRejectsBSPFlag([]byte(c.out)); got != c.want { + t.Errorf("pipRejectsBSPFlag(%q) = %v, want %v", c.out, got, c.want) + } + }) + } +} + +func TestDropPipBSPFlag(t *testing.T) { + in := []string{"install", "--upgrade", "--no-cache-dir", "--break-system-packages", "--upgrade-strategy", "only-if-needed", "--pre", "pkg"} + got := dropPipBSPFlag(in) + want := []string{"install", "--upgrade", "--no-cache-dir", "--upgrade-strategy", "only-if-needed", "--pre", "pkg"} + if strings.Join(got, " ") != strings.Join(want, " ") { + t.Errorf("dropPipBSPFlag() = %v, want %v", got, want) + } +} + +// writeLegacyPipScript writes a fake `pip3` that behaves like pip < 23.0: it +// rejects --break-system-packages with the exact error from issue #956 and +// succeeds without it. Every successful non-flag invocation appends its args +// to argsFile so tests can assert the flag was not passed on the retry. +func writeLegacyPipScript(t *testing.T, argsFile string) string { + t.Helper() + dir := t.TempDir() + path := filepath.Join(dir, "pip3") + body := "#!/bin/sh\n" + + "for a in \"$@\"; do\n" + + " if [ \"$a\" = \"--break-system-packages\" ]; then\n" + + " echo \"no such option: --break-system-packages\" >&2\n" + + " exit 2\n" + + " fi\n" + + "done\n" + + "case \"$1\" in\n" + + " install|cache) echo \"$@\" >> \"" + argsFile + "\"; exit 0 ;;\n" + + "esac\n" + + "exit 0\n" + if runtime.GOOS == "windows" { + path += ".cmd" + body = "@echo off\r\n" + + "setlocal\r\n" + + "for %%A in (%*) do (\r\n" + + " if \"%%~A\"==\"--break-system-packages\" (\r\n" + + " echo no such option: --break-system-packages 1>&2\r\n" + + " exit /b 2\r\n" + + " )\r\n" + + ")\r\n" + + "if \"%~1\"==\"install\" (\r\n" + + " echo %* >> \"" + argsFile + "\"\r\n" + + " exit /b 0\r\n" + + ")\r\n" + + "if \"%~1\"==\"cache\" (\r\n" + + " echo %* >> \"" + argsFile + "\"\r\n" + + " exit /b 0\r\n" + + ")\r\n" + + "exit /b 0\r\n" + } + if err := os.WriteFile(path, []byte(body), 0o755); err != nil { + t.Fatalf("write legacy pip script: %v", err) + } + return path +} + +// setupLegacyPip points pipBinary/pipLookPath at a legacy pip script that +// rejects --break-system-packages. +func setupLegacyPip(t *testing.T, argsFile string) { + t.Helper() + scriptPath := writeLegacyPipScript(t, argsFile) + origBinary := pipBinary + origLookPath := pipLookPath + pipBinary = scriptPath + pipLookPath = func(string) (string, error) { return scriptPath, nil } + t.Cleanup(func() { + pipBinary = origBinary + pipLookPath = origLookPath + }) +} + +// TestInstallSingleDepPip_LegacyPipNoFlag reproduces issue #956: on a pip that +// rejects --break-system-packages, installing a pip dep must still succeed via +// the automatic retry without the flag. +func TestInstallSingleDepPip_LegacyPipNoFlag(t *testing.T) { + argsFile := filepath.Join(t.TempDir(), "captured-args.txt") + setupLegacyPip(t, argsFile) + + ok, msg := InstallSingleDep(context.Background(), "pip:requests") + if !ok { + t.Fatalf("InstallSingleDep failed on legacy pip: %s", msg) + } + + captured, err := os.ReadFile(argsFile) + if err != nil { + t.Fatalf("args file not written: %v", err) + } + if strings.Contains(string(captured), "--break-system-packages") { + t.Fatalf("legacy pip received --break-system-packages: %q", string(captured)) + } +} + +// TestInstallDepsPip_LegacyPipNoFlag covers the batch path (InstallDeps) with +// the same legacy pip fixture. +func TestInstallDepsPip_LegacyPipNoFlag(t *testing.T) { + argsFile := filepath.Join(t.TempDir(), "captured-args.txt") + setupLegacyPip(t, argsFile) + + res, err := InstallDeps(context.Background(), &SkillManifest{}, []string{"pip:requests", "pip:numpy"}) + if err != nil { + t.Fatalf("InstallDeps returned error: %v", err) + } + if len(res.Errors) != 0 { + t.Fatalf("InstallDeps errors = %v, want none", res.Errors) + } + if len(res.Pip) != 2 { + t.Fatalf("InstallDeps installed = %v, want 2 packages", res.Pip) + } + + captured, err := os.ReadFile(argsFile) + if err != nil { + t.Fatalf("args file not written: %v", err) + } + if strings.Contains(string(captured), "--break-system-packages") { + t.Fatalf("legacy pip received --break-system-packages: %q", string(captured)) + } +} + +// TestPipUpdateExecutor_LegacyPipNoFlag verifies the upgrade path also retries +// without the flag on legacy pip. +func TestPipUpdateExecutor_LegacyPipNoFlag(t *testing.T) { + argsFile := filepath.Join(t.TempDir(), "captured-args.txt") + setupLegacyPip(t, argsFile) + + e := NewPipUpdateExecutor() + if err := e.Update(context.Background(), "requests", "2.31.0", nil); err != nil { + t.Fatalf("Update failed on legacy pip: %v", err) + } + + captured, err := os.ReadFile(argsFile) + if err != nil { + t.Fatalf("args file not written: %v", err) + } + if strings.Contains(string(captured), "--break-system-packages") { + t.Fatalf("legacy pip received --break-system-packages: %q", string(captured)) + } +} diff --git a/internal/skills/pip_update_checker.go b/internal/skills/pip_update_checker.go index 951530c1..3095cff1 100644 --- a/internal/skills/pip_update_checker.go +++ b/internal/skills/pip_update_checker.go @@ -111,7 +111,11 @@ func (c *PipUpdateChecker) runOutdated(ctx context.Context, includePre bool) ([] cctx, cancel := context.WithTimeout(ctx, 30*time.Second) defer cancel() - args := []string{"list", "--outdated", "--format", "json", "--break-system-packages"} + // `list` is a read-only query — PEP 668 (externally-managed environments) + // only constrains commands that write into site-packages, and pip rejects + // --break-system-packages on `list` on every version (verified on pip 24). + // Passing it here makes the outdated check fail unconditionally. + args := []string{"list", "--outdated", "--format", "json"} if includePre { args = append(args, "--pre") } diff --git a/internal/skills/pip_update_executor.go b/internal/skills/pip_update_executor.go index b856eca5..0ff97fdf 100644 --- a/internal/skills/pip_update_executor.go +++ b/internal/skills/pip_update_executor.go @@ -36,9 +36,8 @@ func (e *PipUpdateExecutor) Update(ctx context.Context, name, toVersion string, cctx, cancel := context.WithTimeout(ctx, 5*time.Minute) defer cancel() - args := []string{ - "install", "--upgrade", - "--no-cache-dir", "--break-system-packages", + args := []string{"install", "--upgrade", + "--no-cache-dir", pipBreakSystemPackagesFlag, "--upgrade-strategy", "only-if-needed", } @@ -57,18 +56,17 @@ func (e *PipUpdateExecutor) Update(ctx context.Context, name, toVersion string, } args = append(args, name) - cmd := exec.CommandContext(cctx, pipBinary, args...) - cmd.WaitDelay = 2 * time.Second - var stdout, stderr bytes.Buffer - cmd.Stdout = &stdout - cmd.Stderr = &stderr - start := time.Now() - runErr := cmd.Run() + stderr, runErr := runPipUpdateCommand(cctx, args) + // If pip rejects the PEP 668 flag (pip < 23.0, issue #956), retry once + // without it so updates keep working on legacy environments. + if runErr != nil && pipRejectsBSPFlag([]byte(stderr)) { + stderr, runErr = runPipUpdateCommand(cctx, dropPipBSPFlag(args)) + } durationMs := time.Since(start).Milliseconds() if runErr != nil { - sentinel, reason := ClassifyPipStderr(stderr.String()) + sentinel, reason := ClassifyPipStderr(stderr) if sentinel == nil { sentinel = fmt.Errorf("pip install failed: %w", runErr) } @@ -91,3 +89,16 @@ func (e *PipUpdateExecutor) Update(ctx context.Context, name, toVersion string, "duration_ms", durationMs) return nil } + +// runPipUpdateCommand runs `pip3 ` and returns stderr only (it feeds +// ClassifyPipStderr); stdout is captured and discarded. Command-level +// WaitDelay mirrors the previous inline exec. +func runPipUpdateCommand(ctx context.Context, args []string) (stderr string, runErr error) { + cmd := exec.CommandContext(ctx, pipBinary, args...) + cmd.WaitDelay = 2 * time.Second + var outBuf, errBuf bytes.Buffer + cmd.Stdout = &outBuf + cmd.Stderr = &errBuf + runErr = cmd.Run() + return errBuf.String(), runErr +}