diff --git a/cmd/cmdtest.go b/cmd/cmdtest.go index 54ea73bd..90e65c7f 100644 --- a/cmd/cmdtest.go +++ b/cmd/cmdtest.go @@ -22,6 +22,40 @@ var StubbedSuccessResponse = `{ // CallCmd runs the root command for integration-style tests. It passes isTerminal always true so // the default --output matches an interactive terminal (plaintext); non-TTY JSON defaults are // covered in root_test.go. +// CallCmdCapturingStderr runs a command and returns stdout and stderr separately, so +// a test can assert on output written deliberately to stderr — a transitional note, +// say — without it being mistaken for parseable output. +func CallCmdCapturingStderr( + t *testing.T, + clients APIClients, + trackerFn analytics.TrackerFn, + args []string, +) (stdout []byte, stderr []byte, err error) { + rootCmd, err := NewRootCommand( + config.NewService(&resources.MockClient{}), + trackerFn, + clients, + "test", + false, + func() bool { return true }, + nil, + ) + require.NoError(t, err) + cmd := rootCmd.Cmd() + out, errOut := bytes.NewBufferString(""), bytes.NewBufferString("") + cmd.SetOut(out) + cmd.SetErr(errOut) + cmd.SetArgs(args) + + tracker := trackerFn("", "", false) + if err := cmd.Execute(); err != nil { + tracker.SendCommandCompletedEvent(analytics.ERROR) + return out.Bytes(), errOut.Bytes(), err + } + tracker.SendCommandCompletedEvent(analytics.SUCCESS) + return out.Bytes(), errOut.Bytes(), nil +} + func CallCmd( t *testing.T, clients APIClients, diff --git a/cmd/setup/detect.go b/cmd/setup/detect.go index b7d0ced2..aa14e873 100644 --- a/cmd/setup/detect.go +++ b/cmd/setup/detect.go @@ -44,7 +44,17 @@ func runDetect(svc setup.Service) func(*cobra.Command, []string) error { outputKind := cliflags.GetOutputKind(cmd) if outputKind == "json" { - data, _ := json.Marshal(result) + // Candidates are added here rather than in the detection result: they + // report which tools are on this machine, which is not a fact about the + // project. Callers reading this need both. + payload := struct { + *setup.DetectResult + PackageManagerCandidates []setup.PMCandidate `json:"package_manager_candidates,omitempty"` + }{DetectResult: result} + if result.PackageManagerConfidence == setup.PMAmbiguous { + payload.PackageManagerCandidates = setup.PackageManagerChoiceFor(dir, result.SDKID).Candidates + } + data, _ := json.Marshal(payload) fmt.Fprintln(cmd.OutOrStdout(), string(data)) return nil } @@ -53,7 +63,12 @@ func runDetect(svc setup.Service) func(*cobra.Command, []string) error { if result.Framework != "" { fmt.Fprintf(cmd.OutOrStdout(), "Framework: %s\n", result.Framework) } - fmt.Fprintf(cmd.OutOrStdout(), "Package Manager: %s\n", result.PackageManager) + if result.PackageManagerConfidence == setup.PMAmbiguous { + fmt.Fprintf(cmd.OutOrStdout(), "Package Manager: %s (uncertain — %s)\n", + result.PackageManager, result.PackageManagerReason) + } else { + fmt.Fprintf(cmd.OutOrStdout(), "Package Manager: %s\n", result.PackageManager) + } fmt.Fprintf(cmd.OutOrStdout(), "Recommended SDK: %s\n", result.SDKID) if result.EntryPointExists { fmt.Fprintf(cmd.OutOrStdout(), "Entry Point: %s\n", result.EntryPoint) diff --git a/cmd/setup/install.go b/cmd/setup/install.go index 42b85465..96628eb4 100644 --- a/cmd/setup/install.go +++ b/cmd/setup/install.go @@ -34,6 +34,20 @@ func newInstallCmd(svc setup.Service) *cobra.Command { return cmd } +// candidateList renders the choices for an error message, marking the ones that +// are not installed so the user isn't sent to a tool they'd have to install first. +func candidateList(candidates []setup.PMCandidate) string { + names := make([]string, 0, len(candidates)) + for _, c := range candidates { + if c.Installed { + names = append(names, c.Name) + continue + } + names = append(names, c.Name+" (not installed)") + } + return strings.Join(names, ", ") +} + func runInstall(svc setup.Service) func(*cobra.Command, []string) error { return func(cmd *cobra.Command, args []string) error { dir, _ := cmd.Flags().GetString(pathFlag) @@ -48,6 +62,29 @@ func runInstall(svc setup.Service) func(*cobra.Command, []string) error { sdkID, _ := cmd.Flags().GetString(sdkIDFlag) pkgMgr, _ := cmd.Flags().GetString("package-manager") dryRun, _ := cmd.Flags().GetBool(dryRunFlag) + + // Without an explicit choice, read the project rather than falling back to + // pip or npm regardless of what the project uses. An ambiguous project is an + // error: guessing here would install with the wrong manager, and this command + // cannot ask. + if pkgMgr == "" { + choice := setup.PackageManagerChoiceFor(dir, sdkID) + if choice.Confidence == setup.PMAmbiguous { + return fmt.Errorf( + "cannot tell which package manager to use: %s\npass --package-manager with one of: %s", + choice.Reason, candidateList(choice.Candidates), + ) + } + pkgMgr = choice.Name + // This used to fall through to npm or pip whatever the project used, so a + // caller that relied on that default now gets a different manager. Say so + // once, on stderr, where it cannot disturb output being parsed. + fmt.Fprintf(cmd.ErrOrStderr(), + "note: --package-manager was not given, so setup read the project and chose %q. "+ + "This previously defaulted to npm or pip. Pass --package-manager to pin it.\n", + pkgMgr) + } + detection := &setup.DetectResult{ SDKID: sdkID, PackageManager: pkgMgr, diff --git a/cmd/setup/setup_test.go b/cmd/setup/setup_test.go index 17130c2f..78a22b78 100644 --- a/cmd/setup/setup_test.go +++ b/cmd/setup/setup_test.go @@ -220,6 +220,7 @@ func TestInstall_Plaintext(t *testing.T) { "setup", "install", "--access-token", "test-token", "--sdk-id", "node-server", + "--package-manager", "npm", } output, err := cmd.CallCmd( t, @@ -241,6 +242,7 @@ func TestInstall_Plaintext_WithVersion(t *testing.T) { "setup", "install", "--access-token", "test-token", "--sdk-id", "node-server", + "--package-manager", "npm", } output, err := cmd.CallCmd( t, @@ -319,6 +321,7 @@ func TestInstall_DryRun(t *testing.T) { "setup", "install", "--access-token", "test-token", "--sdk-id", "node-server", + "--package-manager", "npm", "--dry-run", } // No Installer provided: dry-run must not invoke it or shell out. @@ -341,6 +344,7 @@ func TestInstall_JSON(t *testing.T) { "setup", "install", "--access-token", "test-token", "--sdk-id", "node-server", + "--package-manager", "npm", "--output", "json", } output, err := cmd.CallCmd( @@ -362,6 +366,7 @@ func TestInstallStubReturnsError(t *testing.T) { "setup", "install", "--access-token", "test-token", "--sdk-id", "node-server", + "--package-manager", "npm", } _, err := cmd.CallCmd( t, @@ -412,3 +417,34 @@ func TestInitMissingRequiredFlags(t *testing.T) { require.Error(t, err) assert.Contains(t, err.Error(), "required flag") } + +// Omitting --package-manager used to fall through to npm or pip. The default now +// reads the project, so a caller relying on the old behaviour gets a note — on +// stderr, so output being parsed is untouched. +func TestInstall_AutoSelectedManager_WarnsOnStderr(t *testing.T) { + dir := t.TempDir() + require.NoError(t, os.WriteFile(filepath.Join(dir, "package.json"), + []byte(`{"packageManager":"pnpm@9.1.0"}`), 0600)) + + args := []string{ + "setup", "install", + "--access-token", "test-token", + "--sdk-id", "node-server", + "--path", dir, + "--dry-run", + } + stdout, stderr, err := cmd.CallCmdCapturingStderr( + t, + cmd.APIClients{ResourcesClient: &resources.MockClient{}}, + analytics.NoopClientFn{}.Tracker(), + args, + ) + + require.NoError(t, err) + assert.Contains(t, string(stderr), "--package-manager was not given") + assert.Contains(t, string(stderr), "previously defaulted to npm or pip") + assert.Contains(t, string(stderr), "pnpm") + // The note must not reach output a caller parses. + assert.NotContains(t, string(stdout), "--package-manager was not given") + assert.Contains(t, string(stdout), "pnpm add @launchdarkly/node-server-sdk") +} diff --git a/go.mod b/go.mod index 0449eb4a..87c1277d 100644 --- a/go.mod +++ b/go.mod @@ -27,6 +27,7 @@ require ( github.com/muesli/reflow v0.3.0 github.com/oapi-codegen/oapi-codegen/v2 v2.7.1 github.com/oapi-codegen/runtime v1.1.2 + github.com/pelletier/go-toml/v2 v2.2.4 github.com/pkg/browser v0.0.0-20240102092130-5ac0b6a4141c github.com/pkg/errors v0.9.1 github.com/samber/lo v1.51.0 @@ -81,7 +82,6 @@ require ( github.com/oasdiff/yaml3 v0.0.14 // indirect github.com/onsi/gomega v1.27.6 // indirect github.com/patrickmn/go-cache v2.1.0+incompatible // indirect - github.com/pelletier/go-toml/v2 v2.2.4 // indirect github.com/pmezard/go-difflib v1.0.1-0.20181226105442-5d4384ee4fb2 // indirect github.com/rivo/uniseg v0.4.7 // indirect github.com/sagikazarmark/locafero v0.11.0 // indirect diff --git a/internal/setup/detector.go b/internal/setup/detector.go index 5d9db9b8..5869f091 100644 --- a/internal/setup/detector.go +++ b/internal/setup/detector.go @@ -1,13 +1,16 @@ package setup import ( - "bytes" "encoding/json" "errors" + "fmt" "io/fs" "os" "path/filepath" + "regexp" "strings" + + "github.com/pelletier/go-toml/v2" ) // DetectResult contains information about the user's project detected from the working directory. @@ -21,6 +24,12 @@ type DetectResult struct { // suggest. Callers must not write initialization code into a suggested path // without telling the user, since the project does not load that file. EntryPointExists bool `json:"entry_point_exists"` + // PackageManagerConfidence says whether the project identifies its package + // manager or PackageManager is only a conventional default. Callers must not + // install against an ambiguous verdict without asking first. + PackageManagerConfidence PMConfidence `json:"package_manager_confidence,omitempty"` + // PackageManagerReason explains an ambiguous verdict, phrased for the user. + PackageManagerReason string `json:"package_manager_reason,omitempty"` } // Detector inspects a directory to determine the language, framework, package manager, @@ -60,6 +69,18 @@ func (FileDetector) Detect(dir string) (*DetectResult, error) { detectNode, } { if result := detect(dir); result != nil { + // One read of the project decides the manager and how sure we are, rather + // than each detector working it out again. An empty name means a language + // this does not model — Java's maven versus gradle — so the detector's own + // answer stands. Candidates carry which tools are installed, which + // describes the machine rather than the project, so they are left to + // callers that need to present a choice. + choice := PackageManagerChoiceFor(dir, result.SDKID) + if choice.Name != "" { + result.PackageManager = choice.Name + } + result.PackageManagerConfidence = choice.Confidence + result.PackageManagerReason = choice.Reason return result, nil } } @@ -184,19 +205,65 @@ func detectNode(dir string) *DetectResult { } func detectNodePM(dir string) string { - if _, err := os.Stat(filepath.Join(dir, "pnpm-lock.yaml")); err == nil { - return "pnpm" + return nodePMSignals(dir).best("npm") +} + +// exactSemver matches the exact versions corepack requires: a bare MAJOR.MINOR.PATCH +// with optional prerelease and build metadata, and no range operator. +var exactSemver = regexp.MustCompile(`^\d+\.\d+\.\d+(-[0-9A-Za-z.-]+)?(\+[0-9A-Za-z.-]+)?$`) + +// corepackPM reads the packageManager field, which names the manager and version +// the project expects. It is the most explicit statement a Node project can make, +// so it outranks lockfiles. +// https://nodejs.org/api/corepack.html +func corepackPM(dir string) string { + b, err := os.ReadFile(filepath.Join(dir, "package.json")) + if err != nil { + return "" } - if _, err := os.Stat(filepath.Join(dir, "yarn.lock")); err == nil { - return "yarn" + var pkg struct { + PackageManager string `json:"packageManager"` + } + if json.Unmarshal(b, &pkg) != nil { + return "" } - // Lockfiles: https://bun.com/docs/install/lockfile - for _, lock := range []string{"bun.lock", "bun.lockb"} { - if _, err := os.Stat(filepath.Join(dir, lock)); err == nil { - return "bun" + // The field must be "@". Corepack accepts nothing else, so + // a missing version ("No version specified for pnpm in packageManager") or a + // range ("Invalid package manager specification (pnpm@^11.13.0); expected a + // semver version") both stop the manager from running at all. Treating either as + // the project's declared manager would route the user into a command that cannot + // work, so the lockfiles decide instead, or the user is asked. + name, version, _ := strings.Cut(pkg.PackageManager, "@") + if !exactSemver.MatchString(version) { + return "" + } + switch name { + case "npm", "yarn", "pnpm", "bun": + return name + } + return "" +} + +// nodePMSignals reports what the project says about its Node package manager. +// Lockfiles: https://bun.com/docs/install/lockfile +func nodePMSignals(dir string) pmSignals { + if declared := corepackPM(dir); declared != "" { + return pmSignals{declared: declared} + } + var s pmSignals + for _, lock := range []struct{ file, pm string }{ + {"pnpm-lock.yaml", "pnpm"}, + {"yarn.lock", "yarn"}, + {"bun.lock", "bun"}, + {"bun.lockb", "bun"}, + {"package-lock.json", "npm"}, + {"npm-shrinkwrap.json", "npm"}, + } { + if _, err := os.Stat(filepath.Join(dir, lock.file)); err == nil { + s.addLocked(lock.pm) } } - return "npm" + return s } func detectGo(dir string) *DetectResult { @@ -219,8 +286,8 @@ func detectPython(dir string) *DetectResult { if _, err := os.Stat(filepath.Join(dir, indicator)); err == nil { ep, exists := EntryPointFor(dir, "python-server-sdk") return &DetectResult{ - Language: "Python", - PackageManager: detectPythonPM(dir), + Language: "Python", + // PackageManager is filled in by Detect from the same read. SDKID: "python-server-sdk", EntryPoint: ep, EntryPointExists: exists, @@ -236,23 +303,64 @@ func detectPython(dir string) *DetectResult { // // https://docs.astral.sh/uv/concepts/projects/layout/ // https://pipenv.pypa.io/en/latest/ -// https://python-poetry.org/docs/pyproject/ -func detectPythonPM(dir string) string { - if _, err := os.Stat(filepath.Join(dir, "uv.lock")); err == nil { - return "uv" - } - if _, err := os.Stat(filepath.Join(dir, "Pipfile")); err == nil { - return "pipenv" - } - if b, err := os.ReadFile(filepath.Join(dir, "pyproject.toml")); err == nil { - if bytes.Contains(b, []byte("[tool.poetry]")) { - return "poetry" + +// pythonPMSignals reports what the project says about its Python package manager. +// A lockfile is treated as the project having committed to a tool; a [tool.*] +// section counts the same way, since the tool owns that config. +func pythonPMSignals(dir string) pmSignals { + var s pmSignals + for _, lock := range []struct{ file, pm string }{ + {"uv.lock", "uv"}, + {"poetry.lock", "poetry"}, + {"pdm.lock", "pdm"}, + {"Pipfile.lock", "pipenv"}, + {"Pipfile", "pipenv"}, + } { + if _, err := os.Stat(filepath.Join(dir, lock.file)); err == nil { + s.addLocked(lock.pm) } - if bytes.Contains(b, []byte("[tool.uv]")) { - return "uv" + } + tools := configuredTools(dir) + for _, section := range []struct{ tool, pm string }{ + {"poetry", "poetry"}, + {"uv", "uv"}, + {"pdm", "pdm"}, + // hatch has no dependency-add command, so it is a signal we cannot act on. + // Recording it keeps the project ambiguous instead of silently falling + // through to pip. + {"hatch", ""}, + } { + if tools[section.tool] { + s.addLocked(section.pm) } } - return "pip" + return s +} + +// configuredTools reports which tools pyproject.toml configures, by the [tool.*] +// tables it declares. Reading the tables rather than matching text means a comment +// or a string that mentions another tool is not mistaken for a declaration, and +// nested tables need no special case: TOML creates the parent table implicitly, so +// [tool.hatch.build] on its own still declares hatch. +// +// A file we cannot parse declares nothing. That leaves the project ambiguous and the +// user asked, which is the honest answer when we cannot read what manages it. +func configuredTools(dir string) map[string]bool { + b, err := os.ReadFile(filepath.Join(dir, "pyproject.toml")) + if err != nil { + return nil + } + var doc struct { + Tool map[string]any `toml:"tool"` + } + if err := toml.Unmarshal(b, &doc); err != nil { + return nil + } + tools := make(map[string]bool, len(doc.Tool)) + for name := range doc.Tool { + tools[name] = true + } + return tools } func detectRuby(dir string) *DetectResult { @@ -282,10 +390,18 @@ func detectRuby(dir string) *DetectResult { // install` would succeed without recording the SDK for the app. // Gemfile: https://bundler.io/guides/gemfile.html func detectRubyPM(dir string) string { + return rubyPMSignals(dir).best("gem") +} + +// rubyPMSignals reports what the project says about its Ruby package manager. A +// Gemfile is Bundler's own manifest, so it settles the question; a gemspec alone +// does not, since the gem could be developed either way. +func rubyPMSignals(dir string) pmSignals { + var s pmSignals if _, err := os.Stat(filepath.Join(dir, "Gemfile")); err == nil { - return "bundle" + s.addLocked("bundle") } - return "gem" + return s } func detectJava(dir string) *DetectResult { @@ -518,7 +634,7 @@ func PackageManagerFor(dir, sdkID string) string { case "node-server", "js-client-sdk", "react-client-sdk", "react-native": return detectNodePM(dir) case "python-server-sdk": - return detectPythonPM(dir) + return PackageManagerChoiceFor(dir, sdkID).Name case "ruby-server-sdk": return detectRubyPM(dir) case "go-server-sdk": @@ -585,3 +701,149 @@ func findFileUnder(dir, root string, names ...string) string { } return "" } + +// PMConfidence says whether the project itself identifies its package manager. +type PMConfidence string + +const ( + // PMDefinite means the project names one manager and only one. + PMDefinite PMConfidence = "definite" + // PMAmbiguous means the project does not say, or contradicts itself. Callers + // must ask rather than guess. + PMAmbiguous PMConfidence = "ambiguous" +) + +// PMCandidate is a package manager the user could pick, with the command it would +// run. Installed reports whether the tool is on PATH; it describes the machine, not +// the project, so it never makes a candidate more likely to be the right one. +type PMCandidate struct { + Name string `json:"name"` + Installed bool `json:"installed"` + Command string `json:"command"` +} + +// PMChoice is the package manager for a project plus how sure we are. +type PMChoice struct { + Name string `json:"name"` + Confidence PMConfidence `json:"confidence"` + // Reason explains an ambiguous verdict in the words the picker shows the user. + Reason string `json:"reason,omitempty"` + Candidates []PMCandidate `json:"candidates,omitempty"` +} + +// pmSignals collects what a project says about its package manager. declared holds +// an explicit statement, which settles the question on its own; locked holds +// managers implied by lockfiles or tool config, where more than one means the +// project contradicts itself. An empty entry in locked marks a tool we recognise +// but cannot drive. +type pmSignals struct { + declared string + locked []string + unactionable bool +} + +func (s *pmSignals) addLocked(pm string) { + if pm == "" { + s.unactionable = true + return + } + for _, existing := range s.locked { + if existing == pm { + return + } + } + s.locked = append(s.locked, pm) +} + +// best returns the manager to use, falling back to fallback when the project says +// nothing. It preserves the old detection behaviour for callers that only want a +// name, including the first-match-wins ordering when lockfiles conflict. +func (s pmSignals) best(fallback string) string { + if s.declared != "" { + return s.declared + } + if len(s.locked) > 0 { + return s.locked[0] + } + return fallback +} + +// choose turns signals into a verdict. options lists every manager valid for the +// language, in the order the picker should show them, and fallback is the +// conventional default when the project is silent. +func (s pmSignals) choose(options []string, fallback string, argvFor func(string) []string) PMChoice { + candidates := make([]PMCandidate, 0, len(options)) + for _, name := range options { + argv := argvFor(name) + // Installed tracks the executable the command actually runs, not the label. + // "pip" resolves to pip3 on a stock macOS box, and reporting that as missing + // would steer the user away from the option that works. + installed := len(argv) > 0 && onPath(argv[0]) + candidates = append(candidates, PMCandidate{ + Name: name, + Installed: installed, + Command: strings.Join(argv, " "), + }) + } + + switch { + case s.declared != "": + return PMChoice{Name: s.declared, Confidence: PMDefinite} + // A manager the project committed to settles it even when an unactionable tool + // is also configured: hatchling is a common build backend for uv and poetry + // projects, and uv can add the dependency regardless of who builds the wheel. + case len(s.locked) == 1: + return PMChoice{Name: s.locked[0], Confidence: PMDefinite} + case len(s.locked) > 1: + // Say what was actually found. A lockfile, a Pipfile and a [tool.*] table all + // count as a project committing to a manager, so naming lockfiles would send + // the reader looking for files that are not there. + return PMChoice{ + Name: s.locked[0], + Confidence: PMAmbiguous, + Reason: fmt.Sprintf("this project is set up for more than one manager (%s)", + strings.Join(s.locked, ", ")), + Candidates: candidates, + } + case s.unactionable: + return PMChoice{ + Name: fallback, + Confidence: PMAmbiguous, + Reason: "this project is managed by a tool that cannot add dependencies for us", + Candidates: candidates, + } + default: + return PMChoice{ + Name: fallback, + Confidence: PMAmbiguous, + Reason: "this project doesn't say which package manager it uses", + Candidates: candidates, + } + } +} + +// PackageManagerChoiceFor reports the package manager for sdkID in dir and whether +// the project actually identifies it. Languages with a single toolchain are always +// definite; there is nothing to ask. +func PackageManagerChoiceFor(dir, sdkID string) PMChoice { + cmdFor := func(pm string) []string { + args, _ := InstallArgs(dir, sdkID, pm) + return args + } + + switch sdkID { + case "node-server", "js-client-sdk", "react-client-sdk", "react-native": + return nodePMSignals(dir).choose([]string{"npm", "yarn", "pnpm", "bun"}, "npm", cmdFor) + case "python-server-sdk": + return pythonPMSignals(dir).choose([]string{"pip", "uv", "poetry", "pipenv", "pdm"}, "pip", cmdFor) + case "ruby-server-sdk": + return rubyPMSignals(dir).choose([]string{"bundle", "gem"}, "gem", cmdFor) + case "go-server-sdk": + return PMChoice{Name: "go", Confidence: PMDefinite} + case "dotnet-server-sdk": + return PMChoice{Name: "dotnet", Confidence: PMDefinite} + default: + // Java, Android and Swift are installed by hand. + return PMChoice{Name: "", Confidence: PMDefinite} + } +} diff --git a/internal/setup/detector_shapes_test.go b/internal/setup/detector_shapes_test.go index 56df04eb..058370f3 100644 --- a/internal/setup/detector_shapes_test.go +++ b/internal/setup/detector_shapes_test.go @@ -367,7 +367,13 @@ func TestFileDetector_ProjectShapes(t *testing.T) { require.NoError(t, err) want := shape.want want.EntryPoint = filepath.Join(dir, want.EntryPoint) - assert.Equal(t, want, *result) + // These shapes assert language, SDK and entry point. Package-manager + // confidence has its own tests, so it is cleared rather than restated on + // every shape. + got := *result + got.PackageManagerConfidence = "" + got.PackageManagerReason = "" + assert.Equal(t, want, got) }) } } diff --git a/internal/setup/detector_test.go b/internal/setup/detector_test.go index 5ab223e2..02e6e047 100644 --- a/internal/setup/detector_test.go +++ b/internal/setup/detector_test.go @@ -850,3 +850,293 @@ func TestPackageManagerFor_DerivesFromProjectNotDetectedLanguage(t *testing.T) { assert.Empty(t, PackageManagerFor(t.TempDir(), "java-server-sdk")) }) } + +func TestPackageManagerChoice_Definite(t *testing.T) { + tests := []struct { + name string + files map[string]string + sdkID string + want string + }{ + // The most explicit statement a Node project can make outranks lockfiles. + {"corepack field", map[string]string{ + "package.json": `{"packageManager":"pnpm@9.1.0"}`, + }, "node-server", "pnpm"}, + {"corepack field with prerelease", map[string]string{ + "package.json": `{"packageManager":"pnpm@9.1.0-beta.1"}`, + }, "node-server", "pnpm"}, + // corepack writes this hash form itself when it pins a manager. + {"corepack field with build metadata", map[string]string{ + "package.json": `{"packageManager":"yarn@4.1.0+sha224.abcdef"}`, + }, "node-server", "yarn"}, + {"corepack field beats a conflicting lockfile", map[string]string{ + "package.json": `{"packageManager":"pnpm@9.1.0"}`, + "yarn.lock": "", + }, "node-server", "pnpm"}, + {"single node lockfile", map[string]string{ + "package.json": `{}`, + "pnpm-lock.yaml": "", + }, "node-server", "pnpm"}, + {"package-lock only", map[string]string{ + "package.json": `{}`, + "package-lock.json": "", + }, "node-server", "npm"}, + {"uv lockfile", map[string]string{"uv.lock": ""}, "python-server-sdk", "uv"}, + {"poetry lockfile", map[string]string{"poetry.lock": ""}, "python-server-sdk", "poetry"}, + {"tool.uv section", map[string]string{ + "pyproject.toml": "[project]\nname=\"a\"\n[tool.uv]\n", + }, "python-server-sdk", "uv"}, + {"tool.pdm section", map[string]string{ + "pyproject.toml": "[project]\nname=\"a\"\n[tool.pdm]\n", + }, "python-server-sdk", "pdm"}, + {"Gemfile", map[string]string{"Gemfile": "source 'x'"}, "ruby-server-sdk", "bundle"}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + dir := t.TempDir() + for name, body := range tt.files { + require.NoError(t, os.WriteFile(filepath.Join(dir, name), []byte(body), 0600)) + } + + choice := PackageManagerChoiceFor(dir, tt.sdkID) + + assert.Equal(t, PMDefinite, choice.Confidence) + assert.Equal(t, tt.want, choice.Name) + assert.Empty(t, choice.Candidates, "a definite verdict needs no choice") + }) + } +} + +func TestPackageManagerChoice_Ambiguous(t *testing.T) { + tests := []struct { + name string + files map[string]string + sdkID string + wantReason string + wantOptions []string + }{ + // No detection tuning can fix this: the project contradicts itself. + {"conflicting node lockfiles", map[string]string{ + "package.json": `{}`, + "yarn.lock": "", + "package-lock.json": "", + }, "node-server", "more than one manager", []string{"npm", "yarn", "pnpm", "bun"}}, + {"bare package.json", map[string]string{ + "package.json": `{}`, + }, "node-server", "doesn't say", []string{"npm", "yarn", "pnpm", "bun"}}, + // Corepack needs one exact version, so honouring anything else would route the + // user into a manager that refuses to run. + {"packageManager without a version", map[string]string{ + "package.json": `{"packageManager":"pnpm"}`, + }, "node-server", "doesn't say", []string{"npm", "yarn", "pnpm", "bun"}}, + {"packageManager with a caret range", map[string]string{ + "package.json": `{"packageManager":"pnpm@^11.13.0"}`, + }, "node-server", "doesn't say", []string{"npm", "yarn", "pnpm", "bun"}}, + {"packageManager with a comparator range", map[string]string{ + "package.json": `{"packageManager":"pnpm@>=11"}`, + }, "node-server", "doesn't say", []string{"npm", "yarn", "pnpm", "bun"}}, + {"packageManager with a partial version", map[string]string{ + "package.json": `{"packageManager":"pnpm@11"}`, + }, "node-server", "doesn't say", []string{"npm", "yarn", "pnpm", "bun"}}, + // uv does not require committing the lock, and PEP 621 has no uv marker. + {"PEP 621 pyproject only", map[string]string{ + "pyproject.toml": "[project]\nname=\"a\"\n", + }, "python-server-sdk", "doesn't say", []string{"pip", "uv", "poetry", "pipenv", "pdm"}}, + {"requirements.txt only", map[string]string{ + "requirements.txt": "flask\n", + }, "python-server-sdk", "doesn't say", []string{"pip", "uv", "poetry", "pipenv", "pdm"}}, + // hatch cannot add dependencies for us, so we must not pick it or silently pip. + {"hatch project", map[string]string{ + "pyproject.toml": "[project]\nname=\"a\"\n[tool.hatch]\n", + }, "python-server-sdk", "cannot add dependencies", []string{"pip", "uv", "poetry", "pipenv", "pdm"}}, + {"gemspec without Gemfile", map[string]string{ + "a.gemspec": "Gem::Specification.new", + }, "ruby-server-sdk", "doesn't say", []string{"bundle", "gem"}}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + dir := t.TempDir() + for name, body := range tt.files { + require.NoError(t, os.WriteFile(filepath.Join(dir, name), []byte(body), 0600)) + } + + choice := PackageManagerChoiceFor(dir, tt.sdkID) + + assert.Equal(t, PMAmbiguous, choice.Confidence) + assert.Contains(t, choice.Reason, tt.wantReason) + names := make([]string, 0, len(choice.Candidates)) + for _, c := range choice.Candidates { + names = append(names, c.Name) + assert.NotEmpty(t, c.Command, "every candidate needs the command it would run") + } + assert.Equal(t, tt.wantOptions, names) + }) + } +} + +// Installed state describes the machine, so it must never change the verdict. +func TestPackageManagerChoice_InstalledStateDoesNotDecide(t *testing.T) { + dir := t.TempDir() + require.NoError(t, os.WriteFile(filepath.Join(dir, "package.json"), []byte(`{}`), 0600)) + + stubPath(t, "pnpm") // the only manager on this machine + choice := PackageManagerChoiceFor(dir, "node-server") + + assert.Equal(t, PMAmbiguous, choice.Confidence, "one installed tool is not evidence about the project") + assert.Equal(t, "npm", choice.Name, "the conventional default stands until the user picks") + for _, c := range choice.Candidates { + assert.Equal(t, c.Name == "pnpm", c.Installed) + } +} + +func TestPackageManagerChoice_SingleToolchainsAreAlwaysDefinite(t *testing.T) { + for sdkID, want := range map[string]string{ + "go-server-sdk": "go", + "dotnet-server-sdk": "dotnet", + "java-server-sdk": "", + } { + t.Run(sdkID, func(t *testing.T) { + choice := PackageManagerChoiceFor(t.TempDir(), sdkID) + assert.Equal(t, PMDefinite, choice.Confidence) + assert.Equal(t, want, choice.Name) + }) + } +} + +// Real pyproject files rarely carry a bare [tool.x] header; the tables that matter +// are nested. Matching only the bare header made these signals near-dead. +func TestPackageManagerChoice_NestedToolTables(t *testing.T) { + tests := []struct { + name string + pyproject string + wantName string + wantDefinite bool + }{ + {"hatch build table only", "[project]\nname=\"a\"\n[tool.hatch.build.targets.wheel]\npackages=[\"a\"]\n", "pip", false}, + {"hatch version table only", "[project]\nname=\"a\"\n[tool.hatch.version]\npath=\"a/__init__.py\"\n", "pip", false}, + {"poetry dependencies table only", "[project]\nname=\"a\"\n[tool.poetry.dependencies]\npython=\"^3.12\"\n", "poetry", true}, + {"uv sources table only", "[project]\nname=\"a\"\n[tool.uv.sources]\nx={git=\"...\"}\n", "uv", true}, + {"pdm dev-dependencies table only", "[project]\nname=\"a\"\n[tool.pdm.dev-dependencies]\ntest=[]\n", "pdm", true}, + // The trailing delimiter matters: [tool.uv] must not match [tool.uvicorn]. + {"uvicorn is not uv", "[project]\nname=\"a\"\n[tool.uvicorn]\nport=8000\n", "pip", false}, + {"hatchling ruff etc are not hatch", "[project]\nname=\"a\"\n[tool.ruff]\nline-length=100\n", "pip", false}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + dir := t.TempDir() + require.NoError(t, os.WriteFile(filepath.Join(dir, "pyproject.toml"), []byte(tt.pyproject), 0600)) + + choice := PackageManagerChoiceFor(dir, "python-server-sdk") + + assert.Equal(t, tt.wantName, choice.Name) + if tt.wantDefinite { + assert.Equal(t, PMDefinite, choice.Confidence) + } else { + assert.Equal(t, PMAmbiguous, choice.Confidence) + } + }) + } +} + +// A PDM project that commits only its lockfile is still a PDM project. +func TestPackageManagerChoice_PdmLockfile(t *testing.T) { + dir := t.TempDir() + require.NoError(t, os.WriteFile(filepath.Join(dir, "pdm.lock"), []byte(""), 0600)) + + choice := PackageManagerChoiceFor(dir, "python-server-sdk") + + assert.Equal(t, PMDefinite, choice.Confidence) + assert.Equal(t, "pdm", choice.Name) +} + +// hatchling is a common build backend for uv and poetry projects. The manager the +// project committed to can still add the dependency, whoever builds the wheel. +func TestPackageManagerChoice_ActionableSignalBeatsHatch(t *testing.T) { + tests := []struct { + name string + files map[string]string + want string + }{ + {"uv lockfile alongside hatch build backend", map[string]string{ + "pyproject.toml": "[project]\nname=\"a\"\n[tool.hatch.build.targets.wheel]\npackages=[\"a\"]\n", + "uv.lock": "", + }, "uv"}, + {"uv table alongside hatch", map[string]string{ + "pyproject.toml": "[project]\nname=\"a\"\n[tool.uv]\n[tool.hatch.version]\npath=\"x\"\n", + }, "uv"}, + {"poetry alongside hatch", map[string]string{ + "pyproject.toml": "[project]\nname=\"a\"\n[tool.poetry]\n[tool.hatch.build]\n", + }, "poetry"}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + dir := t.TempDir() + for name, body := range tt.files { + require.NoError(t, os.WriteFile(filepath.Join(dir, name), []byte(body), 0600)) + } + + choice := PackageManagerChoiceFor(dir, "python-server-sdk") + + assert.Equal(t, PMDefinite, choice.Confidence, + "setup would refuse to install though %s can add the dependency", tt.want) + assert.Equal(t, tt.want, choice.Name) + }) + } +} + +// pyproject.toml is read as TOML rather than searched as text, so only the tables it +// actually declares count as a project committing to a tool. +func TestPackageManagerChoice_ToolTablesAreParsedNotMatched(t *testing.T) { + tests := []struct { + name string + pyproject string + wantName string + wantDefinite bool + }{ + // A note about the tool a project migrated away from is not a declaration. + {"comment mentions another tool", "[project]\nname=\"a\"\n# migrated away from [tool.poetry] in March\n[tool.uv]\n", "uv", true}, + // Nor is a table name inside a string. + {"multi-line string mentions another tool", + "[project]\nname=\"a\"\ndescription=\"\"\"\nsee [tool.poetry] for history\n\"\"\"\n[tool.uv]\n", "uv", true}, + {"single-quoted string mentions another tool", + "[project]\nname=\"a\"\nsummary='see [tool.poetry]'\n[tool.uv]\n", "uv", true}, + // A trailing comment on the header itself is still a declaration. + {"header with a trailing comment", "[project]\nname=\"a\"\n[tool.uv] # the real one\n", "uv", true}, + // Parent tables are implicit, so a nested table declares its tool. + {"nested table only", "[project]\nname=\"a\"\n[tool.poetry.dependencies]\npython=\"^3.12\"\n", "poetry", true}, + // A different tool whose name merely starts the same way is not a match. + {"similarly named tool", "[project]\nname=\"a\"\n[tool.uvicorn]\nport=8000\n", "pip", false}, + // Unreadable TOML declares nothing, so the user is asked rather than guessed at. + {"malformed toml", "[project\nname=\"a\"\n[tool.uv]\n", "pip", false}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + dir := t.TempDir() + require.NoError(t, os.WriteFile(filepath.Join(dir, "pyproject.toml"), []byte(tt.pyproject), 0600)) + + choice := PackageManagerChoiceFor(dir, "python-server-sdk") + + assert.Equal(t, tt.wantName, choice.Name) + if tt.wantDefinite { + assert.Equal(t, PMDefinite, choice.Confidence) + } else { + assert.Equal(t, PMAmbiguous, choice.Confidence) + } + }) + } +} + +// A [tool.*] table and a Pipfile count as commitments too, so the reason must not +// send the reader looking for lockfiles that are not there. +func TestPackageManagerChoice_ConflictReasonNamesWhatWasFound(t *testing.T) { + dir := t.TempDir() + require.NoError(t, os.WriteFile(filepath.Join(dir, "pyproject.toml"), + []byte("[project]\nname=\"a\"\n[tool.uv]\n[tool.poetry]\n"), 0600)) + + choice := PackageManagerChoiceFor(dir, "python-server-sdk") + + require.Equal(t, PMAmbiguous, choice.Confidence) + assert.Contains(t, choice.Reason, "set up for more than one manager") + assert.NotContains(t, choice.Reason, "lockfile", + "neither signal here is a lockfile") +} diff --git a/internal/setup/installer.go b/internal/setup/installer.go index cab87917..170c4e14 100644 --- a/internal/setup/installer.go +++ b/internal/setup/installer.go @@ -143,6 +143,15 @@ func (p PackageInstaller) Install(dir string, detection *DetectResult) (*Install out, err := runner(dir, args) command := strings.Join(args, " ") if err != nil { + if reason := packageManagerSpecReason(out); reason != "" { + return &InstallResult{ + SDKID: detection.SDKID, + Package: pkg, + Command: command, + Failed: true, + FailureReason: reason, + }, nil + } if reason := externallyManagedReason(dir, out); reason != "" { // No Command: the reason says not to run this pip, and the done screen // offers a non-empty Command as "install it yourself with". @@ -190,6 +199,31 @@ func dotnetProjectArg(dir string) (args []string, reason string) { } } +// packageManagerSpecReason recognises a Node manager refusing to run because the +// packageManager field in package.json is not a spec corepack accepts: it requires +// an exact version, so both a missing one and a range are rejected. The manifest is +// malformed rather than the command wrong, and repairing someone's manifest is not +// ours to do, so say what is wrong and let them fix it. +func packageManagerSpecReason(out []byte) string { + // package.json has to be named in the output. Both corepack refusals mention it, + // and without that check any failure whose text happens to mention a missing or + // non-semver version — from a gem, a Python package, a Go module — would have its + // real error replaced by advice about a field it does not have. + if !bytes.Contains(out, []byte("package.json")) { + return "" + } + badSpec := bytes.Contains(out, []byte("No version specified")) || + bytes.Contains(out, []byte("expected a semver version")) || + bytes.Contains(out, []byte("Invalid package manager specification")) + if !badSpec { + return "" + } + return "the packageManager field in package.json is not a specification your package " + + "manager accepts: it needs one exact version, so a missing version or a range such as " + + "\"pnpm@^11.13.0\" is refused. Pin it (for example \"pnpm@11.13.0\") or remove the field, " + + "then run setup again." +} + // externallyManagedReason recognises a PEP 668 refusal and says what to do about // it. Homebrew and most current Linux distributions mark their Python as managed // by the OS package manager, so pip declines to write into it. A virtualenv is the @@ -269,6 +303,7 @@ var installHints = map[string]string{ "poetry": "see https://python-poetry.org/docs/#installation", "uv": "see https://docs.astral.sh/uv/getting-started/installation", "pipenv": "see https://pipenv.pypa.io/en/latest/installation.html", + "pdm": "see https://pdm-project.org/en/latest/#installation", "npm": "install Node.js from https://nodejs.org", "yarn": "see https://yarnpkg.com/getting-started/install", "pnpm": "see https://pnpm.io/installation", @@ -369,6 +404,8 @@ func pythonInstallCmd(dir, pm, pkg string) []string { return []string{"uv", "add", pkg} case "pipenv": return []string{"pipenv", "install", pkg} + case "pdm": + return []string{"pdm", "add", pkg} default: return pipInstallCmd(dir, pkg) } diff --git a/internal/setup/installer_test.go b/internal/setup/installer_test.go index 62a194d1..96390154 100644 --- a/internal/setup/installer_test.go +++ b/internal/setup/installer_test.go @@ -881,3 +881,61 @@ func TestInstall_PinnedSdkVersion_NoWarning(t *testing.T) { require.NoError(t, err) assert.Empty(t, result.Warning) } + +// Corepack needs one exact version, so both a missing version and a range stop the +// manager running. Repairing someone's manifest is not ours to do, so the failure +// has to say what is wrong. +func TestInstall_BadPackageManagerSpec_ExplainsIt(t *testing.T) { + for _, out := range []string{ + `No version specified for pnpm in "packageManager" of package.json`, + "Invalid package manager specification in package.json (pnpm@^11.13.0); expected a semver version", + } { + t.Run(out[:24], func(t *testing.T) { + stubVirtualEnv(t, "") + stubPath(t, "pnpm") + installer := PackageInstaller{ + run: func(string, []string) ([]byte, error) { + return []byte(out), errors.New("exit status 1") + }, + } + + result, err := installer.Install(t.TempDir(), &DetectResult{ + SDKID: "node-server", PackageManager: "pnpm", + }) + + require.NoError(t, err, "a malformed manifest must not dead-end the flow") + assert.True(t, result.Failed) + assert.Contains(t, result.FailureReason, "packageManager field") + assert.Contains(t, result.FailureReason, "one exact version") + assert.Contains(t, result.FailureReason, "pnpm@11.13.0") + }) + } +} + +// Only a Node failure about package.json may be answered with advice about that +// file. Another ecosystem's error keeps its own text, whatever phrases it contains. +func TestInstall_OtherEcosystemErrorsKeepTheirText(t *testing.T) { + for _, out := range []string{ + "ERROR: Could not find a valid gem 'x' (>= 0), here is why:\n No version specified", + "ERROR: Could not find a version that satisfies the requirement x; expected a semver version", + "go: module x: invalid version: expected a semver version", + } { + t.Run(out[:20], func(t *testing.T) { + stubVirtualEnv(t, "") + stubPath(t, "npm") + installer := PackageInstaller{ + run: func(string, []string) ([]byte, error) { + return []byte(out), errors.New("exit status 1") + }, + } + + _, err := installer.Install(t.TempDir(), &DetectResult{ + SDKID: "node-server", PackageManager: "npm", + }) + + require.Error(t, err, "the real failure must reach the caller") + assert.Contains(t, err.Error(), out, "the real error text is kept") + assert.NotContains(t, err.Error(), "packageManager field") + }) + } +}