diff --git a/README.md b/README.md index 2680ca7..54121b6 100644 --- a/README.md +++ b/README.md @@ -258,7 +258,12 @@ verdict claims the documented step works: - `BLOCKED` with the reason `checkout-state` means a line that would have been a plain `FAIL` ran in the repository's own checkout and its output says the target already exists or is already initialized. The collision is with the checkout kibble chose, so - the document may be fine. + the document may be fine. A Go build that finds two packages at the top of the checkout + reads the same way when the document wrote one of the two files and the repository + already held the other. So does a documented tool saying the top of the checkout is not + a repository of its own kind and naming its own init command, when no earlier line of + the document that passed made or cloned one. That line was not tested, and it is not + counted as passing. - A passing line that writes a titled fence to its file reads `VERIFIED` "wrote X as the document shows it". The document supplied the content, so only the write is claimed. - An install whose product is a shell function the documented sourcing defines is @@ -312,7 +317,7 @@ What the weekly job prints, with each repository's counts in VERIFIED, FAIL, GAP ``` ripgrep: 3,0,0 as expected -nodemon: 2,0,0 as expected +nodemon: 3,0,0 as expected fd: 6,0,0 as expected glow: 5,0,0 as expected httpie: 0,0,0 as expected diff --git a/corpus/repos.tsv b/corpus/repos.tsv index 402fa4f..d246323 100644 --- a/corpus/repos.tsv +++ b/corpus/repos.tsv @@ -32,7 +32,16 @@ # this commit and at 034d3c1 reads 3,0,0, and those two lines are the whole # difference. The change is 81ed0b7. ripgrep https://github.com/BurntSushi/ripgrep.git 3fce3b5bb0236da2df6d99672afb8a719642eca7 3,0,0 -nodemon https://github.com/remy/nodemon.git 5de021f6cd5c71814b6c2d1c60af3cc01fa2cc69 2,0,0 +# nodemon once expected 2,0,0. Its README installs it globally and, further down, +# with npm install --save-dev nodemon. Since kibble runs a local npm install for +# a package the checkout publishes (7554920), that second line is a route of its +# own and both installs verify. The two routes print the same help, and equal +# check rows of one binary collapse to one (5b80b1a), so the flag check still +# counts once. +# Run on the same commit, kibble before 7554920 reads 2,0,0, at 7554920 4,0,0 +# with the flag check doubled, and with the collapse 3,0,0. The extra install is +# the whole difference. +nodemon https://github.com/remy/nodemon.git 5de021f6cd5c71814b6c2d1c60af3cc01fa2cc69 3,0,0 # fd is the one row that moves on a quiet network. Its npm package fetches a # prebuilt binary, and when that fetch fails the two checks reading the # installed tool have nothing to read, so 6,0,0 becomes 4,0,0. That is now an diff --git a/dependent.go b/dependent.go index b379ade..c751ff2 100644 --- a/dependent.go +++ b/dependent.go @@ -2,6 +2,7 @@ package main import ( "fmt" + "path" "regexp" "strings" @@ -52,7 +53,11 @@ func resolveMissingBinaries(run *exampleRun, plan *kplan.Plan, have map[string]b // the session is no longer a clean test of this line, which is why these // become blocked rather than skipped: the cascade stops being reported as // several broken lines without any of them being called fine. A gap counts as -// not having run, since the document's own hole stopped the line. +// not having run, since the document's own hole stopped the line. A Go build +// that finds the document's file beside the checkout's own package is blocked +// here too, since it takes the earlier file writes of the run to see it. So is +// a documented tool saying the top of the checkout is not a repository of its +// own kind, since it takes the document's earlier lines to see none made one. func resolveDependentFailures(run *exampleRun, plan *kplan.Plan) { bins := map[string]bool{} for _, b := range plan.Binaries { @@ -85,20 +90,58 @@ func resolveDependentFailures(run *exampleRun, plan *kplan.Plan) { // Earlier line results per document, for the shared-operand rule, which // reads across the document's steps rather than within one. prior := map[string][]lineResult{} + // Go files the checkout held at its top before the session started. + checkout := map[string]bool{} + for _, f := range plan.RootGoFiles { + checkout[f] = true + } + // Files the current document's file blocks wrote from the top of the + // checkout, in run order. The session restores the checkout between + // documents when it could save it, so an earlier document's files are + // gone by the next one and are never counted for it. + written, writtenDoc := map[string]bool{}, "" for si := range run.Steps { s := &run.Steps[si] + if s.Doc != writtenDoc { + written, writtenDoc = map[string]bool{}, s.Doc + } for li := range s.Lines { l := &s.Lines[li] if l.Status != StatusFail { + m := reFileWrite.FindStringSubmatch(l.Cmd) + if m != nil && l.Status == StatusVerified && l.cwd == sessionRepoDir { + written[m[1]] = true + } prior[s.Doc] = append(prior[s.Doc], *l) continue } + if doc, own := mixedCheckoutPackages(l.output, written, checkout); doc != "" { + l.Status = StatusBlocked + l.Reason = ReasonCheckoutState + l.Detail = fmt.Sprintf("wrote %s into the repository's own checkout, beside %s of "+ + "another Go package, so the claim was not tested as written: %s", doc, own, l.Detail) + continue + } + if bin := notItsRepository(l, prior[s.Doc], bins); bin != "" { + l.Status = StatusBlocked + l.Reason = ReasonCheckoutState + l.Detail = fmt.Sprintf("ran at the top of the checkout, which %s reports is not a %s "+ + "repository, and no line of this document that ran created one, so the claim was "+ + "not tested as written: %s", bin, bin, l.Detail) + continue + } if cited := citedSkipped(l.output, skippedCmds); cited != "" { l.Status = StatusBlocked l.Reason = ReasonDependsOnSkipped l.Detail = fmt.Sprintf("failed naming `%s`, which did not run", cited) continue } + if word := skippedProvider(s.Lines[:li], l.output); word != "" { + l.Status = StatusBlocked + l.Reason = ReasonMissingFixture + l.Detail = fmt.Sprintf("follows a skipped line that would have provided %s", word) + continue + } if prior := earlierGapInFamily(s.Lines[:li], l.Cmd, bins); prior != "" { l.Status = StatusBlocked l.Reason = ReasonDependsOnSkipped @@ -119,6 +162,156 @@ func resolveDependentFailures(run *exampleRun, plan *kplan.Plan) { } } +var ( + // reFileWrite matches the line a planned file block runs and captures the + // path it writes, relative to the directory the line ran in. The + // terminator may carry a number after the part matched here. + reFileWrite = regexp.MustCompile(`^(?:mkdir -p '[^']+' && )?cat > '([^']+)' <<'KIBBLE_FILE_END`) + // reMixedPackages matches Go refusing a directory that holds two packages + // and captures the file it names for each and the directory. + reMixedPackages = regexp.MustCompile( + `found packages \w+ \(([^)\s]+\.go)\) and \w+ \(([^)\s]+\.go)\) in (\S+)`) +) + +// mixedCheckoutPackages returns the document's file and the checkout's file +// when Go reports two packages in the repository root, the document's file +// blocks wrote one of the two files it names, and the checkout held the other +// before the session started. sqlc's tutorials tell the reader in prose to work +// in a new directory, so the session stays in the checkout and writes +// tutorial.go beside sqlc's own root package. A clash between two files the +// document wrote is the document's own, and so is a clash with a file its +// commands made some other way, such as a generator's output. A file written +// into a subdirectory is not the one Go names at the root, and a clash in any +// other directory is left as it reads, since only the root's files are known +// to be the checkout's. +func mixedCheckoutPackages(output string, written, checkout map[string]bool) (doc, own string) { + m := reMixedPackages.FindStringSubmatch(output) + if m == nil || m[3] != sessionRepoDir { + return "", "" + } + switch a, b := m[1], m[2]; { + case written[a] && !written[b] && checkout[b]: + return a, b + case written[b] && !written[a] && checkout[a]: + return b, a + } + return "", "" +} + +// reMadeRepository matches a command holding init or clone as a whole word, +// the two ways a tool makes a repository of its own kind. +var reMadeRepository = regexp.MustCompile(`\b(?:init|clone)\b`) + +// notItsRepository returns the documented binary a failing line invoked when +// that tool reports the top of the checkout is not a repository of its own +// kind. A version control tool's guide is written for a reader standing in +// such a repository, and the session stands in a plain clone of the project. +// The tool must say there is no repository of its name there and must name its +// own init command, so the report and the remedy are both the tool's words. +// Every such report must name the directory the line ran in or the checkout's +// own path, since a report about any other directory is about one the document +// named. A line that ran in another directory, or that stepped into one and +// back, went where the document sent it. A usage error is the document's own +// wording, and a crash convicts the line whatever else it printed. An earlier +// line of the same document that passed and made or cloned a repository with +// the tool means the document did set one up, so its failure at the top of the +// checkout stands. +func notItsRepository(l *lineResult, earlier []lineResult, bins map[string]bool) string { + if l.cwd != sessionRepoDir || changesDir(l.Cmd) { + return "" + } + bin, _ := kplan.InvokedBinary(l.Cmd, bins) + if bin == "" { + return "" + } + out, name := stripANSI(l.output), regexp.QuoteMeta(bin) + noRepo := regexp.MustCompile( + `(?i)there is no ` + name + ` repo(?:sitory)? in ["'\x60]?([^\s"'\x60>]*)`) + initCmd := regexp.MustCompile(`(?i)\b` + name + `(?: \w+)? init\b`) + reports := noRepo.FindAllStringSubmatch(out, -1) + if reports == nil || !initCmd.MatchString(out) { + return "" + } + for _, m := range reports { + if m[1] != "." && strings.TrimSuffix(m[1], "/") != sessionRepoDir { + return "" + } + } + if reUsageError.MatchString(out) || reCrash.MatchString(out) { + return "" + } + for _, p := range earlier { + if p.Status == StatusVerified && reMadeRepository.MatchString(p.Cmd) && runsBinary(p.Cmd, bin) { + return "" + } + } + return bin +} + +// runsBinary reports whether any command of a line runs bin, stepping over +// wrappers such as env as InvokedBinary does. +func runsBinary(cmd, bin string) bool { + line, ok := shell.Parse(cmd) + if !ok { + return false + } + for _, c := range line.Cmds { + if shell.Invoked(c).Name() == bin { + return true + } + } + return false +} + +var ( + // reAbsentQuoted matches output reporting a quoted name as absent, such + // as Go's `no such tool "air"`, and captures the name. + reAbsentQuoted = regexp.MustCompile( + "(?i)(?:no such (?:tool|file|command)|not found)[^\"'`\\n]{0,20}[\"'`]([\\w.-]+)[\"'`]" + + "|[\"'`]([\\w.-]+)[\"'`][^\\n]{0,20}(?:not found|no such file)") + // reAbsentBare matches a shell reporting a bare name as absent, such as + // "air: command not found", and captures the name. + reAbsentBare = regexp.MustCompile(`(?i)(?:^|[\s:])([\w.-]+): (?:command )?not found`) +) + +// skippedProvider returns the name a failing line reports as absent when an +// earlier line of the same block was skipped for a missing fixture or +// dependency and would have provided it. air's README skips `go get -tool +// github.com/air-verse/air@latest`, since it adds a tool to the reader's own +// module, and the next line fails with `go: no such tool "air"`. The name +// must be a whole argument of the skipped line or the last element of a +// module path in it, never part of a URL, and a placeholder skip provides +// nothing. +func skippedProvider(earlier []lineResult, output string) string { + var names []string + for _, line := range strings.Split(output, "\n") { + for _, m := range reAbsentQuoted.FindAllStringSubmatch(line, -1) { + names = append(names, m[1]+m[2]) + } + for _, m := range reAbsentBare.FindAllStringSubmatch(line, -1) { + names = append(names, m[1]) + } + } + for _, prev := range earlier { + if prev.Status != StatusSkipped || + (prev.Reason != ReasonMissingFixture && prev.Reason != ReasonMissingDependency) { + continue + } + for _, f := range strings.Fields(prev.Cmd) { + if strings.Contains(f, "://") { + continue + } + mod, _, _ := strings.Cut(f, "@") + for _, n := range names { + if n != "" && (f == n || path.Base(mod) == n) { + return n + } + } + } + } + return "" +} + // sharedOperandNotPassed returns a file-shaped operand of a failing line and // the first earlier line of the same document that also named it and was // skipped, blocked, or stopped by a gap. A query on a database whose load was diff --git a/dependent_adversarial_test.go b/dependent_adversarial_test.go index 6ec46a7..9049401 100644 --- a/dependent_adversarial_test.go +++ b/dependent_adversarial_test.go @@ -2,6 +2,7 @@ package main import ( "fmt" + "slices" "testing" "github.com/google/go-cmp/cmp" @@ -52,6 +53,36 @@ func TestSuppressionNeedsMoreThanAMention(t *testing.T) { Failing: lineResult{Cmd: "tool deploy", Status: StatusFail, output: "error: bad flag\nTry tool help for usage."}, Want: StatusFail, + }, { // Test 4: air's skipped go get would have provided the tool the next + // line reports absent. + Name: "skipped provider", + Skipped: lineResult{Cmd: "go get -tool github.com/air-verse/air@latest", Status: StatusSkipped, + Reason: ReasonMissingFixture}, + Failing: lineResult{Cmd: "go tool air -v", Status: StatusFail, + output: `go: no such tool "air"`}, + Want: StatusBlocked, + }, { // Test 5: the same skip does not excuse an unrelated failure. + Name: "skipped provider, unrelated failure", + Skipped: lineResult{Cmd: "go get -tool github.com/air-verse/air@latest", Status: StatusSkipped, + Reason: ReasonMissingFixture}, + Failing: lineResult{Cmd: "go tool air -v", Status: StatusFail, + output: "open /work/repo/.air.toml: permission denied"}, + Want: StatusFail, + }, { // Test 6: a placeholder skip provides nothing. + Name: "placeholder skip", + Skipped: lineResult{Cmd: "go get -tool github.com//air@latest", Status: StatusSkipped, + Reason: ReasonPlaceholder}, + Failing: lineResult{Cmd: "go tool air -v", Status: StatusFail, + output: `go: no such tool "air"`}, + Want: StatusFail, + }, { // Test 7: a name that appears only inside a URL of the skipped line + // was not provided by it. + Name: "provider named in a url", + Skipped: lineResult{Cmd: "curl -o x https://example.com/air", Status: StatusSkipped, + Reason: ReasonMissingDependency}, + Failing: lineResult{Cmd: "go tool air -v", Status: StatusFail, + output: `go: no such tool "air"`}, + Want: StatusFail, }} for testNum, test := range tests { @@ -181,3 +212,329 @@ func TestSharedOperandAfterLineNotPassed(t *testing.T) { }) } } + +// TestMixedPackagesInCheckout checks the rule for a document's Go file written +// beside the repository's own package. Go refusing to build two packages in +// the checkout's root is the session's doing only when a file block of the +// document wrote one of the two files and the checkout held the other before +// the session started. +// +//nolint:funlen // Test function. +func TestMixedPackagesInCheckout(t *testing.T) { + t.Parallel() + const ( + write = "cat > 'tutorial.go' <<'KIBBLE_FILE_END'" + mixed = "found packages sqlc (gomod_test.go) and main (tutorial.go) in /work/repo" + build = "go build ." + nested = "mkdir -p 'cmd/app' && cat > 'cmd/app/tutorial.go' <<'KIBBLE_FILE_END'" + ) + wrote := lineResult{Cmd: write, Status: StatusVerified, cwd: sessionRepoDir} + failed := lineResult{Cmd: build, Status: StatusFail, Detail: "exited 1: " + mixed, output: mixed} + held := []string{"doc.go", "gomod_test.go"} + one := func(lines ...lineResult) []exampleStep { + return []exampleStep{{ID: "b1", Doc: "README.md", Lines: lines}} + } + tests := []struct { + Steps []exampleStep + Checkout []string + Target string + WantStatus Status + WantReason Reason + WantDetail string + }{{ // Test 0: The document's file landed beside the checkout's own package. + Steps: one(wrote, failed), Checkout: held, + WantStatus: StatusBlocked, WantReason: ReasonCheckoutState, + WantDetail: "wrote tutorial.go into the repository's own checkout, beside gomod_test.go of " + + "another Go package, so the claim was not tested as written: exited 1: " + mixed, + }, { // Test 1: Neither file is the document's, so the failure stands. + Steps: one(lineResult{Cmd: "cat > 'notes.txt' <<'KIBBLE_FILE_END'", Status: StatusVerified, + cwd: sessionRepoDir}, failed), + Checkout: held, + WantStatus: StatusFail, + }, { // Test 2: The document wrote both files, so the clash is its own. + Steps: one(lineResult{Cmd: "cat > 'gomod_test.go' <<'KIBBLE_FILE_END'", Status: StatusVerified, + cwd: sessionRepoDir}, wrote, failed), + Checkout: held, + WantStatus: StatusFail, + }, { // Test 3: A directory outside the checkout holds no package of the repository. + Steps: one(wrote, lineResult{Cmd: build, Status: StatusFail, + output: "found packages sqlc (gomod_test.go) and main (tutorial.go) in /tmp/app"}), + Checkout: held, + WantStatus: StatusFail, + }, { // Test 4: A directory the session made inside the checkout is not the checkout's package. + Steps: one(wrote, lineResult{Cmd: build, Status: StatusFail, + output: "found packages db (db.go) and main (tutorial.go) in /work/repo/app"}), + Checkout: []string{"db.go"}, + WantStatus: StatusFail, + }, { // Test 5: A write that was skipped put nothing in the checkout. + Steps: one(lineResult{Cmd: write, Status: StatusSkipped, cwd: sessionRepoDir}, failed), + Checkout: held, + WantStatus: StatusFail, + }, { // Test 6: A write after the failure did not cause it. + Steps: one(failed, wrote), Checkout: held, + WantStatus: StatusFail, + }, { // Test 7: A file written into a subdirectory is not the one Go names at the root. + Steps: one(lineResult{Cmd: nested, Status: StatusVerified, cwd: sessionRepoDir}, failed), + Checkout: held, + WantStatus: StatusFail, + }, { // Test 8: Any other Go build error after the write stays a failure. + Steps: one(wrote, lineResult{Cmd: build, Status: StatusFail, + output: "./tutorial.go:9:2: undefined: tutorial"}), + Checkout: held, + WantStatus: StatusFail, + }, { // Test 9: A second file the checkout never held is the document's doing. + Steps: one(wrote, lineResult{Cmd: "sqlc generate", Status: StatusVerified, cwd: sessionRepoDir}, + lineResult{Cmd: build, Status: StatusFail, + output: "found packages main (tutorial.go) and tutorial (models.go) in /work/repo"}), + Checkout: held, + WantStatus: StatusFail, + }, { // Test 10: A file a shell line wrote is not a file block's, and not the checkout's. + Steps: one(wrote, lineResult{Cmd: "cat > models.go < 'main.go' <<'KIBBLE_FILE_END'", Status: StatusVerified, + cwd: sessionRepoDir}, lineResult{Cmd: build, Status: StatusFail, Detail: "exited 1", + output: "found packages main (main.go) and zlib (zlib.go) in /work/repo"}), + Checkout: []string{"zlib.go"}, + WantStatus: StatusBlocked, WantReason: ReasonCheckoutState, + WantDetail: "wrote main.go into the repository's own checkout, beside zlib.go of " + + "another Go package, so the claim was not tested as written: exited 1", + }, { // Test 14: An earlier document's file is gone once the checkout is restored. + Steps: []exampleStep{ + {ID: "b1", Doc: "docs/a.md", Lines: []lineResult{wrote}}, + {ID: "b2", Doc: "docs/b.md", Lines: []lineResult{failed}}, + }, + Checkout: held, + WantStatus: StatusFail, + }, { // Test 15: A file written after entering a directory is not at the root. + Steps: one(lineResult{Cmd: write, Status: StatusVerified, cwd: "/work/repo/app"}, failed), + Checkout: held, + WantStatus: StatusFail, + }, { // Test 16: A write whose directory the session did not report proves nothing. + Steps: one(lineResult{Cmd: write, Status: StatusVerified}, failed), + Checkout: held, + WantStatus: StatusFail, + }, { // Test 17: A write that failed put nothing reliable in the checkout. + Steps: one(lineResult{Cmd: write, Status: StatusFail, cwd: sessionRepoDir}, failed), + Checkout: held, + WantStatus: StatusFail, + }, { // Test 18: A write that was blocked did not run as written either. + Steps: one(lineResult{Cmd: write, Status: StatusBlocked, cwd: sessionRepoDir}, failed), + Checkout: held, + WantStatus: StatusFail, + }, { // Test 19: A later line failing for its own reason is not excused by the blocked build. + Steps: one(wrote, failed, lineResult{Cmd: "docker build .", Status: StatusFail, + output: "ERROR: failed to solve: dockerfile parse error on line 3"}), + Checkout: held, + Target: "docker build .", + WantStatus: StatusFail, + }, { // Test 20: A numbered terminator marks a file block all the same. + Steps: one(lineResult{Cmd: "cat > 'tutorial.go' <<'KIBBLE_FILE_END_1'", Status: StatusVerified, + cwd: sessionRepoDir}, failed), + Checkout: held, + WantStatus: StatusBlocked, WantReason: ReasonCheckoutState, + }, { // Test 21: Go prefixes the importing position when the root is reached by an import. + Steps: one(wrote, lineResult{Cmd: build, Status: StatusFail, + output: "cmd/tool/main.go:3:8: " + mixed}), + Checkout: held, + WantStatus: StatusBlocked, WantReason: ReasonCheckoutState, + }} + for testNum, test := range tests { + t.Run(fmt.Sprintf("test %d", testNum), func(t *testing.T) { + t.Parallel() + steps := make([]exampleStep, len(test.Steps)) + for i, s := range test.Steps { + steps[i] = exampleStep{ID: s.ID, Doc: s.Doc, Lines: slices.Clone(s.Lines)} + } + run := &exampleRun{Steps: steps} + resolveDependentFailures(run, &kplan.Plan{RootGoFiles: test.Checkout}) + target := test.Target + if target == "" { + target = build + } + var got lineResult + for _, s := range run.Steps { + for _, l := range s.Lines { + if l.Cmd == target { + got = l + } + } + } + if got.Status != test.WantStatus || got.Reason != test.WantReason { + t.Errorf("got %s %q, want %s %q (detail %q)", + got.Status, got.Reason, test.WantStatus, test.WantReason, got.Detail) + } + if test.WantDetail != "" && got.Detail != test.WantDetail { + t.Errorf("detail = %q, want %q", got.Detail, test.WantDetail) + } + }) + } +} + +// TestNotItsRepository checks the rule for a tool saying the checkout is not a +// repository of its own kind and naming the command that makes one. The first +// outputs are what jj printed in a sweep, plain, colored, and with its debug +// color labels. The line was not tested as written only when it ran at the top +// of the checkout and no earlier line of its document that passed made or +// cloned a repository with the same tool. +// +//nolint:funlen // Test function. +func TestNotItsRepository(t *testing.T) { + t.Parallel() + const ( + hint = "It looks like this is a git repo. You can create a jj repo backed by it by running this:" + plain = "Error: There is no jj repo in \".\"\nHint: " + hint + "\njj git init\n" + color = "\x1b[1m\x1b[38;5;1mError: \x1b[39mThere is no jj repo in \".\"\x1b[0m\n" + + "\x1b[1m\x1b[38;5;6mHint: \x1b[0m\x1b[39m" + hint + "\x1b[39m\n\x1b[39mjj git init\x1b[39m\n" + labeled = "\x1b[1m\x1b[38;5;1m<>\x1b[39m" + + "<>\x1b[0m\n" + + "\x1b[1m\x1b[38;5;6m<>\x1b[0m\x1b[39m<>\x1b[39m\n" + + "\x1b[39m<>\x1b[39m\n" + advice = "[warn] No hook installed — run `rtk init -g` for automatic token savings\n" + + "fatal: not a git repository (or any of the parent directories): .git\n" + logCmd = "jj log" + blocked = "ran at the top of the checkout, which jj reports is not a jj repository, and no " + + "line of this document that ran created one, so the claim was not tested as written: " + ) + fail := func(cmd, output, cwd string) lineResult { + const detail = "exited 1: jj git init" + return lineResult{Cmd: cmd, Status: StatusFail, Detail: detail, output: output, cwd: cwd} + } + ran := func(cmd string, status Status) lineResult { + return lineResult{Cmd: cmd, Status: status, cwd: sessionRepoDir} + } + failed := fail(logCmd, plain, sessionRepoDir) + made := ran("jj git init somedir", StatusVerified) + one := func(lines ...lineResult) []exampleStep { + return []exampleStep{{ID: "b1", Doc: "docs/tutorial.md", Lines: lines}} + } + tests := []struct { + Steps []exampleStep + Binaries []string + Target string + WantStatus Status + WantReason Reason + WantDetail string + }{{ // Test 0: The tool says the checkout top is not its repository and names its init. + Steps: one(failed), + WantStatus: StatusBlocked, WantReason: ReasonCheckoutState, + WantDetail: blocked + "exited 1: jj git init", + }, { // Test 1: The colored form reads the same once the codes are stripped. + Steps: one(fail(logCmd, color, sessionRepoDir)), + WantStatus: StatusBlocked, WantReason: ReasonCheckoutState, + }, { // Test 2: The form with debug color labels reads the same. + Steps: one(fail(logCmd, labeled, sessionRepoDir)), + WantStatus: StatusBlocked, WantReason: ReasonCheckoutState, + }, { // Test 3: A line that ran in another directory is the document's own doing. + Steps: one(fail(logCmd, plain, "/work/repo/docs")), + WantStatus: StatusFail, + }, { // Test 4: A document that made its repository and then failed keeps its failure. + Steps: one(made, failed), + WantStatus: StatusFail, + }, { // Test 5: A document that cloned its repository keeps its failure too. + Steps: one(ran("jj git clone https://example.com/hello.git", StatusVerified), failed), + WantStatus: StatusFail, + }, { // Test 6: An init that passed in another document made nothing for this one. + Steps: []exampleStep{ + {ID: "b1", Doc: "docs/install.md", Lines: []lineResult{made}}, + {ID: "b2", Doc: "docs/tutorial.md", Lines: []lineResult{failed}}, + }, + WantStatus: StatusBlocked, WantReason: ReasonCheckoutState, + }, { // Test 7: A message naming a different tool is not this tool's report. + Steps: one(fail(logCmd, "abort: there is no hg repository in \".\"\nTry jj git init here.\n", + sessionRepoDir)), + WantStatus: StatusFail, + }, { // Test 8: A report that names no init command stays a failure. + Steps: one(fail(logCmd, "Error: There is no jj repo in \".\"\n", sessionRepoDir)), + WantStatus: StatusFail, + }, { // Test 9: A usage error is the document's own wording, whatever else was printed. + Steps: one(fail("jj lg --oneline", "error: unexpected argument '--oneline' found\n"+plain, + sessionRepoDir)), + Target: "jj lg --oneline", + WantStatus: StatusFail, + }, { // Test 10: Advice to run an init command is no report of a missing repository. + Steps: one(fail("rtk git status", advice, sessionRepoDir)), + Target: "rtk git status", + WantStatus: StatusBlocked, WantReason: ReasonDependsOnSkipped, + WantDetail: "says to run `rtk init` first, which did not run", + }, { // Test 11: An init line that did not pass made no repository, so the rule holds. + Steps: one(ran("jj git init", StatusSkipped), failed), + WantStatus: StatusBlocked, WantReason: ReasonCheckoutState, + }, { // Test 12: A passing line of the tool with no init or clone made no repository. + Steps: one(ran("jj --version", StatusVerified), failed), + WantStatus: StatusBlocked, WantReason: ReasonCheckoutState, + }, { // Test 13: A tool the document does not install is not read for this. + Steps: one(failed), + Binaries: []string{"rtk"}, + WantStatus: StatusFail, + }, { // Test 14: A report about a directory the line named is not about the checkout. + Steps: one(ran("git init demo", StatusVerified), fail("jj -R demo log", + "Error: There is no jj repo in \"demo\"\nHint: "+hint+"\njj git init\n", sessionRepoDir)), + Target: "jj -R demo log", + WantStatus: StatusFail, + }, { // Test 15: A line that stepped into a directory and back was not run at the top. + Steps: one(fail("cd demo; jj st; cd ..", plain, sessionRepoDir)), + Target: "cd demo; jj st; cd ..", + WantStatus: StatusFail, + }, { // Test 16: A crash after the report convicts the line whatever else it printed. + Steps: one(fail(logCmd, plain+"thread 'main' panicked at main.rs:9:5:\n", sessionRepoDir)), + WantStatus: StatusFail, + }, { // Test 17: An init that passed after another documented tool on its line still counts. + Steps: one(ran("rtk sync && jj git init demo", StatusVerified), failed), + WantStatus: StatusFail, + }, { // Test 18: A report naming the checkout by its full path is about the checkout. + Steps: one(fail(logCmd, "Error: There is no jj repo in \"/work/repo\"\nHint: "+hint+ + "\njj git init\n", sessionRepoDir)), + WantStatus: StatusBlocked, WantReason: ReasonCheckoutState, + }, { // Test 19: A repository another tool made is not one of this tool's kind. + Steps: one(ran("git init demo", StatusVerified), failed), + WantStatus: StatusBlocked, WantReason: ReasonCheckoutState, + }} + for testNum, test := range tests { + t.Run(fmt.Sprintf("test %d", testNum), func(t *testing.T) { + t.Parallel() + steps := make([]exampleStep, len(test.Steps)) + for i, s := range test.Steps { + steps[i] = exampleStep{ID: s.ID, Doc: s.Doc, Lines: slices.Clone(s.Lines)} + } + bins := test.Binaries + if bins == nil { + bins = []string{"jj", "rtk"} + } + run := &exampleRun{Steps: steps} + resolveDependentFailures(run, &kplan.Plan{Binaries: bins}) + target := test.Target + if target == "" { + target = logCmd + } + var got lineResult + for _, s := range run.Steps { + for _, l := range s.Lines { + if l.Cmd == target { + got = l + } + } + } + if got.Status != test.WantStatus || got.Reason != test.WantReason { + t.Errorf("got %s %q, want %s %q (detail %q)", + got.Status, got.Reason, test.WantStatus, test.WantReason, got.Detail) + } + if test.WantDetail != "" && got.Detail != test.WantDetail { + t.Errorf("detail = %q, want %q", got.Detail, test.WantDetail) + } + }) + } +} diff --git a/doccoverage.go b/doccoverage.go index 258b004..e91c334 100644 --- a/doccoverage.go +++ b/doccoverage.go @@ -111,7 +111,7 @@ func readDocSet(dir, readme string) docSet { // it is a false drift, so those documents are left out of the set the // flag check and doc coverage read. base := strings.ToLower(strings.TrimSuffix(name, filepath.Ext(name))) - if skipNames[base] || hasSkipPrefix(base) || isHistoryDoc(base) { + if skippedDoc(base) || isHistoryDoc(base) { return nil } body, err := os.ReadFile(path) @@ -171,7 +171,7 @@ func docCoverageChecks(results []Result) []Result { if len(cmds) == 0 { check.Status = StatusSkipped check.Detail = "no command list in the help output to check against" - out = append(out, check) + out = appendCheck(out, check) continue } docs := readDocSet(r.Step.dir, r.Step.readme) @@ -199,7 +199,7 @@ func docCoverageChecks(results []Result) []Result { len(outsideReadme), strings.Join(outsideReadme, " ")) } } - out = append(out, check) + out = appendCheck(out, check) } return out } diff --git a/doccoverage_test.go b/doccoverage_test.go index 5e65000..ed2f63f 100644 --- a/doccoverage_test.go +++ b/doccoverage_test.go @@ -189,3 +189,77 @@ func TestDocCoverageChecks(t *testing.T) { }) } } + +// TestChecksCollapseEqualRows checks that two install routes of one binary +// with the same outcome yield one flag-check and one doc-coverage row, while +// routes whose outcomes differ keep a row each. +func TestChecksCollapseEqualRows(t *testing.T) { + t.Parallel() + ok := map[string]string{"json": "ok", "yaml": "ok"} + tests := []struct { + SecondProbes map[string]string + SecondBinary string + WantFlag int + WantCoverage int + }{{ // Test 0: a global and a local install of one binary answer alike. + SecondProbes: ok, SecondBinary: "tool", WantFlag: 1, WantCoverage: 1, + }, { // Test 1: a second route that rejects a flag drifts, so both rows stay. + SecondProbes: map[string]string{"json": "unknown flag: --json", "yaml": "ok"}, + SecondBinary: "tool", WantFlag: 2, WantCoverage: 1, + }, { // Test 2: two binaries with equal outcomes are separate findings. + SecondProbes: ok, SecondBinary: "other", WantFlag: 2, WantCoverage: 2, + }} + for testNum, test := range tests { + t.Run(fmt.Sprintf("test %d", testNum), func(t *testing.T) { + t.Parallel() + dir := t.TempDir() + if err := os.WriteFile(filepath.Join(dir, "README.md"), + []byte("Run `tool attest --json --yaml` or `other attest`."), 0o600); err != nil { + t.Fatalf("write README: %v", err) + } + in := []Result{ + collapseRoute(dir, "tool", ok), + collapseRoute(dir, test.SecondBinary, test.SecondProbes), + } + if got := len(flagChecks(in)); got != test.WantFlag { + t.Errorf("flag-check rows = %d, want %d", got, test.WantFlag) + } + if got := len(docCoverageChecks(in)); got != test.WantCoverage { + t.Errorf("doc-coverage rows = %d, want %d", got, test.WantCoverage) + } + }) + } +} + +// TestChecksKeepEqualStatusApart checks that two routes of one binary that +// both drift on different flags keep a row each, since the detail differs. +func TestChecksKeepEqualStatusApart(t *testing.T) { + t.Parallel() + dir := t.TempDir() + in := []Result{ + collapseRoute(dir, "tool", map[string]string{"json": "unknown flag: --json", "yaml": "ok"}), + collapseRoute(dir, "tool", map[string]string{"json": "ok", "yaml": "unknown flag: --yaml"}), + } + got := flagChecks(in) + if len(got) != 2 { + t.Fatalf("got %d flag-check rows, want 2", len(got)) + } + if got[0].Status != StatusDrift || got[1].Status != StatusDrift { + t.Errorf("statuses = %s, %s, want both %s", got[0].Status, got[1].Status, StatusDrift) + } +} + +// collapseRoute builds a passing npm install result for binary in dir whose +// cited flags json and yaml were probed with the given answers. +func collapseRoute(dir, binary string, probes map[string]string) Result { + return Result{ + Status: StatusVerified, + helpText: "Usage: tool [options]\nCommands:\n attest Seal it\n", + helpByFlag: probes, + Step: InstallStep{Repo: "repo", Kind: "npm-install", Binary: binary, + dir: dir, readme: "README.md", Usage: &Usage{ + Flags: []string{"json", "yaml"}, + FlagSub: map[string]string{"json": "", "yaml": ""}, + }}, + } +} diff --git a/docs/DESIGN.md b/docs/DESIGN.md index a2fcc4f..e6367fa 100644 --- a/docs/DESIGN.md +++ b/docs/DESIGN.md @@ -16,6 +16,16 @@ reports which steps a brand-new user could actually complete. - Runs documented package installs the same way, verbatim as written: `cargo install`, `npm install -g`, `yarn global add`, `pipx install`, and `uv tool install`. +- Runs a local `npm install ` in a scratch project when a `package.json` in the + checkout declares that name. The binary comes from the installed package's own `bin` + field, so a dependency's tool is never tested in the package's name. A package with no + `bin` is checked as an install only. The rule is narrow on purpose, because the line + runs for real: the name must be exactly a package name a manifest in the checkout + declares, a package marked `private` does not count, every argument must be a registry + name or a plain save or logging flag, and a line naming a registry, a workspace, or a + prefix is left alone. A local and a global install of the same package are checked as + two routes and open one example session. Binaries a package provides through + `directories.bin` are not read. - Runs each `git clone` recipe too: the clone and the build lines that follow it in the same code block, with GitHub SSH remotes rewritten to HTTPS for the keyless container. - Picks the image from the toolchain the step assumes, so a Rust project builds with @@ -223,7 +233,20 @@ procedure, which is the same boundary `inherited-state` reads. The same holds for the checkout itself. A would-be plain `FAIL` that ran in the repository's own checkout, and whose output says the target "already exists" or is "already initialized", is `BLOCKED` with the reason `checkout-state`. The collision is -with the checkout kibble chose, not with anything the document wrote. A project manager, +with the checkout kibble chose, not with anything the document wrote. A Go build that +reports two packages at the top of the checkout is `checkout-state` as well, when a file +block of the document wrote one of the two files Go names and the checkout held the other +before the session started. A tutorial that tells the reader in prose to work in a new +directory has its file written beside the repository's own package instead. A second file +the checkout never held, such as a generator's output, leaves the failure standing, since +that clash is the document's own. A documented tool that reports there is no repository +of its own kind at the top of the checkout, and names its own init command, is +`checkout-state` too. A version control tool's guide is written for a reader standing in +such a repository, and the session stands in a plain clone of the project. The line was +not tested as written, which is not a pass. A line that ran in any other directory or +stepped into one, a report about a directory the line named, a usage error, a crash, and +a document whose earlier passing line made or cloned a repository with the tool all keep +the failure. A project manager, a documented binary with a known ecosystem such as uv, that fails in the repository's own checkout is `checkout-state` as well, because it read the checkout's own project. A project that a scaffold line reported adding to the checkout's workspace, in the tool's own words, is @@ -301,8 +324,10 @@ matches. A tool that names what was missing is evidence, and the verdict follows what it names. Output saying "File not found: X" names the file, and settles like "No such file or -directory". A line whose operand starts with `../` is skipped at plan time as -`missing-fixture`, since that path lies outside the checkout and only the reader has it. +directory". So does a bundler saying "Cannot resolve entry module index.html" or "Could +not resolve entry module", where the name must carry an extension. A line whose operand +starts with `../` is skipped at plan time as `missing-fixture`, since that path lies +outside the checkout and only the reader has it. A missing file or directory the document itself mentioned is a `GAP`, the same as a file the planner sees no step creating. A missing path the document never mentions is `BLOCKED`, because the tool may have a bug or the document may have skipped a step, and diff --git a/docselect.go b/docselect.go index 8dd9bd4..3ab90cf 100644 --- a/docselect.go +++ b/docselect.go @@ -183,7 +183,7 @@ func replayDocs(dir, readme string, cfg *config.ExamplesConfig) []string { return nil } base := strings.ToLower(strings.TrimSuffix(d.Name(), filepath.Ext(d.Name()))) - if skipNames[base] || hasSkipPrefix(base) { + if skippedDoc(base) { return nil } top := strings.Split(filepath.ToSlash(rel), "/")[0] @@ -268,7 +268,7 @@ func installDocs(dir, readme string) []string { } base := strings.ToLower(strings.TrimSuffix(d.Name(), filepath.Ext(d.Name()))) install := isInstallDoc(base) || isInstallIndex(base, filepath.Base(filepath.Dir(rel))) - if skipNames[base] || hasSkipPrefix(base) || !install { + if skippedDoc(base) || !install { return nil } found = append(found, rel) @@ -331,6 +331,29 @@ func hasShellBlock(path string) bool { return false } +// legacyWords mark a document kept for an older or retired form of the tool. +// asdf's getting-started-legacy.md documents the shell version it replaced, +// and its commands were reported as drift against the current binary. +var legacyWords = []string{"legacy", "deprecated", "archived"} + +// isLegacyDoc reports whether a document base name marks it as kept for an +// older or retired form of the tool. +func isLegacyDoc(base string) bool { + for _, w := range legacyWords { + if strings.Contains(base, w) { + return true + } + } + return false +} + +// skippedDoc reports whether a document base name marks it as one no reader +// is asked to follow: a project record, a document for an agent or a +// maintainer, or a guide kept for a retired version. +func skippedDoc(base string) bool { + return skipNames[base] || hasSkipPrefix(base) || isLegacyDoc(base) +} + // hasSkipPrefix reports whether a document's name marks it as written for an // agent or a maintainer. func hasSkipPrefix(base string) bool { diff --git a/docselect_test.go b/docselect_test.go index 248f588..5d358d6 100644 --- a/docselect_test.go +++ b/docselect_test.go @@ -76,6 +76,10 @@ func TestReplayDocs(t *testing.T) { Files: map[string]string{"README.md": shell, "docs/2023/a.md": shell, "docs/2024/a.md": shell}, Want: []string{"README.md", "docs/2023/a.md", "docs/2024/a.md"}, + }, { // Test 13: a guide kept for a retired version is not replayed. + Files: map[string]string{"README.md": shell, "docs/guide/getting-started.md": shell, + "docs/guide/getting-started-legacy.md": shell, "docs/Deprecated-API.md": shell}, + Want: []string{"README.md", "docs/guide/getting-started.md"}, }} for testNum, test := range tests { t.Run(fmt.Sprintf("test %d", testNum), func(t *testing.T) { diff --git a/examplerun.go b/examplerun.go index 1b1d596..1d7da6b 100644 --- a/examplerun.go +++ b/examplerun.go @@ -61,6 +61,8 @@ type lineResult struct { Synthetic []string // output is what the line printed, kept for dependency analysis. output string + // cwd is the directory the line ran in, empty when the session did not say. + cwd string } // markerLead is printed before every session marker. A documented command @@ -96,12 +98,18 @@ func (d *DockerRunner) runExample(ctx context.Context, step InstallStep) Result res.Detail = detail return res } - image, ok := d.exampleImage(plan) + image, ok, blocked := d.exampleImage(plan, step.dir) if !ok { res.Status = StatusSkipped res.Detail = "documented installs need more than one toolchain; no single image serves them" return res } + if blocked != "" { + res.Status = StatusBlocked + res.Reason = ReasonMissingDependency + res.Detail = blocked + return res + } script, wrapped := sessionScript(plan, int(d.Timeout.Seconds()), int(d.lineBudget().Seconds())) ctx, cancel := context.WithTimeout(ctx, sessionBudget(plan, d.Timeout)) @@ -149,22 +157,25 @@ func (d *DockerRunner) runExample(ctx context.Context, step InstallStep) Result // exampleImage returns the image an example session runs in. Every documented // install must be servable by one image, since the session is one shell: a // project installed with both cargo and npm names two toolchains and is -// reported as a skip rather than run in an image missing one of them. -func (d *DockerRunner) exampleImage(plan *kplan.Plan) (string, bool) { +// reported as a skip rather than run in an image missing one of them. A +// Python session runs on the newest release the checkout at dir states it +// supports, and blocked is set when that range allows no image. +func (d *DockerRunner) exampleImage(plan *kplan.Plan, dir string) (image string, ok bool, blocked string) { eco := "" for _, in := range plan.Installs { if in.Ecosystem == "" { continue } if eco != "" && eco != in.Ecosystem { - return "", false + return "", false, "" } eco = in.Ecosystem } if tc, known := toolchains[eco]; known && tc.Image != "" { - return tc.Image, true + image, blocked = toolchainImage(tc, dir) + return image, true, blocked } - return d.Image, true + return d.Image, true, "" } // sessionBudget bounds the whole example session: each module build gets the diff --git a/exitcodes_test.go b/exitcodes_test.go new file mode 100644 index 0000000..310d451 --- /dev/null +++ b/exitcodes_test.go @@ -0,0 +1,58 @@ +package main + +import ( + "fmt" + "testing" + + "github.com/google/go-cmp/cmp" + + kplan "github.com/dcadolph/kibble/internal/plan" +) + +// TestDocumentedNotFoundCode checks that a documented binary exiting with a +// code its documents call not found reads as no data, and that any other +// code, or a code the documents never named, still fails. +func TestDocumentedNotFoundCode(t *testing.T) { + t.Parallel() + tests := []struct { + Codes []int + Cmd string + Output string + Code int + WantStatus Status + }{{ // Test 0: witr's exit 2 with nothing printed is its documented not found. + Codes: []int{2}, Cmd: "witr --port 5000 --json", Code: 2, + WantStatus: StatusSkipped, + }, { // Test 1: witr's exit 2 naming a missing process is the same lookup. + Codes: []int{2}, Cmd: "witr --pid 8123 --warnings", Output: "Error: process 8123 does not exist", + Code: 2, WantStatus: StatusSkipped, + }, { // Test 2: another documented code still fails. + Codes: []int{2}, Cmd: "witr --pid 1", Output: "internal error", Code: 5, + WantStatus: StatusFail, + }, { // Test 3: with no table the same exit fails. + Cmd: "witr --port 5000 --json", Output: "}", Code: 2, + WantStatus: StatusFail, + }, { // Test 4: a line that is not the documented binary is not excused. + Codes: []int{2}, Cmd: "ls /nope", Output: "ls: cannot access '/nope'", Code: 2, + WantStatus: StatusFail, + }} + for testNum, test := range tests { + t.Run(fmt.Sprintf("test %d", testNum), func(t *testing.T) { + t.Parallel() + plan := &kplan.Plan{Repo: "witr", Binaries: []string{"witr"}, NotFoundCodes: test.Codes, + Steps: []kplan.PlanStep{{ID: "b1", Doc: "README.md", Heading: "Usage", + Lines: []kplan.PlanLine{{Cmd: test.Cmd}}}}} + out := sessionOut("KIBBLE-BUILD CODE=0", "KIBBLE-STEP b1 START", test.Output, + fmt.Sprintf("KIBBLE-LINE b1:0 CODE=%d CWD=/work/repo", test.Code), "KIBBLE-DONE") + res := classifyExample(InstallStep{Repo: "witr", Kind: "example"}, plan, out, nil, 0, + lineTimeout, sessionEnd{}) + if res.example == nil { + t.Fatalf("no per-line outcomes, status %s: %s", res.Status, res.Detail) + } + got := res.example.Steps[0].Lines[0] + if diff := cmp.Diff(test.WantStatus, got.Status); diff != "" { + t.Errorf("status mismatch (-want +got):\n%s\ndetail: %s", diff, got.Detail) + } + }) + } +} diff --git a/extract.go b/extract.go index 5151e00..8dd4b74 100644 --- a/extract.go +++ b/extract.go @@ -68,8 +68,9 @@ func (f ExtractorFunc) Extract(repo, markdown string) []InstallStep { var ( // reGoInstall matches a `go install @` invocation, allowing - // leading flags such as `-v` before the module path. - reGoInstall = regexp.MustCompile(`\bgo\s+install\s+(?:-\S+\s+)*(\S+@\S+)`) + // leading flags such as `-v` or `-tags 'postgres'` before the module path. + reGoInstall = regexp.MustCompile( + `\bgo\s+install\s+(?:-\S+(?:\s+(?:'[^']*'|"[^"]*"|[^-\s@][^\s@]*))?\s+)*(\S+@\S+)`) // reBrew matches a `brew install ` invocation. // reBrew captures the flags and the first target of a brew install. Flags // come first so `brew install --cask alacritty` names alacritty, not @@ -157,11 +158,14 @@ func cloneTarget(line string) string { return plain } -// DefaultExtractor returns an Extractor that reads code from fenced blocks, +// ExtractorWith returns an Extractor that reads code from fenced blocks, // indented blocks, and inline spans, and classifies the install commands // inside them. Only text the author marked as code is considered, so plain -// prose does not count as a runnable step. -func DefaultExtractor() Extractor { +// prose does not count as a runnable step. The repository's own names are +// what make a local npm install a route to the tool: `npm install chalk` is +// chalk's documented install in chalk's README and a dependency install in +// every other one. +func ExtractorWith(owned repoNames) Extractor { return ExtractorFunc(func(repo, markdown string) []InstallStep { seen := map[string]bool{} var steps []InstallStep @@ -175,6 +179,9 @@ func DefaultExtractor() Extractor { taps = append(taps, m[1]) } s, key, ok := classifyLine(repo, line) + if !ok { + s, key, ok = localNpmStep(repo, line, owned) + } if !ok || seen[key] { continue } @@ -409,9 +416,110 @@ func classifyPackage(repo, line string) (string, string, bool) { // rather than a project dependency install. var reGlobalFlag = regexp.MustCompile(`(^|\s)(-g|--global)(\s|$)`) -// hasGlobalFlag reports whether a line installs globally. +// hasGlobalFlag reports whether a line installs globally. A trailing comment +// is not part of the command: zx's `npm install zx # add -g to install +// globally` is a local install that mentions the flag. func hasGlobalFlag(line string) bool { - return reGlobalFlag.MatchString(line) + return reGlobalFlag.MatchString(stripComment(line)) +} + +// reNpmPackage matches a plain registry package argument, scoped or not, with +// an optional version or tag. A URL, a path, a GitHub shorthand, and an alias +// of another package all fail it. +var reNpmPackage = regexp.MustCompile(`^((?:@[\w~][\w.~-]*/)?[\w~][\w.~-]*)(?:@[\w.^~*-]+)?$`) + +// npmLocalFlags are the npm install flags a local route may carry. Each takes +// no value and leaves the install in the current project, so every other +// argument on the line is a package. +var npmLocalFlags = map[string]bool{ + "-S": true, "--save": true, "-P": true, "--save-prod": true, "-D": true, "--save-dev": true, + "-O": true, "--save-optional": true, "--save-peer": true, "-E": true, "--save-exact": true, + "--no-save": true, "-f": true, "--force": true, "--legacy-peer-deps": true, + "--no-audit": true, "--no-fund": true, "--no-package-lock": true, "--ignore-scripts": true, + "-s": true, "--silent": true, "-q": true, "--quiet": true, "-d": true, "--verbose": true, +} + +// localNpmPackage returns the first package a local npm install names and +// that package's bare name, or empty strings when the line is not a local +// install of plain registry packages. Every argument must be a registry name +// or a flag from npmLocalFlags. A registry setting is refused, since the +// package it fetches need not be the one the public name means. So is a +// workspace or prefix flag, which installs somewhere a new project lacks, and +// a path or URL riding along, which a new project cannot resolve. +func localNpmPackage(line string) (string, string) { + cmd := shellCommand(line) + if !localNpmInstall(cmd) { + return "", "" + } + var pkg, name string + for _, f := range strings.Fields(cmd)[2:] { + if strings.HasPrefix(f, "-") { + if !npmLocalFlags[f] { + return "", "" + } + continue + } + f = unquote(f) + m := reNpmPackage.FindStringSubmatch(f) + if m == nil { + return "", "" + } + if pkg == "" { + pkg, name = f, m[1] + } + } + return pkg, name +} + +// unquote removes one matching pair of quotes around a shell word. A quote +// with no partner stays, so a broken line does not read as a clean name. +func unquote(s string) string { + if len(s) >= 2 && (s[0] == '"' || s[0] == '\'') && s[len(s)-1] == s[0] { + return s[1 : len(s)-1] + } + return s +} + +// npmPackageName returns a node package request without its version or tag, +// or the request unchanged when it is not a plain registry name. +func npmPackageName(module string) string { + if m := reNpmPackage.FindStringSubmatch(module); m != nil { + return m[1] + } + return module +} + +// localNpmStep returns the install step for a local npm install of a package +// the repository itself declares. The name must equal a declared package name +// exactly: the checkout's folder name and a near spelling are not enough, +// because the line runs for real and the registry serves whatever holds the +// name. The expected binary comes from the manifest and is empty for a +// package that declares none. The key differs from a global install's, so a +// document that shows both has both verified. +func localNpmStep(repo, line string, owned repoNames) (InstallStep, string, bool) { + pkg, name := localNpmPackage(line) + if pkg == "" { + return InstallStep{}, "", false + } + bin, ok := owned.npmBinary(name) + if !ok { + return InstallStep{}, "", false + } + return InstallStep{ + Repo: repo, Kind: "npm-install", Raw: strings.TrimSpace(line), + Module: pkg, Binary: bin, Run: !docblock.HasPlaceholder(pkg), + }, repo + "|npm-local|" + pkg, true +} + +// installKey names an install step for the purpose of adding it once across +// documents. A local npm install and a global one of the same package are two +// routes and keep separate keys. +func installKey(s InstallStep) string { + key := s.Kind + "|" + s.Module + if localNpmRoute(s) { + key += "|local" + } + return key } // valueFlags are flags that consume the argument after them, so the argument diff --git a/extract_test.go b/extract_test.go index f761dca..ec80289 100644 --- a/extract_test.go +++ b/extract_test.go @@ -71,6 +71,16 @@ func TestClassifyLine(t *testing.T) { }, { // Test 16: Python extras are not a placeholder, so the install runs. In: "pip install 'httpie[socks]'", WantKind: "pip-install", WantMod: "httpie", WantRun: true, WantOK: true, + }, { // Test 17: A quoted flag value before the module is skipped. + In: "go install -tags 'postgres' github.com/golang-migrate/migrate/v4/cmd/migrate@latest", + WantKind: "go-install", WantMod: "github.com/golang-migrate/migrate/v4/cmd/migrate@latest", + WantRun: true, WantOK: true, + }, { // Test 18: A double-quoted multiword flag value is skipped. + In: `go install -ldflags "-s -w" example.com/mod@v1.2.3`, + WantKind: "go-install", WantMod: "example.com/mod@v1.2.3", WantRun: true, WantOK: true, + }, { // Test 19: A bare flag value is skipped. + In: "go install -tags postgres example.com/mod@v1.0.0", + WantKind: "go-install", WantMod: "example.com/mod@v1.0.0", WantRun: true, WantOK: true, }} for testNum, test := range tests { t.Run(fmt.Sprintf("test %d", testNum), func(t *testing.T) { @@ -116,7 +126,7 @@ func TestExtract(t *testing.T) { for testNum, test := range tests { t.Run(fmt.Sprintf("test %d", testNum), func(t *testing.T) { t.Parallel() - steps := DefaultExtractor().Extract("repo", test.In) + steps := ExtractorWith(repoNames{}).Extract("repo", test.In) if diff := cmp.Diff(test.WantCount, len(steps)); diff != "" { t.Fatalf("count mismatch (-want +got):\n%s\nsteps: %+v", diff, steps) } @@ -166,7 +176,7 @@ func TestInstallRecipeCapture(t *testing.T) { for testNum, test := range tests { t.Run(fmt.Sprintf("test %d", testNum), func(t *testing.T) { t.Parallel() - steps := DefaultExtractor().Extract("repo", test.In) + steps := ExtractorWith(repoNames{}).Extract("repo", test.In) var clone *InstallStep for i := range steps { if steps[i].Kind == "git-clone" { @@ -252,6 +262,8 @@ func TestClassifyPackage(t *testing.T) { WantKind: "npm-install", WantPkg: "prettier", }, { // Test 10: an unrelated line matches nothing. In: "go build ./...", WantOK: false, + }, { // Test 11: A global flag named only in a comment is not a global install. + In: "npm install zx # add -g to install globally", WantOK: false, }} for testNum, test := range tests { t.Run(fmt.Sprintf("test %d", testNum), func(t *testing.T) { @@ -270,6 +282,151 @@ func TestClassifyPackage(t *testing.T) { } } +// TestLocalNpmStep checks that a local npm install is an install route only +// for a plain registry name the repository's own manifest declares, and that +// the expected binary is the one the manifest names. +func TestLocalNpmStep(t *testing.T) { + t.Parallel() + owned := repoNames{ + names: map[string]bool{"checkout": true}, + npm: map[string][]string{ + "zx": {"zx"}, "chalk": nil, "ink": nil, "@scope/tool": {"other", "tool"}, + "multi": {"aaa", "bbb"}, "core": nil, + }, + } + tests := []struct { + In string + WantModule string + WantBinary string + WantOK bool + WantNoRun bool + }{{ // Test 0: A declared package with a binary is a route. + In: "npm install zx", WantOK: true, WantModule: "zx", WantBinary: "zx", + }, { // Test 1: A declared package with no binary is a route that expects none. + In: "npm install chalk", WantOK: true, WantModule: "chalk", + }, { // Test 2: A package no manifest declares is a dependency install. + In: "npm install prettier", + }, { // Test 3: The checkout's folder name alone is not a declaration. + In: "npm install checkout", + }, { // Test 4: A URL is not a registry name. + In: "npm install https://example.com/zx.tgz", + }, { // Test 5: A GitHub shorthand is not a registry name. + In: "npm install google/zx", + }, { // Test 6: A line naming a registry is refused. + In: "npm install --registry https://npm.example.com zx", + }, { // Test 7: The glued registry form is refused too. + In: "npm install zx --registry=https://npm.example.com", + }, { // Test 8: A global flag named only in a comment leaves the line local. + In: "npm install zx # add -g to install globally", WantOK: true, + WantModule: "zx", WantBinary: "zx", + }, { // Test 9: A global install is not a local route. + In: "npm install -g zx", + }, { // Test 10: The first package decides, and the rest ride along. + In: "npm install ink react", WantOK: true, WantModule: "ink", + }, { // Test 11: An undeclared first package is not rescued by a later one. + In: "npm install react ink", + }, { // Test 12: A version tag is kept on the module and ignored for the match. + In: "npm i zx@8.8.5", WantOK: true, WantModule: "zx@8.8.5", WantBinary: "zx", + }, { // Test 13: A scoped package prefers the binary named after it. + In: "$ npm add @scope/tool", WantOK: true, WantModule: "@scope/tool", WantBinary: "tool", + }, { // Test 14: An alias installs another package under the declared name. + In: "npm install zx@npm:other", + }, { // Test 15: A near spelling is not the declared name. + In: "npm install ZX", + }, { // Test 16: A compound line is not a single install. + In: "npm install zx && zx --version", + }, { // Test 17: Without a binary named after the package, the first by name is expected. + In: "npm install multi", WantOK: true, WantModule: "multi", WantBinary: "aaa", + }, { // Test 18: A path is the checkout, not the registry. + In: "npm install ./zx", + }, { // Test 19: A save flag before the name leaves the line a route. + In: "npm install --save-dev zx", WantOK: true, WantModule: "zx", WantBinary: "zx", + }, { // Test 20: A save flag after the name does too. + In: "npm i zx -D", WantOK: true, WantModule: "zx", WantBinary: "zx", + }, { // Test 21: A workspace flag's value is not the package being installed. + In: "npm install -w core lodash", + }, { // Test 22: A workspace install is not an install in a new project. + In: "npm install --workspace=core zx", + }, { // Test 23: A prefix sends the install somewhere other than the scratch project. + In: "npm install --prefix ./app zx", + }, { // Test 24: A flag outside the known set is not guessed at. + In: "npm install zx --install-strategy nested", + }, { // Test 25: A path riding along fails in a new project, so the line is no route. + In: "npm install zx ./local", + }, { // Test 26: A URL riding along is refused the same way. + In: "npm install zx https://example.com/other.tgz", + }, { // Test 27: A scoped registry setting is a registry by another name. + In: "npm install zx --@scope:registry=https://npm.example.com", + }, { // Test 28: A scoped name keeps its version on the module and matches without it. + In: "npm install @scope/tool@2", WantOK: true, WantModule: "@scope/tool@2", WantBinary: "tool", + }, { // Test 29: A stand-in version is recorded and not run. + In: "npm install zx@xxxx", WantOK: true, WantModule: "zx@xxxx", WantBinary: "zx", + WantNoRun: true, + }, { // Test 30: The glued prefix form is refused by the flag list alone. + In: "npm install --prefix=./app zx", + }, { // Test 31: A quoted name is the name. + In: `npm install "zx"`, WantOK: true, WantModule: "zx", WantBinary: "zx", + }, { // Test 32: A quote that never closes is a broken line, not a route. + In: `npm install "zx`, + }, { // Test 33: Two names inside one pair of quotes are one argument npm rejects. + In: `npm install "zx lodash"`, + }} + for testNum, test := range tests { + t.Run(fmt.Sprintf("test %d", testNum), func(t *testing.T) { + t.Parallel() + got, _, ok := localNpmStep("checkout", test.In, owned) + if diff := cmp.Diff(test.WantOK, ok); diff != "" { + t.Fatalf("ok mismatch (-want +got):\n%s", diff) + } + if diff := cmp.Diff(test.WantModule, got.Module); diff != "" { + t.Errorf("module mismatch (-want +got):\n%s", diff) + } + if diff := cmp.Diff(test.WantBinary, got.Binary); diff != "" { + t.Errorf("binary mismatch (-want +got):\n%s", diff) + } + if ok && (got.Kind != "npm-install" || got.Run == test.WantNoRun) { + t.Errorf("step = %s run %v, want npm-install run %v", got.Kind, got.Run, !test.WantNoRun) + } + if _, _, plain := localNpmStep("checkout", test.In, repoNames{}); plain { + t.Errorf("a repository that declares nothing gave a route for %q", test.In) + } + }) + } +} + +// TestExtractLocalAndGlobalNpm checks that a local and a global install of one +// package are two routes. Each is a line a reader may type, and sharing one +// key left the second unverified. +func TestExtractLocalAndGlobalNpm(t *testing.T) { + t.Parallel() + owned := repoNames{npm: map[string][]string{"zx": {"zx"}}} + tests := []struct { + In string + WantRaws []string + }{{ // Test 0: The local line first. + In: "```sh\nnpm install zx\nnpm install -g zx\n```\n", + WantRaws: []string{"npm install zx", "npm install -g zx"}, + }, { // Test 1: The global line first. + In: "```sh\nnpm install -g zx\nnpm install zx\n```\n", + WantRaws: []string{"npm install -g zx", "npm install zx"}, + }, { // Test 2: A repeated local line is still one route. + In: "```sh\nnpm install zx\n```\n\n```sh\nnpm install zx\n```\n", + WantRaws: []string{"npm install zx"}, + }} + for testNum, test := range tests { + t.Run(fmt.Sprintf("test %d", testNum), func(t *testing.T) { + t.Parallel() + var got []string + for _, s := range ExtractorWith(owned).Extract("zx", test.In) { + got = append(got, s.Raw) + } + if diff := cmp.Diff(test.WantRaws, got); diff != "" { + t.Errorf("steps mismatch (-want +got):\n%s", diff) + } + }) + } +} + // TestPackageBinary checks the fallback binary name guessed for a package, // used only when the install adds nothing detectable to the bin directory. func TestPackageBinary(t *testing.T) { @@ -429,7 +586,7 @@ func TestExtractListRecipe(t *testing.T) { t.Run(fmt.Sprintf("test %d", testNum), func(t *testing.T) { t.Parallel() var got []string - for _, s := range DefaultExtractor().Extract("tool", test.In) { + for _, s := range ExtractorWith(repoNames{}).Extract("tool", test.In) { if s.Kind == "git-clone" { got = s.Block break diff --git a/flagahead.go b/flagahead.go index b0dfd56..be4f027 100644 --- a/flagahead.go +++ b/flagahead.go @@ -6,6 +6,7 @@ import ( "os" "path/filepath" "regexp" + "slices" "strings" ) @@ -43,37 +44,65 @@ var reFlagDefinition = regexp.MustCompile( `(?i)flag|option|add_argument|arg::new|\.long\(|\bname:\s`) // unreleasedFlags returns the flags among names that the checkout's source, in -// the language of the install kind, defines. A flag the installed release rejects while HEAD defines it is a -// document ahead of the release, not a stale one: yq documented -// --ini-key-value-delimiters before shipping it, and the maintainer who adds -// a flag and its docs in one change cannot release first. A definition is a -// quoted --name, or a quoted name on a line that reads like a flag definition, -// so the bare word in an unrelated string does not count. +// the language of the install kind, defines. A flag the installed release +// rejects while HEAD defines it is a document ahead of the release, not a +// stale one: yq documented --ini-key-value-delimiters before shipping it, and +// the maintainer who adds a flag and its docs in one change cannot release +// first. A definition is a quoted --name, or a quoted name on a line that +// reads like a flag definition, so the bare word in an unrelated string does +// not count. func unreleasedFlags(dir, kind string, names []string) map[string]bool { - found := map[string]bool{} - exts := ecosystemSources[installEcosystem(kind)] - if dir == "" || len(names) == 0 || exts == nil { - return found - } - type pattern struct { - // dashed matches the name quoted with its dashes. - dashed *regexp.Regexp - // quoted matches the bare name in quotes. - quoted *regexp.Regexp + defines := map[string]func(string) bool{} + for _, n := range names { + q := regexp.QuoteMeta(n) + dashed := regexp.MustCompile(`["'` + "`" + `]--?` + q + `([^\w-]|$)`) + quoted := regexp.MustCompile(`["'` + "`" + `]` + q + `["'` + "`" + `]`) + defines[n] = func(line string) bool { + return dashed.MatchString(line) || (quoted.MatchString(line) && reFlagDefinition.MatchString(line)) + } } - pats := map[string]pattern{} + return sourceDefines(dir, kind, defines) +} + +// unreleasedSubs returns the subcommands among names that the checkout's +// source defines, the subcommand side of unreleasedFlags. sqlc documented +// analyze and fmt while its latest release had neither. A definition is the +// parser's own declaration of the name: cobra's Use: "name" with or without +// argument text, urfave's Name: "name", clap's Command::new("name") or +// #[command(name = "name")], and argparse's add_parser("name"). +func unreleasedSubs(dir, kind string, names []string) map[string]bool { + defines := map[string]func(string) bool{} for _, n := range names { q := regexp.QuoteMeta(n) - pats[n] = pattern{ - dashed: regexp.MustCompile(`["'` + "`" + `]--?` + q + `([^\w-]|$)`), - quoted: regexp.MustCompile(`["'` + "`" + `]` + q + `["'` + "`" + `]`), + pats := []*regexp.Regexp{ + regexp.MustCompile(`\bUse:\s*"` + q + `(\s[^"]*)?"`), + regexp.MustCompile(`\bName:\s*"` + q + `"`), + regexp.MustCompile(`Command::new\(\s*"` + q + `"\s*\)`), + regexp.MustCompile(`#\[command\([^)]*\bname\s*=\s*"` + q + `"`), + regexp.MustCompile(`add_parser\(\s*["']` + q + `["']`), } + defines[n] = func(line string) bool { + return slices.ContainsFunc(pats, func(re *regexp.Regexp) bool { return re.MatchString(line) }) + } + } + return sourceDefines(dir, kind, defines) +} + +// sourceDefines walks the checkout's source in the language of the install +// kind and returns the names with a line their define function accepts. +// Hidden, vendored, and generated directories, oversized files, and minified +// lines are skipped, since none of them is where a project declares its CLI. +func sourceDefines(dir, kind string, defines map[string]func(string) bool) map[string]bool { + found := map[string]bool{} + exts := ecosystemSources[installEcosystem(kind)] + if dir == "" || len(defines) == 0 || exts == nil { + return found } _ = filepath.WalkDir(dir, func(p string, d fs.DirEntry, err error) error { if err != nil { return nil } - if len(found) == len(names) { + if len(found) == len(defines) { return filepath.SkipAll } if d.IsDir() { @@ -101,11 +130,8 @@ func unreleasedFlags(dir, kind string, names []string) map[string]bool { if len(line) > sourceMaxLine { continue } - for n, pat := range pats { - if found[n] || !strings.Contains(line, n) { - continue - } - if pat.dashed.MatchString(line) || (pat.quoted.MatchString(line) && reFlagDefinition.MatchString(line)) { + for n, def := range defines { + if !found[n] && strings.Contains(line, n) && def(line) { found[n] = true } } diff --git a/flagahead_test.go b/flagahead_test.go index 40984cf..68849fc 100644 --- a/flagahead_test.go +++ b/flagahead_test.go @@ -108,3 +108,77 @@ func TestFlagCheckUnreleased(t *testing.T) { }) } } + +// TestFlagCheckUnreleasedSubcommand checks that a cited subcommand the release +// rejects but the source at HEAD declares is reported as unreleased, not as +// drift. sqlc's docs cite analyze and fmt, which its latest release lacks. +func TestFlagCheckUnreleasedSubcommand(t *testing.T) { + t.Parallel() + const sqlcCmd = "\t\tUse: \"analyze [query-file]\",\n\t\tUse: \"fmt\",\n\tUse: \"generate\",\n" + tests := []struct { + Source map[string]string + Kind string + Subs []string + WantStatus Status + WantDetail string + }{{ // Test 0: sqlc's two cobra subcommands are unreleased. + Source: map[string]string{"internal/cmd/cmd.go": sqlcCmd}, Kind: "go-install", + Subs: []string{"analyze", "fmt"}, + WantStatus: StatusVerified, + WantDetail: "0 cited flags ok, 2 subcommands cited, 2 unreleased (analyze fmt), " + + "defined in the source at HEAD but not in the installed release", + }, { // Test 1: a subcommand neither help nor source knows still drifts. + Source: map[string]string{"internal/cmd/cmd.go": sqlcCmd}, Kind: "go-install", + Subs: []string{"vet"}, + WantStatus: StatusDrift, WantDetail: "unknown subcommand vet", + }, { // Test 2: urfave, clap, and argparse declarations count too. + Source: map[string]string{ + "cmd/app.go": "\t{Name: \"sync\", Usage: \"sync it\"},\n", + "src/main.rs": " .subcommand(Command::new(\"pack\"))\n #[command(name = \"lint\")]\n", + "cli.py": "sub.add_parser('serve')\n", + }, + Kind: "go-install", Subs: []string{"sync"}, + WantStatus: StatusVerified, + WantDetail: "0 cited flags ok, 1 subcommands cited, 1 unreleased (sync), " + + "defined in the source at HEAD but not in the installed release", + }, { // Test 3: the name in a comment or a string is not a declaration. + Source: map[string]string{"cmd/root.go": "\t// run analyze first\n\tfmt.Println(\"analyze\")\n"}, + Kind: "go-install", Subs: []string{"analyze"}, + WantStatus: StatusDrift, WantDetail: "unknown subcommand analyze", + }} + for testNum, test := range tests { + t.Run(fmt.Sprintf("test %d", testNum), func(t *testing.T) { + t.Parallel() + dir := t.TempDir() + for name, body := range test.Source { + p := filepath.Join(dir, name) + if err := os.MkdirAll(filepath.Dir(p), 0o750); err != nil { + t.Fatal(err) + } + writeRepoFile(t, filepath.Dir(p), filepath.Base(p), body) + } + help := "Usage: tool [command]\n -h, --help help for tool\n" + for _, s := range test.Subs { + help += fmt.Sprintf("Error: unknown command %q for \"tool\"\n", s) + } + res := Result{ + Step: InstallStep{ + Repo: "tool", Kind: test.Kind, Binary: "tool", dir: dir, + Usage: &Usage{Subs: test.Subs}, + }, + Status: StatusVerified, + helpText: help, + } + got := flagChecks([]Result{res}) + if len(got) != 1 { + t.Fatalf("got %d flag checks, want 1", len(got)) + } + if diff := cmp.Diff(test.WantStatus, got[0].Status); diff != "" { + t.Errorf("status mismatch (-want +got):\n%s", diff) + } + if diff := cmp.Diff(test.WantDetail, got[0].Detail); diff != "" { + t.Errorf("detail mismatch (-want +got):\n%s", diff) + } + }) + } +} diff --git a/flagcheck.go b/flagcheck.go index 818e0c9..40fb5ef 100644 --- a/flagcheck.go +++ b/flagcheck.go @@ -310,6 +310,21 @@ func rejectedSub(r Result, sub string) bool { return named.MatchString(r.helpBySub[sub]) || named.MatchString(r.helpText) } +// appendCheck adds check to out unless an earlier check already reports the +// same binary with the same outcome. Two install routes of one binary, such as +// a global and a local npm install, print the same help, and one finding is +// one row. +func appendCheck(out []Result, check Result) []Result { + for _, c := range out { + if c.Step.Repo == check.Step.Repo && c.Step.Kind == check.Step.Kind && + c.Step.Binary == check.Step.Binary && c.Status == check.Status && + c.Reason == check.Reason && c.Detail == check.Detail { + return out + } + } + return append(out, check) +} + // flagChecks derives one flag-check result per binary whose install succeeded // and whose README cites usage. A cited flag missing from every collected // help screen, or a subcommand the binary rejects, is reported as drift. @@ -329,7 +344,7 @@ func flagChecks(results []Result) []Result { if len(known) == 0 && len(r.helpByFlag) == 0 { check.Status = StatusSkipped check.Detail = "no help output to check against" - out = append(out, check) + out = appendCheck(out, check) continue } var rejected, missing, unreleased, unverifiable []string @@ -371,21 +386,32 @@ func flagChecks(results []Result) []Result { missing = append(missing, r.Step.Usage.dashOf(f)+f) } } + var rejectedSubs, badSubs, unreleasedSubList []string + for _, s := range r.Step.Usage.Subs { + if rejectedSub(r, s) { + rejectedSubs = append(rejectedSubs, s) + } + } + // A subcommand the release rejects while HEAD's source declares it is + // the same document-ahead case as a flag, and joins the same note. + aheadSubs := unreleasedSubs(r.Step.dir, r.Step.Kind, rejectedSubs) + for _, s := range rejectedSubs { + if aheadSubs[s] { + unreleasedSubList = append(unreleasedSubList, s) + } else { + badSubs = append(badSubs, s) + } + } sort.Strings(unreleased) + sort.Strings(unreleasedSubList) release := "the installed release" if r.SmokeLine != "" { release = "the installed " + r.SmokeLine } unreleasedNote := "" - if len(unreleased) > 0 { + if all := append(slices.Clone(unreleased), unreleasedSubList...); len(all) > 0 { unreleasedNote = fmt.Sprintf("%d unreleased (%s), defined in the source at HEAD but not in %s", - len(unreleased), strings.Join(unreleased, " "), release) - } - var badSubs []string - for _, s := range r.Step.Usage.Subs { - if rejectedSub(r, s) { - badSubs = append(badSubs, s) - } + len(all), strings.Join(all, " "), release) } sort.Strings(missing) sort.Strings(badSubs) @@ -415,7 +441,7 @@ func flagChecks(results []Result) []Result { } check.Detail = strings.Join(parts, ", ") } - out = append(out, check) + out = appendCheck(out, check) } return out } diff --git a/internal/docblock/docblock.go b/internal/docblock/docblock.go index a5cb6c5..ee07f50 100644 --- a/internal/docblock/docblock.go +++ b/internal/docblock/docblock.go @@ -31,6 +31,11 @@ type Block struct { // before it, "on macOS run the following", and a planner that cannot see // that sentence convicts commands written for another operating system. Intro string + // Lead is the paragraph that ends directly above a fenced block, within + // two lines and with no other block between them, empty otherwise. It is + // where a tutorial names the file a block holds: "create a file named + // `app.py`" followed by the file's body. + Lead string // Span reports whether this is an inline span rather than a block. Span bool // Line is the 1-based README line the block's content starts on. @@ -86,7 +91,8 @@ func CodeBlocks(markdown string) []Block { src := stripFrontMatter([]byte(markdown)) doc := goldmark.New().Parser().Parse(text.NewReader(src)) var blocks []Block - var heading, intro string + var heading, intro, lead string + leadEnd := 0 lists, depth, list, item := 0, 0, 0, 0 _ = ast.Walk(doc, func(n ast.Node, entering bool) (ast.WalkStatus, error) { switch n.(type) { @@ -110,16 +116,27 @@ func CodeBlocks(markdown string) []Block { } if p, ok := n.(*ast.Paragraph); ok { intro = spanText(p, src) + lead, leadEnd = rawText(p, src), 0 + if ls := p.Lines(); ls.Len() > 0 { + leadEnd = lineAt(src, ls.At(ls.Len()-1).Start) + } return ast.WalkContinue, nil } b := Block{Heading: heading, Intro: intro, List: list, Item: item} switch node := n.(type) { case *ast.Heading: heading = spanText(node, src) - intro = "" + intro, lead = "", "" return ast.WalkContinue, nil case *ast.FencedCodeBlock: b.Lang = string(node.Language(src)) + // The content starts the line after the fence, so a paragraph + // ending on the line before the fence is two lines up and one + // with a blank line between is three. + if lead != "" && leadEnd > 0 && BlockLine(node, src)-leadEnd <= 3 { + b.Lead = lead + } + lead = "" if node.Info != nil { if m := reFenceTitle.FindStringSubmatch(string(node.Info.Segment.Value(src))); m != nil { b.Title = m[1] + m[2] @@ -128,6 +145,7 @@ func CodeBlocks(markdown string) []Block { b.Lines = strings.Split(BlockText(node, src), "\n") b.Line = BlockLine(node, src) case *ast.CodeBlock: + lead = "" if admonitionMarker.MatchString(intro) { blocks = append(blocks, admonitionBlocks(node, src, heading, intro)...) return ast.WalkContinue, nil @@ -240,6 +258,18 @@ func spanText(n ast.Node, src []byte) string { return b.String() } +// rawText returns a block node's source lines joined by spaces, markup kept, +// so a backticked file name in a paragraph is still marked as one. +func rawText(l liner, src []byte) string { + lines := l.Lines() + parts := make([]string, 0, lines.Len()) + for i := 0; i < lines.Len(); i++ { + seg := lines.At(i) + parts = append(parts, strings.TrimSpace(string(seg.Value(src)))) + } + return strings.Join(parts, " ") +} + // ShownFailures returns the commands of a prompted block whose displayed // output is an error. Docs sometimes show a command failing on purpose, to // teach why the corrected form that follows is needed, and the demonstrated @@ -458,9 +488,10 @@ var ( // , xxxx runs, path/to/ and /home/user stand-ins, // double-brace templates such as a config example's {{.Field | quote}}, a // repository under a stand-in owner such as github.com/example/, a stand-in ref -// such as @fix-something, and values that trail off in an ellipsis. The -// ellipsis is ignored after a slash so a Go package pattern such as ./... is -// not mistaken for one. +// such as @fix-something, values that trail off in an ellipsis, an ellipsis +// standing in for a leading path such as .../install.sh, and numbered +// all-caps stand-ins such as UTILITY_1. The ellipsis is ignored after a slash +// so a Go package pattern such as ./... is not mistaken for one. var rePlaceholder = regexp.MustCompile( `<[^<>\s]+>|=<[^<>]+>|(^|[^${])\{[A-Za-z][A-Za-z0-9_]*\}|(^|\s)\[[^\[\]\s]+\](\s|$)` + `|\{\{[^{}]*\}\}` + @@ -471,7 +502,7 @@ var rePlaceholder = regexp.MustCompile( `|yourusername|your-github-username|my-?org|my-?user)/` + `|@(fix-something|my-branch|some-branch|your-branch|feature-branch|abc123def|abc123` + `|deadbeef|1234567|123abc)\b` + - `|(^|[^/])\.\.\.($|[\s'".])`) + `|(^|[^/])\.\.\.($|[\s'"./])|\b[A-Z][A-Z]+_[0-9]+\b`) // HasPlaceholder reports whether s holds a token the reader must replace // before running it. The planner asks it of an example line and the extractor diff --git a/internal/docblock/docblock_test.go b/internal/docblock/docblock_test.go index ffcd635..1584195 100644 --- a/internal/docblock/docblock_test.go +++ b/internal/docblock/docblock_test.go @@ -98,6 +98,20 @@ func TestHasPlaceholder(t *testing.T) { In: `fzf --delimiter : --nth 3.. --bind 'enter:become(vim {1} +{2})'`, Want: false, }, { // Test 21: A named slot still is. In: `tool {name}`, Want: true, + }, { // Test 22: An ellipsis standing in for a leading path is a stand-in. + In: `curl -fsSL .../install.sh | sh`, Want: true, + }, { // Test 23: Numbered all-caps names are stand-ins. + In: `make UTILS='UTILITY_1 UTILITY_2'`, Want: true, + }, { // Test 24: An all-caps variable without a number is not. + In: `export GITHUB_TOKEN=x`, Want: false, + }, { // Test 25: An all-caps assignment without a number is not. + In: `FOO_BAR=1 ./run`, Want: false, + }, { // Test 26: An ellipsis inside a word is not. + In: `echo a...b`, Want: false, + }, { // Test 27: A Go package pattern is not. + In: `go test ./...`, Want: false, + }, { // Test 28: A Go package pattern under a directory is not. + In: `go build ./cmd/...`, Want: false, }} for testNum, test := range tests { diff --git a/internal/docblock/exitcodes.go b/internal/docblock/exitcodes.go new file mode 100644 index 0000000..73714f6 --- /dev/null +++ b/internal/docblock/exitcodes.go @@ -0,0 +1,71 @@ +package docblock + +import ( + "regexp" + "slices" + "strconv" + "strings" +) + +var ( + // reExitHeading matches a heading that opens a section about exit codes. + reExitHeading = regexp.MustCompile(`(?i)^#{1,6}\s.*\bexit (codes?|status(es)?)\b`) + // reAnyHeading matches any markdown heading line. + reAnyHeading = regexp.MustCompile(`^#{1,6}\s`) + // reExitLead matches prose that starts a run of exit codes, such as witr's + // "Exit codes: 0 found, 1 found with warnings, 2 not found". + reExitLead = regexp.MustCompile(`(?i)\bexit (codes?|status(es)?)\s*:`) + // reExitRow matches one documented code and its meaning, as a table row, + // a list item, or one entry of a comma-separated run. + reExitRow = regexp.MustCompile("^\\s*[|*-]?\\s*`?(\\d{1,3})`?\\s*[|:=)–—-]?\\s*(.+)$") + // reNotFoundMeaning matches the meanings that say a lookup came back empty. + reNotFoundMeaning = regexp.MustCompile(`(?i)\b(not found|no match(es)?|nothing found)\b`) +) + +// NotFoundCodes returns the exit codes a document says mean that nothing was +// found, from a section headed as exit codes or a prose run that starts with +// "Exit codes:". Only a meaning of not found, no match, or nothing found +// counts. Every other documented code, including any failure a project +// documents, is left out, so a table cannot excuse a real error. Codes are +// nonzero ordinary exits only, in ascending order. +func NotFoundCodes(markdown string) []int { + var spans []string + lines := strings.Split(ProseOnly(markdown), "\n") + for i := 0; i < len(lines); i++ { + line := lines[i] + switch { + case reExitHeading.MatchString(line): + var sec []string + for i++; i < len(lines) && !reAnyHeading.MatchString(lines[i]); i++ { + sec = append(sec, lines[i]) + } + i-- + spans = append(spans, strings.Join(sec, "\n")) + case reExitLead.MatchString(line): + para := []string{line[reExitLead.FindStringIndex(line)[1]:]} + for i+1 < len(lines) && strings.TrimSpace(lines[i+1]) != "" && !reAnyHeading.MatchString(lines[i+1]) { + i++ + para = append(para, lines[i]) + } + spans = append(spans, strings.Join(para, " ")) + } + } + var codes []int + for _, span := range spans { + for _, piece := range strings.FieldsFunc(span, func(r rune) bool { + return r == ',' || r == ';' || r == '\n' + }) { + m := reExitRow.FindStringSubmatch(piece) + if m == nil || !reNotFoundMeaning.MatchString(m[2]) { + continue + } + code, err := strconv.Atoi(m[1]) + if err != nil || code < 1 || code > 125 || code == 124 || slices.Contains(codes, code) { + continue + } + codes = append(codes, code) + } + } + slices.Sort(codes) + return codes +} diff --git a/internal/docblock/exitcodes_test.go b/internal/docblock/exitcodes_test.go new file mode 100644 index 0000000..a978c53 --- /dev/null +++ b/internal/docblock/exitcodes_test.go @@ -0,0 +1,55 @@ +package docblock + +import ( + "fmt" + "testing" + + "github.com/google/go-cmp/cmp" + "github.com/google/go-cmp/cmp/cmpopts" +) + +// TestNotFoundCodes checks that only a documented code meaning nothing was +// found is collected, from a prose run, a table, or a list under an exit code +// heading, and that a failure code or a code outside such a section is not. +func TestNotFoundCodes(t *testing.T) { + t.Parallel() + tests := []struct { + Markdown string + WantCodes []int + }{{ // Test 0: witr's prose run names 2 as not found and nothing else. + Markdown: "## Process and port debugging\n\n" + + "Exit codes: 0 found, 1 found with warnings (not a failure), 2 not found,\n" + + "3 permission denied (retry with sudo), 4 ambiguous name or bad input\n" + + "(re-run with --pid), 5 internal error, 6 found but what started it can't\n" + + "be traced. Run `witr --help` for all options.\n", + WantCodes: []int{2}, + }, { // Test 1: a table under an exit status heading. + Markdown: "## Exit status\n\n| Code | Meaning |\n|---|---|\n| 0 | success |\n" + + "| 1 | no matches |\n| 2 | error |\n", + WantCodes: []int{1}, + }, { // Test 2: a list under an exit codes heading. + Markdown: "### Exit codes\n\n- `0`: ok\n- `3`: nothing found\n- `4`: bad input\n", + WantCodes: []int{3}, + }, { // Test 3: a documented failure is never a not-found code. + Markdown: "## Exit codes\n\n- 2 permission denied\n", + WantCodes: nil, + }, { // Test 4: not found outside an exit code section is not a code. + Markdown: "## Usage\n\nIf the file is missing, 2 not found is printed.\n", + WantCodes: nil, + }, { // Test 5: the section ends at the next heading. + Markdown: "## Exit codes\n\n- 1 success\n\n## Errors\n\n- 2 not found\n", + WantCodes: nil, + }, { // Test 6: zero and shell codes are never collected. + Markdown: "## Exit codes\n\n- 0 not found\n- 127 not found\n", + WantCodes: nil, + }} + for testNum, test := range tests { + t.Run(fmt.Sprintf("test %d", testNum), func(t *testing.T) { + t.Parallel() + got := NotFoundCodes(test.Markdown) + if diff := cmp.Diff(test.WantCodes, got, cmpopts.EquateEmpty()); diff != "" { + t.Errorf("codes mismatch (-want +got):\n%s", diff) + } + }) + } +} diff --git a/internal/plan/plan.go b/internal/plan/plan.go index ec2ed5c..1794b99 100644 --- a/internal/plan/plan.go +++ b/internal/plan/plan.go @@ -42,6 +42,14 @@ type Plan struct { Settings []string `json:"settings,omitempty"` // Excluded counts code blocks that are not shell recipes. Excluded int `json:"excluded,omitempty"` + // NotFoundCodes are the exit codes the documents say mean a lookup found + // nothing, such as witr's "2 not found". A documented binary exiting with + // one of them reports an empty result rather than a broken command. + NotFoundCodes []int `json:"notFoundCodes,omitempty"` + // RootGoFiles are the Go files at the top of the checkout before the + // session starts. A Go build that finds a document's file beside one of + // them met the repository's own package, which the document never wrote. + RootGoFiles []string `json:"rootGoFiles,omitempty"` } // PlanInstall is one documented install the session runs before replaying @@ -182,7 +190,8 @@ func (r *Recognizer) provides(flat string) string { } func BuildPlan(repo, dir, markdown string, binaries []string, installs []PlanInstall, cfg *config.ExamplesConfig, rec *Recognizer) *Plan { - p := &Plan{Repo: repo, Installs: installs, Binaries: binaries} + p := &Plan{Repo: repo, Installs: installs, Binaries: binaries, + NotFoundCodes: docblock.NotFoundCodes(markdown), RootGoFiles: rootGoFiles(dir)} pl := &planner{ rec: rec, plan: p, @@ -239,9 +248,18 @@ func BuildPlan(repo, dir, markdown string, binaries []string, installs []PlanIns pl.addFileBlock(block, path) continue } + if path := proseFile(block); path != "" && !block.Span { + pl.addFileBlock(block, path) + continue + } if block.Span || !docblock.ShellLangs[block.Lang] { continue } + if path, body := fileBodyBlock(block); path != "" { + block.Lines = body + pl.addFileBlock(block, path) + continue + } pl.addBlock(block) } for pkg := range pl.packages { @@ -402,6 +420,9 @@ func (pl *planner) addBlock(block docblock.Block) { Heading: block.Heading, } scopedTo := otherPlatform(block.Intro) + if scopedTo == "" { + scopedTo = platformHeading(block.Heading) + } lineIn := docblock.SourceLineIndex(block) shownErr := docblock.ShownFailures(block) nonzero := false @@ -561,6 +582,158 @@ func fileTitle(title string) string { return strings.TrimPrefix(title, "./") } +// proseExts maps a fence language to the file extensions a block in it can +// hold. A named file has to agree with its fence, which is what separates the +// tutorial's "save this as `app.py`" from a paragraph that mentions a file in +// passing above an unrelated example. +var proseExts = map[string][]string{ + "py": {"py"}, "python": {"py"}, "go": {"go"}, + "yaml": {"yaml", "yml"}, "yml": {"yaml", "yml"}, "toml": {"toml"}, "json": {"json"}, + "js": {"js", "mjs", "cjs"}, "javascript": {"js", "mjs", "cjs"}, + "ts": {"ts", "mts"}, "typescript": {"ts", "mts"}, "rs": {"rs"}, "rust": {"rs"}, + "sql": {"sql"}, "api": {"api"}, "ini": {"ini", "cfg"}, +} + +var ( + // reBacktickPath matches a backticked token shaped like a file path with + // an extension, and captures the path and the extension. + reBacktickPath = regexp.MustCompile("`([\\w./-]*[\\w-]\\.([A-Za-z0-9]{1,8}))`") + // reFileVerb matches the words a sentence uses to tell the reader to put + // a block into a file. + reFileVerb = regexp.MustCompile(`(?i)\b(create|creating|save|write|add|put|name|named|called|open|edit|file|into)\b`) +) + +// proseFile returns the path a fenced block's lead paragraph tells the reader +// to put it in, or empty when the paragraph does not. gin, flask, sqlc, and +// typer show a file in one block and run it in the next, naming it only in the +// sentence above: "create a file `app.py`". The block needs a language other +// than a shell, no title, and a body. The paragraph must name exactly one +// file, backticked, with an extension the fence language allows, in a sentence +// that tells the reader to create, save, or edit it. A placeholder path or one +// fileTitle rejects is never written, and addFileBlock still refuses a path +// the checkout holds. +func proseFile(block docblock.Block) string { + exts := proseExts[strings.ToLower(block.Lang)] + if block.Title != "" || exts == nil || !hasCommandLine(block.Lines) { + return "" + } + if path := namedFile(block.Lead, exts); path != "" { + return path + } + // flask names the file in the block's own first line instead, as a + // comment: "# save this as app.py". + for _, ln := range block.Lines { + t := strings.TrimSpace(ln) + if t == "" { + continue + } + if c, ok := strings.CutPrefix(t, "#"); ok { + return namedFile(markBareFiles(c, exts), exts) + } + if c, ok := strings.CutPrefix(t, "//"); ok { + return namedFile(markBareFiles(c, exts), exts) + } + return "" + } + return "" +} + +// markBareFiles backticks the bare words of a code comment that end in one +// of exts, so a comment is read by the same rule as prose, where file names +// are marked. +func markBareFiles(comment string, exts []string) string { + words := strings.Fields(comment) + for i, w := range words { + trimmed := strings.TrimRight(w, ".,:;") + if j := strings.LastIndex(trimmed, "."); j > 0 && slices.Contains(exts, strings.ToLower(trimmed[j+1:])) { + words[i] = "`" + trimmed + "`" + w[len(trimmed):] + } + } + return strings.Join(words, " ") +} + +// namedFile returns the one file text tells the reader to put a block in: a +// single backticked path whose extension is among exts, in a sentence with a +// verb such as create or save. Two different paths, a placeholder, or a path +// fileTitle rejects name nothing. +func namedFile(text string, exts []string) string { + if text == "" { + return "" + } + var path string + at := 0 + for _, m := range reBacktickPath.FindAllStringSubmatchIndex(text, -1) { + p, ext := text[m[2]:m[3]], strings.ToLower(text[m[4]:m[5]]) + if !slices.Contains(exts, ext) { + continue + } + if path != "" && path != p { + return "" + } + path, at = p, m[0] + } + if path == "" || docblock.HasPlaceholder(path) || !reFileVerb.MatchString(sentenceAt(text, at)) { + return "" + } + return fileTitle(path) +} + +// sentenceAt returns the sentence of text that holds the byte offset at. +func sentenceAt(text string, at int) string { + start := 0 + for _, sep := range []string{". ", "! ", "? "} { + if i := strings.LastIndex(text[:at], sep); i >= 0 && i+len(sep) > start { + start = i + len(sep) + } + } + end := len(text) + for _, sep := range []string{". ", "! ", "? "} { + if i := strings.Index(text[at:], sep); i >= 0 && at+i < end { + end = at + i + } + } + return text[start:end] +} + +// reFileComment matches a comment line that is nothing but a file path with +// an extension, the way a document captions a block holding a file's body. +var reFileComment = regexp.MustCompile(`^#\s*([\w./-]+\.[A-Za-z]{1,8})\s*$`) + +// fileBodyBlock returns the path and body of a shell block whose first line +// is a comment naming one file, such as asdf's "# test/some_tests.bats" above +// the test it shows. The body is the lines after that comment, without the +// blank lines that follow it. A block with nothing after the comment, a path +// that is a placeholder, or a path fileTitle rejects is not a file body, and +// the result is empty. +func fileBodyBlock(block docblock.Block) (string, []string) { + first := -1 + for i, ln := range block.Lines { + if strings.TrimSpace(ln) != "" { + first = i + break + } + } + if first < 0 { + return "", nil + } + m := reFileComment.FindStringSubmatch(strings.TrimSpace(block.Lines[first])) + if m == nil || docblock.HasPlaceholder(m[1]) { + return "", nil + } + path := fileTitle(m[1]) + if path == "" { + return "", nil + } + body := block.Lines[first+1:] + for len(body) > 0 && strings.TrimSpace(body[0]) == "" { + body = body[1:] + } + if len(body) == 0 { + return "", nil + } + return path, body +} + // addFileBlock plans a titled block as the document writing the file it names. // A path the checkout already holds is never written, since overwriting the // repository's own file would test kibble's copy rather than the project's. @@ -676,6 +849,13 @@ func (p *Plan) Merge(doc string, other *Plan) { p.Binaries = mergeUnique(p.Binaries, other.Binaries) p.Packages = mergeUnique(p.Packages, other.Packages) p.Settings = mergeUnique(p.Settings, other.Settings) + for _, c := range other.NotFoundCodes { + if !slices.Contains(p.NotFoundCodes, c) { + p.NotFoundCodes = append(p.NotFoundCodes, c) + } + } + slices.Sort(p.NotFoundCodes) + p.RootGoFiles = mergeUnique(p.RootGoFiles, other.RootGoFiles) if p.Env == nil && len(other.Env) > 0 { p.Env = map[string]string{} } diff --git a/internal/plan/plan_test.go b/internal/plan/plan_test.go index c110b7f..28f17fe 100644 --- a/internal/plan/plan_test.go +++ b/internal/plan/plan_test.go @@ -304,6 +304,77 @@ func TestBuildPlanRepoTree(t *testing.T) { } } +// TestBuildPlanRootGoFiles checks that a plan records the Go files at the top +// of the checkout and nothing below it, and that merging documents keeps them +// once. +func TestBuildPlanRootGoFiles(t *testing.T) { + t.Parallel() + tests := []struct { + Files []string + Dirs []string + Links map[string]string + NoDir bool + WantFiles []string + }{{ // Test 0: Root Go files are kept in name order, other files and nested ones are not. + Files: []string{"zlib.go", "doc.go", "README.md", "cmd/tool/main.go", "go.mod"}, + WantFiles: []string{"doc.go", "zlib.go"}, + }, { // Test 1: A directory named like a Go file is not one. + Files: []string{"main.go"}, + Dirs: []string{"gen.go"}, + WantFiles: []string{"main.go"}, + }, { // Test 2: A checkout with no Go file at its top records none. + Files: []string{"README.md", "internal/x/x.go"}, + }, { // Test 3: A plan built without a checkout records none. + NoDir: true, + }, { // Test 4: A link to a file is a root Go file, a link to a directory or nothing is not. + Files: []string{"internal/gen/real.go", "README.md"}, + Links: map[string]string{ + "api.go": "internal/gen/real.go", "dir.go": "internal", "gone.go": "nope", + }, + WantFiles: []string{"api.go"}, + }} + for testNum, test := range tests { + t.Run(fmt.Sprintf("test %d", testNum), func(t *testing.T) { + t.Parallel() + dir := "" + if !test.NoDir { + dir = t.TempDir() + } + for _, d := range test.Dirs { + if err := os.MkdirAll(filepath.Join(dir, d), 0o755); err != nil { + t.Fatal(err) + } + } + for _, f := range test.Files { + full := filepath.Join(dir, f) + if err := os.MkdirAll(filepath.Dir(full), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(full, []byte("package x\n"), 0o644); err != nil { + t.Fatal(err) + } + } + for name, target := range test.Links { + if err := os.Symlink(filepath.Join(dir, target), filepath.Join(dir, name)); err != nil { + t.Fatal(err) + } + } + md := "```sh\ntool run\n```\n" + plan := BuildPlan("repo", dir, md, []string{"tool"}, nil, nil, testRecognizer()) + if diff := cmp.Diff(test.WantFiles, plan.RootGoFiles, cmpopts.EquateEmpty()); diff != "" { + t.Errorf("root Go files mismatch (-want +got):\n%s", diff) + } + merged := &Plan{} + merged.Merge("README.md", plan) + merged.Merge("docs/guide.md", BuildPlan("repo", dir, md, []string{"tool"}, nil, nil, + testRecognizer())) + if diff := cmp.Diff(test.WantFiles, merged.RootGoFiles, cmpopts.EquateEmpty()); diff != "" { + t.Errorf("merged root Go files mismatch (-want +got):\n%s", diff) + } + }) + } +} + func TestLogicalLines(t *testing.T) { t.Parallel() tests := []struct { @@ -589,6 +660,17 @@ func TestPlatformScopedBlocks(t *testing.T) { // when the final sentence moves on. Markdown: "Users on macOS often ask this. The answer is the same " + "everywhere, run the following:\n\n```sh\ntool run --fix\n```\n", + }, { // Test 6: a block under a bare macOS heading is skipped. + Markdown: "### macOS\n\n```sh\ntool run --fix\n```\n", + WantSkip: "documented for", + }, { // Test 7: a bare Windows heading scopes a block whose own + // introduction names no platform. + Markdown: "## Windows\n\nRun the following:\n\n```sh\ntool run --fix\n```\n", + WantSkip: "documented for", + }, { // Test 8: a heading that only mentions macOS does not scope the block. + Markdown: "## Building on macOS\n\n```sh\ntool run --fix\n```\n", + }, { // Test 9: a bare Linux heading runs. + Markdown: "## Linux\n\n```sh\ntool run --fix\n```\n", }} for testNum, test := range tests { t.Run(fmt.Sprintf("test %d", testNum), func(t *testing.T) { @@ -1073,6 +1155,50 @@ func TestPlanFile(t *testing.T) { Markdown: "```js title=\"prettier.config.mjs, .prettierrc.mjs\"\nexport default {}\n```\n\n" + "```sh title=\".git/hooks/pre-commit\"\nuv sync\n```\n", WantLines: []string{"uv sync file= need= line=6"}, + }, { // Test 6: a shell block captioned by a comment naming a file writes it. + Markdown: "```sh\n# scripts/run.sh\n\necho a\necho b\n```\n", + WantLines: []string{ + "mkdir -p 'scripts' && cat > 'scripts/run.sh' <<'KIBBLE_FILE_END'\necho a\necho b\nKIBBLE_FILE_END" + + " file=scripts/run.sh need= line=2", + }, + }, { // Test 7: a comment naming a platform is not a file, so the block runs. + Markdown: "```sh\n# Linux\nuv sync\n```\n", + WantLines: []string{"uv sync file= need= line=3"}, + }, { // Test 8: a comment naming a placeholder path is not a file either. + Markdown: "```sh\n# path/to/run.sh\nuv sync\n```\n", + WantLines: []string{"uv sync file= need= line=3"}, + }, { // Test 9: flask's quickstart names the file in the sentence above it. + Markdown: "Create a file named `app.py`:\n\n```python\nfrom flask import Flask\n```\n\n" + + "```sh\nuv run app.py\n```\n", + WantLines: []string{ + "cat > 'app.py' <<'KIBBLE_FILE_END'\nfrom flask import Flask\nKIBBLE_FILE_END file=app.py need= line=4", + "uv run app.py file= need= line=8", + }, + }, { // Test 10: two named files leave the block unwritten. + Markdown: "Copy `a.py` into `b.py`:\n\n```python\nx = 1\n```\n", + WantLines: nil, + }, { // Test 11: a file the fence language cannot hold is not this block. + Markdown: "Edit `config.yaml` and add:\n\n```python\nx = 1\n```\n", + WantLines: nil, + }, { // Test 12: a sentence that only mentions the file does not ask for it. + Markdown: "The `app.py` module looks like this:\n\n```python\nx = 1\n```\n", + WantLines: nil, + }, { // Test 13: a paragraph separated from the fence by another block is not its lead. + Markdown: "Create `app.py`:\n\n```sh\nuv sync\n```\n\n```python\nx = 1\n```\n", + WantLines: []string{"uv sync file= need= line=4"}, + }, { // Test 14: flask names the file in a comment on the block's first line. + Markdown: "```python\n# save this as app.py\nfrom flask import Flask\n```\n", + WantLines: []string{ + "cat > 'app.py' <<'KIBBLE_FILE_END'\n# save this as app.py\nfrom flask import Flask\nKIBBLE_FILE_END" + + " file=app.py need= line=2", + }, + }, { // Test 15: a first-line comment that only mentions a file asks for nothing. + Markdown: "```python\n# see app.py for the rest\nx = 1\n```\n", + WantLines: nil, + }, { // Test 16: a file comment the checkout already holds writes nothing. + Markdown: "```sh\n# run.sh\nuv sync\n```\n", + Tree: []string{"run.sh"}, + WantLines: nil, }} for testNum, test := range tests { t.Run(fmt.Sprintf("test %d", testNum), func(t *testing.T) { diff --git a/internal/plan/planinputs.go b/internal/plan/planinputs.go index af34eab..25af47a 100644 --- a/internal/plan/planinputs.go +++ b/internal/plan/planinputs.go @@ -319,6 +319,35 @@ func repoTree(dir string) map[string]bool { return tree } +// rootGoFiles returns the names of the Go files directly inside the checkout, +// sorted. The directory is read on its own instead of taken from repoTree, +// whose cap can stop before the walk returns to the top of a large checkout. +// A link to a file counts, since the Go tool compiles it like any other. +func rootGoFiles(dir string) []string { + if dir == "" { + return nil + } + entries, err := os.ReadDir(dir) + if err != nil { + return nil + } + var out []string + for _, e := range entries { + if !strings.HasSuffix(e.Name(), ".go") { + continue + } + regular := e.Type().IsRegular() + if e.Type()&os.ModeSymlink != 0 { + info, err := os.Stat(filepath.Join(dir, e.Name())) + regular = err == nil && info.Mode().IsRegular() + } + if regular { + out = append(out, e.Name()) + } + } + return out +} + // reSettingMention matches an environment setting named anywhere in the // document. Readers are commonly told about a variable in a table or a // sentence rather than in a runnable line, and a document that names one has diff --git a/internal/plan/planskip.go b/internal/plan/planskip.go index e0fd8fb..d5f9073 100644 --- a/internal/plan/planskip.go +++ b/internal/plan/planskip.go @@ -550,10 +550,6 @@ func describedAsLinter(markdown string) []string { return subs } -// describedAsWatcher reports whether a document introduces a binary as a tool -// that keeps running. The words have to sit near the tool's name, since a -// document can mention watching for reasons that say nothing about the -// command, and being wrong here costs a check that would have run. // reOtherPlatform matches the operating systems a Linux container cannot // stand in for. BSD sed counts because the FreeBSD and macOS sed shares a // name with GNU sed and nothing else, which is exactly why docs call it out. @@ -584,6 +580,26 @@ func otherPlatform(intro string) string { return m } +// rePlatformHeading matches a section heading that is nothing but the name of +// an operating system a Linux container cannot stand in for. +var rePlatformHeading = regexp.MustCompile(`(?i)^\s*(macOS|OS X|FreeBSD|OpenBSD|NetBSD|Windows)\s*$`) + +// platformHeading returns the platform a block's section heading scopes it to, +// or empty when the heading is anything other than a bare platform name. A +// heading such as "Building on macOS" is left to the prose rule, since the +// section under it often covers every platform. +func platformHeading(heading string) string { + m := rePlatformHeading.FindStringSubmatch(heading) + if m == nil { + return "" + } + return m[1] +} + +// describedAsWatcher reports whether a document introduces a binary as a tool +// that keeps running. The words have to sit near the tool's name, since a +// document can mention watching for reasons that say nothing about the +// command, and being wrong here costs a check that would have run. func describedAsWatcher(markdown, bin string) bool { if bin == "" { return false diff --git a/internal/verdict/verdict.go b/internal/verdict/verdict.go index ac1cc16..25b53e5 100644 --- a/internal/verdict/verdict.go +++ b/internal/verdict/verdict.go @@ -162,7 +162,9 @@ const ( // ReasonInheritedState marks a failure in a directory an earlier section // entered, so the section's own claim was not tested as written. ReasonInheritedState Reason = "inherited-state" - // ReasonCheckoutState marks a scaffold command that ran in the repository's - // own checkout, which already holds what the command creates. + // ReasonCheckoutState marks a line that failed on what the repository's own + // checkout already holds: a scaffold command refused because its target + // exists, a project manager read the checkout's project, or a Go build + // found a file the document wrote beside the checkout's own package. ReasonCheckoutState Reason = "checkout-state" ) diff --git a/lineverdict.go b/lineverdict.go index cf7e171..9f8af3f 100644 --- a/lineverdict.go +++ b/lineverdict.go @@ -56,7 +56,7 @@ func classifyLineResult(lr lineResult, l kplan.PlanLine, o lineOutcome, wrapped lr.Status = StatusBlocked lr.Reason = ReasonLongRunning lr.Detail = "ran without exiting and never reached its documented readiness signal" - lr.output = o.output + lr.output, lr.cwd = o.output, o.cwd return lr case wrapped && o.code == 124: lr.Status = StatusTimeout @@ -105,6 +105,14 @@ func classifyLineResult(lr lineResult, l kplan.PlanLine, o lineOutcome, wrapped lr.Status = StatusSkipped lr.Reason = ReasonMissingDependency lr.Detail = "invokes a command the container lacks: " + tail + case o.notFound && !o.resolver && documentedNonzeroCode(o.code): + // The project documents this exit as its lookup finding nothing, as + // witr does with "2 not found". It is checked before the silent-exit + // rule because the document has said what the code means. Only such a + // row counts: a documented failure code still convicts. + lr.Status = StatusSkipped + lr.Reason = ReasonNoDataExpected + lr.Detail = fmt.Sprintf("exit %d is the documented not-found code", o.code) case reGitNoHistory.MatchString(o.output): // git speaking from outside the tool, with the same standing as the // shell's 127. The session's repository is one kibble initialized @@ -301,11 +309,22 @@ func classifyLineResult(lr lineResult, l kplan.PlanLine, o lineOutcome, wrapped // uv's README pins a Python version inside a project an earlier // section created. The section was never tested as written. lr = inheritedResult(lr, o, tail) + case missingScriptPackage(lr.Cmd, o) != "": + // Node ended on a package the script on the command line imports and + // could not resolve. Nothing in the session installed it, and whether + // the container or an unwritten step owes it is not something the + // error says. Every rule above has had its turn, so this excuses only + // what would otherwise convict. A package the tool's own code imports + // and lacks is a different matter and still convicts below. + lr.Status = StatusBlocked + lr.Reason = ReasonMissingDependency + lr.Detail = fmt.Sprintf("runs a script that imports %s, which Node could not find, and "+ + "that may be the container or a missing step", missingScriptPackage(lr.Cmd, o)) default: lr.Status = StatusFail lr.Detail = fmt.Sprintf("exited %d: %s", o.code, tail) } - lr.output = o.output + lr.output, lr.cwd = o.output, o.cwd return lr } @@ -331,7 +350,8 @@ var ( `(?i)/dev/tty|not a (tty|terminal)|terminal is required|requires a terminal|no tty` + `|needs an interactive terminal|interactive terminal|must be run interactively` + `|not interactive|requires a controlling terminal|inappropriate ioctl for device` + - `|no such device or address`) + `|no such device or address|terminal entry not found|term not set` + + `|TERM (is )?(not set|unset|empty)`) // reOtherArch matches a binary built for an architecture the container // cannot run, such as an emulator that lacks the target's loader. reOtherArch = regexp.MustCompile( @@ -440,7 +460,7 @@ var ( `|\bnothing to search\b|\bno files found to search\b`) reNoData = regexp.MustCompile( `(?i)\bno (entries|results|matches|data|records)\b|\bfound no\b` + - `|\bnothing (found|to (show|report))\b|\bno \w+(\s\w+)? found\b` + + `|\bnothing (found|to (show|report))\b|\bno \w+(\s\w+){0,4} found\b` + `|\bno [a-z]+ (backups?|snapshots?|indexes|indices)\b`) // reEmptyInput matches a command that rejected the empty input the // session's stubbed editor produced. @@ -575,6 +595,14 @@ var rePathNotExist = regexp.MustCompile(`(?:^|\s)([\w./-]*/[\w.-]+) does not exi // "error: File not found: requirements.in". var reFileNotFound = regexp.MustCompile(`(?i)\bfile not found: '?"?([^\s'"]+)`) +// reEntryModule matches a bundler's sentence that the file it builds from is +// absent, as vite says "Cannot resolve entry module index.html." It also reads +// Rollup's "Could not resolve entry module". The name must carry an extension +// and is captured without the period that ends the sentence. Quotes, +// backticks, or the parentheses older Rollup puts around the name are dropped. +var reEntryModule = regexp.MustCompile(`(?i)\b(?:cannot|could not) resolve entry module ` + + `["'\x60(]?([\w@/.-]+?\.\w+)["'\x60)]?\.?(?:\s|$)`) + // rePyNoFile matches the line a Python traceback ends with when a file or a // program it tried to run was missing. Python names it after the message. var rePyNoFile = regexp.MustCompile( @@ -590,7 +618,9 @@ var reErrnoName = regexp.MustCompile(`^E[A-Z]+$`) // generic shape, which would otherwise capture ENOENT itself. func reportedNames(output string) []string { var out []string - shapes := []*regexp.Regexp{reNodeEnoent, reNotExist, rePathNotExist, reFileNotFound, reEnoent} + shapes := []*regexp.Regexp{ + reNodeEnoent, reNotExist, rePathNotExist, reFileNotFound, reEntryModule, reEnoent, + } for _, re := range shapes { m := re.FindStringSubmatch(output) if m == nil { @@ -628,6 +658,107 @@ func crashMissingName(output string) string { return "" } +// reNodeNoPackage matches Node's module loader reporting an import of a package +// no node_modules directory holds. The package and the importing file are +// captured. +var reNodeNoPackage = regexp.MustCompile( + `(?m)^Error \[ERR_MODULE_NOT_FOUND\]: Cannot find package '([^']+)' imported from (.+)$`) + +// reNodeErrorTail matches a line Node prints after an error's message as part +// of that same error: a blank line, a stack frame, the code or url property of +// the error object, a brace, or the closing version banner. A frame ends in a +// position or a parenthesis, so an indented sentence that starts with "at" is +// not one, and no other property is read, so a script's own indented +// "error: text" is not one either. +var reNodeErrorTail = regexp.MustCompile(`^(?:\s*|\s+at (?:.*(?:\)|:\d+)|)(?: \{)?|` + + `\s+(?:code|url): '[^']*',?|\s*[{}],?|Node\.js v\S+)$`) + +// reAddedExt matches the one extension a runner appends to the document it +// compiles, such as the .mjs zx adds to a Markdown file. +var reAddedExt = regexp.MustCompile(`^\.\w+$`) + +// missingScriptPackage returns the package a script the command passed tried +// to import and Node could not find, or empty when the outcome says anything +// else. The line must have exited 1 with its output kept whole. The importing +// file must be the path the command names, resolved from the directory the +// line ran in or one the line itself entered first, with at most one extension +// a runner added, and must sit outside node_modules. A file elsewhere that +// only ends the way the argument does is the tool's own code, and a package +// missing under it is the tool's defect. Every kept line after the loader +// error must be part of that error, since a script that catches a failed +// optional import and dies later died of something else. A script that +// catches the import, prints it, and exits 1 in silence reads the same as an +// uncaught one and is not told apart. +func missingScriptPackage(cmd string, o lineOutcome) string { + if o.code != 1 || o.truncated { + return "" + } + all := reNodeNoPackage.FindAllStringSubmatchIndex(o.output, -1) + if len(all) == 0 { + return "" + } + loc := all[len(all)-1] + for _, line := range strings.Split(o.output[loc[1]:], "\n") { + if !reNodeErrorTail.MatchString(strings.TrimRight(line, "\r")) { + return "" + } + } + pkg, importer := o.output[loc[2]:loc[3]], strings.TrimSpace(o.output[loc[4]:loc[5]]) + if strings.Contains(importer, "/node_modules/") { + return "" + } + dir := o.cwd + if dir == "" { + dir = sessionRepoDir + } + words := shell.ArgWordsOrFields(cmd) + for i := 0; i < len(words); i++ { + tok := strings.Trim(words[i], "'\"") + if (tok == "cd" || tok == "pushd") && i+1 < len(words) { + i++ + dir = enteredDir(dir, strings.Trim(words[i], "'\"")) + continue + } + if strings.HasPrefix(tok, "-") { + _, val, ok := strings.Cut(tok, "=") + if !ok { + continue + } + tok = val + } + name := normName(tok) + if name == "" || !strings.ContainsAny(tok, "./") { + continue + } + if !path.IsAbs(name) { + if dir == "" { + continue + } + name = path.Join(dir, name) + } + rest, found := strings.CutPrefix(importer, path.Clean(name)) + if found && (rest == "" || reAddedExt.MatchString(rest)) { + return pkg + } + } + return "" +} + +// enteredDir returns the directory a cd or pushd to target leaves a line in +// when it started in dir, or empty when the target is not a plain path and the +// result cannot be known from the line's words. +func enteredDir(dir, target string) string { + switch { + case target == "" || strings.ContainsAny(target, "$~`*?") || strings.HasPrefix(target, "-"): + return "" + case path.IsAbs(target): + return path.Clean(target) + case dir == "": + return "" + } + return path.Join(dir, target) +} + // crashWantsInput reports whether a crash's last line is an EOFError, which is // a program reading a prompt from a stdin that is closed. func crashWantsInput(output string) bool { diff --git a/lineverdict_missing_test.go b/lineverdict_missing_test.go index 2872182..b7152de 100644 --- a/lineverdict_missing_test.go +++ b/lineverdict_missing_test.go @@ -5,6 +5,8 @@ import ( "strings" "testing" + "github.com/google/go-cmp/cmp" + kplan "github.com/dcadolph/kibble/internal/plan" ) @@ -17,6 +19,17 @@ import ( func TestClassifyMissingNames(t *testing.T) { t.Parallel() + // entryModule is what vite printed in a sweep when the file it builds from + // was absent, color codes included, around the name it gives. + entryModule := func(name string) string { + const dist = "file:///root/kibble-npm/node_modules/" + return "transforming...\nBuild failed with 1 error:\n\n" + + "\x1b[31m[UNRESOLVED_ENTRY] \x1b[0mCannot resolve entry module " + name + ".\n\n" + + " at #build (" + dist + "rolldown/dist/shared/rolldown-AohEFLrh.mjs:146:34)\n" + + " at async CAC. (" + dist + "vite/dist/node/cli.js:789:3) {\n" + + " errors: [Getter/Setter]\n}\n" + } + tests := []struct { Creators map[string]string WantStatus Status @@ -261,6 +274,40 @@ FileNotFoundError: [Errno 2] No such file or directory: 'patfile' Cmd: "nve 18 node app.js", Output: "Error: Cannot find module 'node:path'\n", Code: 1, WantStatus: StatusFail, + }, { // Test 26: A bundler's entry file the command never named is blocked. + Cmd: "vite build", + Output: entryModule("index.html"), + Code: 1, WantStatus: StatusBlocked, WantReason: ReasonMissingFixture, + WantDetail: "reads index.html, which no documented step creates: ", + }, { // Test 27: An entry file the command passed is the document's gap. + Cmd: "vite build --ssr src/entry-server.js", + Output: entryModule("src/entry-server.js"), + Code: 1, WantStatus: StatusGap, + WantDetail: "references src/entry-server.js, which no documented step creates: ", + }, { // Test 28: A name with several dots is captured whole, without the period. + Cmd: "vite build", + Output: entryModule("vite.config.ts"), + Code: 1, WantStatus: StatusBlocked, WantReason: ReasonMissingFixture, + WantDetail: "reads vite.config.ts, which no documented step creates: ", + }, { // Test 29: A quoted entry path is read without its quotes. + Cmd: "vite build", + Output: entryModule(`"src/main.ts"`), + Code: 1, WantStatus: StatusBlocked, WantReason: ReasonMissingFixture, + WantDetail: "reads src/main.ts, which no documented step creates: ", + }, { // Test 30: An entry with no extension names no file, so the failure stands. + Cmd: "vite build", + Output: entryModule("main"), + Code: 1, WantStatus: StatusFail, + }, { // Test 31: Rollup's wording for the same absent entry file reads the same. + Cmd: "vite build", + Output: "error during build:\nCould not resolve entry module \"index.html\".\n", + Code: 1, WantStatus: StatusBlocked, WantReason: ReasonMissingFixture, + WantDetail: "reads index.html, which no documented step creates: ", + }, { // Test 32: Older Rollup wraps the name in parentheses, and it reads the same. + Cmd: "rollup src/main.js -f cjs", + Output: "[!] Error: Could not resolve entry module (src/main.js).\n", + Code: 1, WantStatus: StatusGap, + WantDetail: "references src/main.js, which no documented step creates: ", }} for testNum, test := range tests { @@ -279,3 +326,280 @@ FileNotFoundError: [Errno 2] No such file or directory: 'patfile' }) } } + +// TestClassifyMissingScriptPackage pins the rule that reads Node failing to +// find a package the script on the command line imports. The first output is +// what zx printed in a sweep. A package missing under a file the command never +// named, under installed code, or under a file elsewhere that only ends like +// the argument stays a failure, and so does a run that printed the loader +// error and then died of something else. +func TestClassifyMissingScriptPackage(t *testing.T) { + t.Parallel() + + const ( + stack = " at packageResolve (node:internal/modules/esm/resolve:772:81)\n" + + " at ModuleLoader.import (node:internal/modules/esm/loader:679:21) {\n" + + " code: 'ERR_MODULE_NOT_FOUND'\n}\n" + typeError = "TypeError: Cannot read properties of undefined (reading 'port')\n" + + " at file:///work/repo/opt.mjs:3:1\n\nNode.js v26.13.0\n" + ) + noPackage := func(pkg, importer string) string { + return "Error [ERR_MODULE_NOT_FOUND]: Cannot find package '" + pkg + + "' imported from " + importer + "\n" + stack + } + + tests := []struct { + WantStatus Status + WantReason Reason + WantDetail string + Cmd string + Cwd string + Output string + Code int + Truncated bool + }{{ // Test 0: A script a runner compiled from the named document is blocked. + Cmd: "zx docs/markdown.md", + Cwd: "/work/repo", + Output: noPackage("chalk", "/work/repo/docs/markdown.md.mjs"), + WantStatus: StatusBlocked, WantReason: ReasonMissingDependency, + WantDetail: "imports chalk, which Node could not find", + }, { // Test 1: A script named outright is blocked, with no directory on the marker. + Cmd: "node app.js", + Output: noPackage("express", "/work/repo/app.js"), + WantStatus: StatusBlocked, WantReason: ReasonMissingDependency, + WantDetail: "imports express, which Node could not find", + }, { // Test 2: A leading ./ on the named script does not hide it. + Cmd: "node ./src/app.mjs --port 8080", + Cwd: "/work/repo", + Output: noPackage("@scope/pkg", "/work/repo/src/app.mjs"), + WantStatus: StatusBlocked, WantReason: ReasonMissingDependency, + WantDetail: "imports @scope/pkg, which Node could not find", + }, { // Test 3: A package the tool's installed code lacks is the tool's defect. + Cmd: "tool build site.config.js", + Output: noPackage("chalk", "/usr/local/lib/node_modules/tool/dist/index.js"), + WantStatus: StatusFail, + }, { // Test 4: A named script under node_modules is installed code all the same. + Cmd: "node node_modules/tool/cli.js", + Output: noPackage("chalk", "/work/repo/node_modules/tool/cli.js"), + WantStatus: StatusFail, + }, { // Test 5: A file the command never named settles nothing in the line's favor. + Cmd: "node app.js", + Output: noPackage("chalk", "/work/repo/lib/util.js"), + WantStatus: StatusFail, + }, { // Test 6: A name that only ends like the named script is another file. + Cmd: "node app.js", + Output: noPackage("chalk", "/work/repo/myapp.js"), + WantStatus: StatusFail, + }, { // Test 7: A file inside a directory named like the script is another file. + Cmd: "zx docs/markdown.md", + Output: noPackage("chalk", "/work/repo/docs/markdown.md.d/run.mjs"), + WantStatus: StatusFail, + }, { // Test 8: A command naming no script has no importer to match. + Cmd: "tool serve", + Output: noPackage("chalk", "/work/repo/serve.js"), + WantStatus: StatusFail, + }, { // Test 9: A crash before the loader error still convicts. + Cmd: "node app.js", + Output: "fatal error: out of memory\n" + noPackage("chalk", "/work/repo/app.js"), + WantStatus: StatusFail, WantDetail: "crashed with exit 1", + }, { // Test 10: A workspace tool's own source ends like the argument and is not it. + Cmd: "tool build src/index.js", + Cwd: "/work/repo", + Output: noPackage("chalk", "/work/repo/packages/tool/src/index.js"), + WantStatus: StatusFail, + }, { // Test 11: The same holds for a tool installed outside node_modules. + Cmd: "tool lint index.js", + Cwd: "/work/repo", + Output: noPackage("chalk", "/opt/tool/index.js"), + WantStatus: StatusFail, + }, { // Test 12: A caught optional import followed by the real error convicts. + Cmd: "node opt.mjs", + Cwd: "/work/repo", + Output: noPackage("fsevents", "/work/repo/opt.mjs") + typeError, + WantStatus: StatusFail, + }, { // Test 13: A caught optional import does not hide the file the document never made. + Cmd: "node opt2.mjs data.json", + Cwd: "/work/repo", + Output: noPackage("fsevents", "/work/repo/opt2.mjs") + + "Error: ENOENT: no such file or directory, open 'data.json'\n", + WantStatus: StatusGap, WantDetail: "references data.json", + }, { // Test 14: Of two scripts, the one that ended the line decides. + Cmd: "node a.mjs; node b.mjs", + Cwd: "/work/repo", + Output: noPackage("chalk", "/work/repo/a.mjs") + typeError, + WantStatus: StatusFail, + }, { // Test 15: A script named through the parent directory is resolved first. + Cmd: "node ../app.mjs", + Cwd: "/work/repo/sub", + Output: noPackage("chalk", "/work/repo/app.mjs"), + WantStatus: StatusBlocked, WantReason: ReasonMissingDependency, + }, { // Test 16: A directory named like the script does not hide the script inside it. + Cmd: "node demo.mjs", + Cwd: "/work/repo/demo.mjs.org", + Output: noPackage("chalk", "/work/repo/demo.mjs.org/demo.mjs"), + WantStatus: StatusBlocked, WantReason: ReasonMissingDependency, + }, { // Test 17: A script passed as a flag's value is named all the same. + Cmd: "tool --config=app.mjs", + Cwd: "/work/repo", + Output: noPackage("chalk", "/work/repo/app.mjs"), + WantStatus: StatusBlocked, WantReason: ReasonMissingDependency, + }, { // Test 18: A script named by its full path is named. + Cmd: "node /work/repo/src/./app.js", + Cwd: "/tmp", + Output: noPackage("chalk", "/work/repo/src/app.js"), + WantStatus: StatusBlocked, WantReason: ReasonMissingDependency, + }, { // Test 19: Two added extensions make another file, such as a test beside the script. + Cmd: "tool src/app", + Cwd: "/work/repo", + Output: noPackage("chalk", "/work/repo/src/app.test.js"), + WantStatus: StatusFail, + }, { // Test 20: A process a signal killed did not end on the loader error. + Cmd: "node app.js", + Cwd: "/work/repo", + Output: noPackage("chalk", "/work/repo/app.js"), + Code: 137, + WantStatus: StatusFail, + }, { // Test 21: An uncaught loader error carries Node's header and version banner. + Cmd: "node app.mjs", + Cwd: "/work/repo", + Output: "node:internal/modules/esm/resolve:873\n" + + " throw new ERR_MODULE_NOT_FOUND(packageName, fileURLToPath(base), null);\n" + + " ^\n\n" + noPackage("chalk", "/work/repo/app.mjs") + "\nNode.js v22.11.0\n", + WantStatus: StatusBlocked, WantReason: ReasonMissingDependency, + }, { // Test 22: A program the tool could not start is still the container's gap. + Cmd: "node run.mjs", + Cwd: "/work/repo", + Output: "Error: spawn ava ENOENT\n" + noPackage("chalk", "/work/repo/run.mjs"), + WantStatus: StatusSkipped, WantReason: ReasonMissingDependency, + }, { // Test 23: The directory the line ran in decides which file a relative name means. + Cmd: "node app.js", + Cwd: "/work/repo/sub", + Output: noPackage("chalk", "/work/repo/app.js"), + WantStatus: StatusFail, + }, { // Test 24: A directory the line itself enters first is where its script is. + Cmd: "cd examples && node demo.mjs", + Cwd: "/work/repo", + Output: noPackage("chalk", "/work/repo/examples/demo.mjs"), + WantStatus: StatusBlocked, WantReason: ReasonMissingDependency, + }, { // Test 25: A pushd and a runner in front of the script read the same way. + Cmd: "pushd examples && npx tsx src/main.ts", + Cwd: "/work/repo", + Output: noPackage("chalk", "/work/repo/examples/src/main.ts"), + WantStatus: StatusBlocked, WantReason: ReasonMissingDependency, + }, { // Test 26: After the line changes directory the old one no longer names the script. + Cmd: "cd examples && node demo.mjs", + Cwd: "/work/repo", + Output: noPackage("chalk", "/work/repo/demo.mjs"), + WantStatus: StatusFail, + }, { // Test 27: A directory only a variable names leaves a relative script unplaced. + Cmd: "cd \"$DEMO_DIR\" && node demo.mjs", + Cwd: "/work/repo", + Output: noPackage("chalk", "/work/repo/demo.mjs"), + WantStatus: StatusFail, + }, { // Test 28: An indented message of the script's own after the stack is not Node's. + Cmd: "node opt.mjs", + Cwd: "/work/repo", + Output: noPackage("fsevents", "/work/repo/opt.mjs") + " error: port must be a number\n", + WantStatus: StatusFail, + }, { // Test 29: An indented sentence that starts with "at" is not a stack frame. + Cmd: "node opt.mjs", + Cwd: "/work/repo", + Output: noPackage("fsevents", "/work/repo/opt.mjs") + " at least one entry is required\n", + WantStatus: StatusFail, + }, { // Test 30: Output the session cut short cannot show what the line ended on. + Cmd: "node app.js", + Cwd: "/work/repo", + Output: noPackage("chalk", "/work/repo/app.js"), + Truncated: true, + WantStatus: StatusFail, + }, { // Test 31: A script named by ./ and no extension is still a path. + Cmd: "node ./server", + Cwd: "/work/repo", + Output: noPackage("express", "/work/repo/server.js"), + WantStatus: StatusBlocked, WantReason: ReasonMissingDependency, + }, { // Test 32: A bare frame, a url property, and a pre-release banner are Node's own. + Cmd: "node app.mjs", + Cwd: "/work/repo", + Output: "Error [ERR_MODULE_NOT_FOUND]: Cannot find package 'chalk' imported from " + + "/work/repo/app.mjs\n at packageResolve (node:internal/modules/esm/resolve:772:81)\n" + + " at async file:///work/repo/app.mjs:1:1 {\n code: 'ERR_MODULE_NOT_FOUND',\n" + + " url: 'file:///work/repo/node_modules/chalk'\n}\n\nNode.js v23.0.0-pre\n", + WantStatus: StatusBlocked, WantReason: ReasonMissingDependency, + }} + + for testNum, test := range tests { + t.Run(fmt.Sprintf("test %d", testNum), func(t *testing.T) { + t.Parallel() + code := 1 + if test.Code != 0 { + code = test.Code + } + lr := classifyLineResult(lineResult{Cmd: test.Cmd}, kplan.PlanLine{}, + lineOutcome{code: code, cwd: test.Cwd, output: test.Output, truncated: test.Truncated}, + false, nil, lineTimeout) + if lr.Status != test.WantStatus || lr.Reason != test.WantReason { + t.Errorf("verdict = %s %q, want %s %q (detail %q)", + lr.Status, lr.Reason, test.WantStatus, test.WantReason, lr.Detail) + } + if !strings.Contains(lr.Detail, test.WantDetail) { + t.Errorf("detail = %q, want it to contain %q", lr.Detail, test.WantDetail) + } + }) + } +} + +// TestClassifyExampleLongOutput pins that a line whose output passed the +// session's cap is not read as ending where the kept lines end. A script that +// printed a caught loader error and then died of something else past the cap +// stays a failure, and the same loader error inside the cap is still blocked. +func TestClassifyExampleLongOutput(t *testing.T) { + t.Parallel() + + const ( + loader = "Error [ERR_MODULE_NOT_FOUND]: Cannot find package 'chalk' imported from " + + "/work/repo/build.mjs\n" + + " at packageResolve (node:internal/modules/esm/resolve:772:81)\n" + + " at ModuleLoader.import (node:internal/modules/esm/loader:679:21) {\n" + + " code: 'ERR_MODULE_NOT_FOUND'\n}\n" + typeError = "TypeError: Cannot read properties of undefined (reading 'port')\n" + + " at file:///work/repo/build.mjs:9:1\n\nNode.js v22.11.0\n" + ) + tests := []struct { + WantStatus Status + Progress int + After string + }{{ // Test 0: A loader error that ends short output is blocked. + Progress: 20, WantStatus: StatusBlocked, + }, { // Test 1: A later error inside the cap is read and convicts. + Progress: 20, After: typeError, WantStatus: StatusFail, + }, { // Test 2: A later error the cap dropped convicts all the same. + Progress: 195, After: typeError, WantStatus: StatusFail, + }, { // Test 3: Output that fills the cap exactly was not cut. + Progress: 195, WantStatus: StatusBlocked, + }, { // Test 4: One kept line past the cap was cut, whatever it said. + Progress: 195, After: "done\n", WantStatus: StatusFail, + }} + for testNum, test := range tests { + t.Run(fmt.Sprintf("test %d", testNum), func(t *testing.T) { + t.Parallel() + plan := &kplan.Plan{ + Repo: "repo", Binaries: []string{"zx"}, + Steps: []kplan.PlanStep{{ID: "b1", Lines: []kplan.PlanLine{{Cmd: "zx build.mjs"}}}}, + } + out := "KIBBLE-BUILD CODE=0\nKIBBLE-STEP b1 START\n" + + strings.Repeat("built one more page\n", test.Progress) + loader + test.After + + "KIBBLE-LINE b1:0 CODE=1 CWD=/work/repo\nKIBBLE-DONE\n" + res := classifyExample(InstallStep{Repo: "repo", Kind: "example"}, plan, out, nil, 0, + lineTimeout, sessionEnd{}) + var got []Status + for _, s := range res.example.Steps { + for _, l := range s.Lines { + got = append(got, l.Status) + } + } + if diff := cmp.Diff([]Status{test.WantStatus}, got); diff != "" { + t.Errorf("line statuses mismatch (-want +got):\n%s\ndetail %q", diff, res.Detail) + } + }) + } +} diff --git a/lineverdict_tty_test.go b/lineverdict_tty_test.go index a02bd24..951ea6b 100644 --- a/lineverdict_tty_test.go +++ b/lineverdict_tty_test.go @@ -36,6 +36,9 @@ func TestTerminalErrorIsNotAFailure(t *testing.T) { }, { // Test 4: an ordinary failure is untouched. Name: "unrelated failure", Output: "error: no such option --heigth", Code: 2, WantStatus: StatusFail, + }, { // Test 5: tcell names the unset TERM a container leaves behind. + Name: "tcell term not set", Output: "ERROR terminal entry not found: term not set", Code: 2, + WantStatus: StatusSkipped, WantReason: ReasonInteractive, }} for testNum, test := range tests { diff --git a/main.go b/main.go index 029301c..8c2740d 100644 --- a/main.go +++ b/main.go @@ -227,7 +227,6 @@ func kibbleVersion() string { // a result per repository kibble could not read, so a path with no README and // a malformed config both reach the report instead of passing as silence. func collect(paths []string, examples bool) ([]InstallStep, []*kplan.Plan, []Result) { - ex := DefaultExtractor() var out []InstallStep var plans []*kplan.Plan var problems []Result @@ -242,6 +241,8 @@ func collect(paths []string, examples bool) ([]InstallStep, []*kplan.Plan, []Res }) continue } + owned := ownedNames(p, repo) + ex := ExtractorWith(owned) steps := ex.Extract(repo, md) // Install instructions outlive the README they started in: a docs tree // and an INSTALL file are where they go once the front page fills up, @@ -251,7 +252,7 @@ func collect(paths []string, examples bool) ([]InstallStep, []*kplan.Plan, []Res // verified rather than reported as having nothing to install. installSeen := map[string]bool{} for i := range steps { - installSeen[steps[i].Kind+"|"+steps[i].Module] = true + installSeen[installKey(steps[i])] = true } idocs := installDocs(p, name) for _, doc := range idocs { @@ -263,7 +264,7 @@ func collect(paths []string, examples bool) ([]InstallStep, []*kplan.Plan, []Res continue } for _, st := range ex.Extract(repo, string(body)) { - key := st.Kind + "|" + st.Module + key := installKey(st) if installSeen[key] { continue } @@ -276,7 +277,6 @@ func collect(paths []string, examples bool) ([]InstallStep, []*kplan.Plan, []Res // not a route to it. Kept as an install, it became the tool under test, // and uv's report described ruff. Package installs still run as example // lines, since the planner only leaves owned installs to the installer. - owned := ownedNames(p, repo) seen := installRoutes(steps) steps = owned.keep(steps) own := slices.DeleteFunc(slices.Clone(steps), func(s InstallStep) bool { return !owned.owns(s) }) @@ -430,6 +430,7 @@ func sessionInstalls(dir string, steps []InstallStep) ([]string, []kplan.PlanIns var bins []string var all []kplan.PlanInstall var clone *kplan.PlanInstall + npmSeen := map[string]bool{} for _, s := range steps { // A step recorded but not run, such as a clone of a placeholder URL, // must not become the session's install either. @@ -446,6 +447,21 @@ func sessionInstalls(dir string, steps []InstallStep) ([]string, []kplan.PlanIns case s.Kind == "go-install": in = kplan.PlanInstall{Cmd: "go install " + s.Module, Ecosystem: "go", Binary: s.Binary} case pkgKinds[s.Kind].Ecosystem != "": + // A package whose manifest declares no binary puts no tool on PATH, + // so its install is verified as a step and opens no session. + if libraryInstall(s) { + continue + } + // A local and a global install of one node package are two routes + // to one tool. The package name decides, since the binary a + // manifest declares need not be the one guessed for a global line. + if s.Kind == "npm-install" { + name := npmPackageName(s.Module) + if npmSeen[name] { + continue + } + npmSeen[name] = true + } pk := pkgKinds[s.Kind] in = kplan.PlanInstall{ Cmd: shellCommand(s.Raw), Ecosystem: pk.Ecosystem, @@ -596,20 +612,7 @@ var readmeFallbackDirs = []string{"docs", "doc"} // filesystem answers to README.md for a file named readme.md and would have // kibble annotate a path that does not exist on a case-sensitive host. func readREADME(dir string) (string, string, error) { - entries, err := os.ReadDir(dir) - if err != nil { - return "", "", err - } - present := map[string]bool{} - for _, e := range entries { - if !e.IsDir() { - present[e.Name()] = true - } - } - for _, name := range readmeNames { - if !present[name] { - continue - } + if name, ok := findReadmeName(dir); ok { b, err := os.ReadFile(filepath.Join(dir, name)) if err != nil { return "", "", err @@ -620,31 +623,35 @@ func readREADME(dir string) (string, string, error) { // case-insensitive filesystem cannot make kibble name a path that a // case-sensitive host does not have. for _, sub := range readmeFallbackDirs { - subEntries, err := os.ReadDir(filepath.Join(dir, sub)) - if err != nil { + name, ok := findReadmeName(filepath.Join(dir, sub)) + if !ok { continue } - subPresent := map[string]bool{} - for _, e := range subEntries { - if !e.IsDir() { - subPresent[e.Name()] = true - } - } - for _, name := range readmeNames { - if !subPresent[name] { - continue - } - rel := filepath.Join(sub, name) - b, err := os.ReadFile(filepath.Join(dir, rel)) - if err != nil { - return "", "", err - } - return string(b), filepath.ToSlash(rel), nil + rel := filepath.Join(sub, name) + b, err := os.ReadFile(filepath.Join(dir, rel)) + if err != nil { + return "", "", err } + return string(b), filepath.ToSlash(rel), nil } return "", "", fmt.Errorf("no README found") } +// findReadmeName lists dir and returns the real name of the first file that +// matches readme.md without regard to case. +func findReadmeName(dir string) (string, bool) { + entries, err := os.ReadDir(dir) + if err != nil { + return "", false + } + for _, e := range entries { + if !e.IsDir() && strings.EqualFold(e.Name(), "readme.md") { + return e.Name(), true + } + } + return "", false +} + // runAll executes steps with bounded concurrency and returns their results. // A canceled run stops handing out work, so an interrupt does not queue more // containers behind the ones already being torn down. @@ -737,7 +744,8 @@ func checkFreeSpace(ctx context.Context, r *DockerRunner, steps []InstallStep) e if i < 0 { return nil } - free, err := sandbox.FreeBytes(ctx, r.imageFor(steps[i])) + img, _ := r.imageFor(steps[i]) + free, err := sandbox.FreeBytes(ctx, img) if err != nil { fmt.Fprintf(os.Stderr, "kibble could not measure free space in Docker: %v\n", err) return nil diff --git a/main_test.go b/main_test.go index b393306..28fd8f0 100644 --- a/main_test.go +++ b/main_test.go @@ -400,3 +400,51 @@ func TestSessionInstallsClone(t *testing.T) { }) } } + +// TestReadREADMENameCase checks that a README spelled in any case is found and +// reported under its real name, at the root and in a fallback directory. +func TestReadREADMENameCase(t *testing.T) { + t.Parallel() + + tests := []struct { + Name string + File string + WantName string + }{{ // Test 0: a title-case name resolves. + Name: "title case", + File: "Readme.md", + WantName: "Readme.md", + }, { // Test 1: an upper-case extension resolves. + Name: "upper extension", + File: "README.MD", + WantName: "README.MD", + }, { // Test 2: a fallback directory honors the same rule. + Name: "docs title case", + File: "docs/Readme.md", + WantName: "docs/Readme.md", + }} + + for testNum, test := range tests { + t.Run(fmt.Sprintf("test %d %s", testNum, test.Name), func(t *testing.T) { + t.Parallel() + dir := t.TempDir() + p := filepath.Join(dir, test.File) + if err := os.MkdirAll(filepath.Dir(p), 0o755); err != nil { + t.Fatalf("mkdir: %v", err) + } + if err := os.WriteFile(p, []byte("body"), 0o644); err != nil { + t.Fatalf("write: %v", err) + } + body, name, err := readREADME(dir) + if err != nil { + t.Fatalf("readREADME: %v", err) + } + if diff := cmp.Diff(test.WantName, name); diff != "" { + t.Errorf("name mismatch (-want +got):\n%s", diff) + } + if body != "body" { + t.Errorf("body = %q, want %q", body, "body") + } + }) + } +} diff --git a/npmfail_test.go b/npmfail_test.go index 1909cc1..1d8852a 100644 --- a/npmfail_test.go +++ b/npmfail_test.go @@ -1,8 +1,11 @@ package main import ( + "fmt" "strings" "testing" + + kplan "github.com/dcadolph/kibble/internal/plan" ) // TestNpmFailureLine pins which line of an npm failure a reader is shown. npm @@ -51,3 +54,59 @@ npm error A complete log of this run can be found in: /root/.npm/_logs/x-debug-0 t.Errorf("fail line = %q, want the notsup sentence", got) } } + +// TestNpmLocalInstall checks that a local npm install of a named package runs +// in a scratch project, where its binaries are looked for, while a global or +// bare install is left as documented. zx's README says `npm install zx`, and +// run in zx's own checkout it failed with `Tracker "idealTree" already +// exists`. +func TestNpmLocalInstall(t *testing.T) { + t.Parallel() + tests := []struct { + Raw string + WantLocal bool + }{{ // Test 0: Zx's documented local install. + Raw: "npm install zx", WantLocal: true, + }, { // Test 1: The short form with a dev flag is local too. + Raw: "npm i -D chalk", WantLocal: true, + }, { // Test 2: A global install is unchanged. + Raw: "npm install -g zx", WantLocal: false, + }, { // Test 3: The long global flag is unchanged. + Raw: "npm install --global zx", WantLocal: false, + }, { // Test 4: A bare install is the checkout's own dependencies. + Raw: "npm install", WantLocal: false, + }, { // Test 5: A compound line is not rewritten. + Raw: "npm install zx && zx --version", WantLocal: false, + }} + for testNum, test := range tests { + t.Run(fmt.Sprintf("test %d", testNum), func(t *testing.T) { + t.Parallel() + script := pkgScriptFor(InstallStep{Kind: "npm-install", Raw: test.Raw, Module: "zx", Binary: "zx"}, 60) + hasInit := strings.Contains(script, "npm init -y") + hasBin := strings.Contains(script, `BINDIR="$HOME/kibble-npm/node_modules/.bin"`) + if hasInit != test.WantLocal || hasBin != test.WantLocal { + t.Errorf("npm init %v, local bin dir %v, want both %v:\n%s", hasInit, hasBin, test.WantLocal, script) + } + if !test.WantLocal && !strings.Contains(script, `BINDIR="$(npm prefix -g)/bin"`) { + t.Errorf("non-local install lost the global bin dir:\n%s", script) + } + }) + } +} + +// TestSessionNpmLocalInstall checks that the example session runs a local npm +// install in the scratch project and can find its binary on PATH. +func TestSessionNpmLocalInstall(t *testing.T) { + t.Parallel() + plan := &kplan.Plan{ + Installs: []kplan.PlanInstall{{Cmd: "npm install zx", Ecosystem: "node"}}, + Binaries: []string{"zx"}, + Steps: []kplan.PlanStep{{ID: "b1", Lines: []kplan.PlanLine{{Cmd: "zx --version"}}}}, + } + script, _ := sessionScript(plan, 60, 30) + for _, want := range []string{"npm init -y", `cd "$HOME/kibble-npm"`, ":$HOME/kibble-npm/node_modules/.bin"} { + if !strings.Contains(script, want) { + t.Errorf("session script lacks %q", want) + } + } +} diff --git a/ownership.go b/ownership.go index ad9ffa7..f2ff423 100644 --- a/ownership.go +++ b/ownership.go @@ -29,6 +29,10 @@ type repoNames struct { // declared reports whether any manifest named anything. Without one, // ownership is unknown rather than absent, and every install is kept. declared bool + // npm maps each node package name a manifest declares, exactly as written, + // to the binaries that package declares. A local npm install is a route to + // the tool only for a name found here. + npm map[string][]string } // manifestMaxDepth bounds how deep below the root ownedNames looks for @@ -51,6 +55,11 @@ var ( reGoModule = regexp.MustCompile(`(?m)^module\s+"?([^\s"]+)"?`) // reMajorSuffix matches a module major-version path element such as /v2. reMajorSuffix = regexp.MustCompile(`/v\d+(/|$)`) + // reVersionSuffix matches the version or tag that ends a package request. + // An alias such as name@npm:other is not one. + reVersionSuffix = regexp.MustCompile(`@[\w.^~*-]+$`) + // reBinName matches a binary name made of plain word characters. + reBinName = regexp.MustCompile(`^\w[\w.+-]*$`) ) // ownedNames reads the manifests under dir and returns the names the @@ -137,23 +146,79 @@ func (rn *repoNames) declare(name string) { } // addPackageJSON records a node package's name and the binaries it provides. +// A private package still names the repository, and it stays out of the npm +// map: npm refuses to publish it, so an install of that name from the registry +// fetches someone else's package or nothing. A binary name that is not a plain +// word is dropped, since no install could put it on PATH under that name. func (rn *repoNames) addPackageJSON(body []byte) { var pkg struct { // Name is the package name, scope included. Name string `json:"name"` // Bin is a single path or a map of binary name to path. Bin json.RawMessage `json:"bin"` + // Private is any truthy value for a package npm refuses to publish. + Private json.RawMessage `json:"private"` } if json.Unmarshal(body, &pkg) != nil { return } rn.declare(pkg.Name) var bins map[string]string - if json.Unmarshal(pkg.Bin, &bins) == nil { + var one string + var provided []string + switch { + case json.Unmarshal(pkg.Bin, &bins) == nil: for b := range bins { + if !reBinName.MatchString(b) { + continue + } rn.declare(b) + provided = append(provided, b) + } + case json.Unmarshal(pkg.Bin, &one) == nil && one != "": + // A single path installs one binary named after the package. + if b := packageBinary(pkg.Name); reBinName.MatchString(b) { + provided = append(provided, b) + } + } + if pkg.Name == "" || npmPrivate(pkg.Private) { + return + } + if rn.npm == nil { + rn.npm = map[string][]string{} + } + for _, b := range provided { + if !slices.Contains(rn.npm[pkg.Name], b) { + rn.npm[pkg.Name] = append(rn.npm[pkg.Name], b) } } + rn.npm[pkg.Name] = slices.Sorted(slices.Values(rn.npm[pkg.Name])) +} + +// npmPrivate reports whether a manifest's private value keeps its package off +// the registry. npm refuses on any truthy value, so only a missing value and +// the JSON values that read as false leave the package published. +func npmPrivate(raw json.RawMessage) bool { + switch strings.TrimSpace(string(raw)) { + case "", "false", "null", "0", `""`: + return false + } + return true +} + +// npmBinary reports whether a manifest declares a node package of exactly +// this name, and names the binary to expect from it. The binary named after +// the package is preferred, then the first by name, and it is empty for a +// package that declares none, as a library does. +func (rn repoNames) npmBinary(name string) (string, bool) { + bins, ok := rn.npm[name] + if !ok || len(bins) == 0 { + return "", ok + } + if want := packageBinary(name); slices.Contains(bins, want) { + return want, true + } + return bins[0], true } // addSections records names from an INI-shaped manifest. keys maps a section @@ -216,11 +281,16 @@ func (rn repoNames) owns(s InstallStep) bool { } // packageCandidates returns the names a package argument could match: the -// whole name and its bare form for a registry package, and every path element -// for a URL, so git+https://github.com/psf/black.git@main offers black. +// whole name, its bare form, and the name without a version or tag for a +// registry package, so @scope/tool@2 offers @scope/tool, and every path element for a +// URL, so git+https://github.com/psf/black.git@main offers black. func packageCandidates(pkg string) []string { if !strings.Contains(pkg, "://") && !strings.HasPrefix(pkg, "git+") { - return []string{pkg, packageBinary(pkg)} + out := []string{pkg, packageBinary(pkg)} + if loc := reVersionSuffix.FindStringIndex(pkg); loc != nil && loc[0] > 0 { + out = append(out, pkg[:loc[0]]) + } + return out } u := strings.TrimPrefix(pkg, "git+") if _, after, ok := strings.Cut(u, "://"); ok { @@ -256,10 +326,12 @@ func stripMajor(mod string) string { // in a Go repository. Otherwise it is only a name kibble did not recognize: // `cargo install ripgreppp` is the one cargo route ripgrep's README offers, // and whether that crate exists is the registry's to say when the line runs. +// A library install puts no tool in hand, so `npm install -g express-generator` +// stays beside `npm install express`. func (rn repoNames) keep(steps []InstallStep) []InstallStep { known := map[string]bool{} for _, s := range steps { - if s.Run && rn.owns(s) { + if s.Run && rn.owns(s) && !libraryInstall(s) { known[stepEcosystem(s)] = true } } @@ -272,6 +344,12 @@ func (rn repoNames) keep(steps []InstallStep) []InstallStep { }) } +// libraryInstall reports whether a step installs a node package whose manifest +// declares no binary, which verifies the install line and provides no tool. +func libraryInstall(s InstallStep) bool { + return s.Kind == "npm-install" && s.Binary == "" +} + // stepEcosystem is the ecosystem an install step runs in, empty for a kind // that is not a package install. func stepEcosystem(s InstallStep) string { diff --git a/ownership_test.go b/ownership_test.go index e458e40..338f532 100644 --- a/ownership_test.go +++ b/ownership_test.go @@ -125,6 +125,37 @@ func TestCollectOwnedInstalls(t *testing.T) { WantInstalls: []string{"pip-install mytool"}, WantSessionCmds: []string{"pip install mytool"}, WantSessionBins: []string{"mytool"}, + }, { // Test 10: A local install of the repository's own package is its route, + // and the session expects the binary the manifest declares. + Name: "zx", + Files: map[string]string{ + "package.json": `{"name": "zx", "bin": {"zx": "build/cli.js"}}`, + "README.md": "# zx\n\n```bash\nnpm install zx # add -g to install globally\n```\n\n" + + "```bash\nzx --version\n```\n", + }, + WantInstalls: []string{"npm-install zx"}, + WantSessionCmds: []string{"npm install zx"}, + WantSessionBins: []string{"zx"}, + }, { // Test 11: A single-path bin names its binary after the scoped package. + Name: "tool-repo", + Files: map[string]string{ + "package.json": `{"name": "@scope/tool", "bin": "cli.js"}`, + "README.md": "# tool\n\n```sh\nnpm i @scope/tool\n```\n\n```sh\ntool --version\n```\n", + }, + WantInstalls: []string{"npm-install @scope/tool"}, + WantSessionCmds: []string{"npm i @scope/tool"}, + WantSessionBins: []string{"tool", "tool-repo"}, + }, { // Test 12: A workspace member's package is declared, and the private root is not it. + Name: "mono", + Files: map[string]string{ + "package.json": `{"name": "@org/monorepo", "private": true}`, + "packages/tool/package.json": `{"name": "tool", "bin": {"tool": "bin/tool.js"}}`, + "README.md": "# tool\n\n```sh\nnpm install tool\n```\n\n" + + "```sh\ntool --help\n```\n", + }, + WantInstalls: []string{"npm-install tool"}, + WantSessionCmds: []string{"npm install tool"}, + WantSessionBins: []string{"tool", "mono"}, }} for testNum, test := range tests { t.Run(fmt.Sprintf("test %d", testNum), func(t *testing.T) { @@ -167,6 +198,82 @@ func TestCollectOwnedInstalls(t *testing.T) { } } +// TestCollectLocalNpmLibrary checks the routes a node library's README gives. +// A local install of the declared package is verified as a step and opens no +// session, since a package with no binary puts no tool on PATH. A local +// install of anything else is a dependency install and no route at all. +func TestCollectLocalNpmLibrary(t *testing.T) { + t.Parallel() + tests := []struct { + Name string + Manifest string + Readme string + WantInstalls []string + WantSession []string + WantNoRoute bool + }{{ // Test 0: Chalk's README installs chalk, which declares no binary. + Name: "chalk", Manifest: `{"name": "chalk"}`, + Readme: "# chalk\n\n```sh\nnpm install chalk\n```\n\n```sh\nnode example.js\n```\n", + WantInstalls: []string{"npm-install chalk"}, + }, { // Test 1: A second package on the line rides along with the declared one. + Name: "ink", Manifest: `{"name": "ink"}`, + Readme: "# ink\n\n```sh\nnpm install ink react\n```\n", + WantInstalls: []string{"npm-install ink"}, + }, { // Test 2: A dependency install in another package's README is no route. + Name: "widget", Manifest: `{"name": "widget"}`, + Readme: "# widget\n\n```sh\nnpm install prettier\n```\n", + WantNoRoute: true, + }, { // Test 3: The folder name is not a declaration when the manifest names another package. + Name: "chalk", Manifest: `{"name": "@fork/chalk-next"}`, + Readme: "# chalk\n\n```sh\nnpm install chalk\n```\n", + WantNoRoute: true, + }, { // Test 4: A library keeps the clone session and the tool install beside it. + Name: "express", Manifest: `{"name": "express"}`, + Readme: "# express\n\n```sh\nnpm install express\n```\n\n" + + "```sh\nnpm install -g express-generator@4\n```\n\n" + + "```sh\ngit clone https://github.com/expressjs/express.git\ncd express\nnpm install\n```\n\n" + + "```sh\nexpress --version\n```\n", + WantInstalls: []string{"npm-install express", "npm-install express-generator@4"}, + WantSession: []string{ + "git clone https://github.com/expressjs/express.git\ncd express\nnpm install", + }, + }} + for testNum, test := range tests { + t.Run(fmt.Sprintf("test %d", testNum), func(t *testing.T) { + t.Parallel() + dir := filepath.Join(t.TempDir(), test.Name) + if err := os.MkdirAll(dir, 0o750); err != nil { + t.Fatalf("mkdir: %v", err) + } + writeRepoFile(t, dir, "package.json", test.Manifest) + writeRepoFile(t, dir, "README.md", test.Readme) + steps, plans, problems := collect([]string{dir}, true) + var installs []string + for _, s := range steps { + if s.Run && pkgKinds[s.Kind].Ecosystem != "" { + installs = append(installs, s.Kind+" "+s.Module) + } + } + if diff := cmp.Diff(test.WantInstalls, installs, cmpopts.EquateEmpty()); diff != "" { + t.Errorf("install steps mismatch (-want +got):\n%s", diff) + } + var session []string + for _, p := range plans { + for _, in := range p.Installs { + session = append(session, in.Cmd) + } + } + if diff := cmp.Diff(test.WantSession, session, cmpopts.EquateEmpty()); diff != "" { + t.Errorf("session installs mismatch (-want +got):\n%s", diff) + } + noRoute := len(problems) == 1 && problems[0].Reason == ReasonNoInstallRoute + if noRoute != test.WantNoRoute || len(problems) > 1 { + t.Errorf("no-install-route row = %v, want %v: %+v", noRoute, test.WantNoRoute, problems) + } + }) + } +} + // TestRepoNamesKeep checks which unrecognized installs are dropped as foreign // tools and which are kept for the registry to answer. func TestRepoNamesKeep(t *testing.T) { @@ -224,3 +331,201 @@ func TestRepoNamesKeep(t *testing.T) { }) } } + +// TestCollectLocalNpmTool checks the routes a node tool's documents give when +// a local install names the package the repository publishes. +func TestCollectLocalNpmTool(t *testing.T) { + t.Parallel() + const zx = `{"name": "zx", "bin": {"zx": "build/cli.js"}}` + const use = "```sh\nzx --version\n```\n" + tests := []struct { + Manifest string + Readme string + Install string + WantInstalls []string + WantSession []string + WantNoRoute bool + }{{ // Test 0: A scoped name with a version is still the repository's own package. + Manifest: `{"name": "@scope/tool", "bin": {"cli": "cli.js"}}`, + Readme: "# tool\n\n```sh\nnpm install @scope/tool@2\n```\n\n```sh\ncli --version\n```\n", + WantInstalls: []string{"npm install @scope/tool@2"}, + WantSession: []string{"npm install @scope/tool@2"}, + }, { // Test 1: A local and a global install in one document are both verified. + Manifest: zx, + Readme: "# zx\n\n```sh\nnpm install zx\n```\n\n```sh\nnpm install -g zx\n```\n\n" + use, + WantInstalls: []string{"npm install zx", "npm install -g zx"}, + WantSession: []string{"npm install zx"}, + }, { // Test 2: The same pair split across two documents is both verified too. + Manifest: zx, + Readme: "# zx\n\n```sh\nnpm install zx\n```\n\n" + use, + Install: "# Install\n\n```sh\nnpm install -g zx\n```\n", + WantInstalls: []string{"npm install zx", "npm install -g zx"}, + WantSession: []string{"npm install zx"}, + }, { // Test 3: A private package is never published, so its name is no route. + Manifest: `{"name": "demo", "private": true, "bin": {"demo": "cli.js"}}`, + Readme: "# demo\n\n```sh\nnpm install demo\n```\n", + WantNoRoute: true, + }, { // Test 4: A workspace install of a declared name is no route. + Manifest: zx, + Readme: "# zx\n\n```sh\nnpm install -w zx lodash\n```\n", + WantNoRoute: true, + }, { // Test 5: Two routes to one package open one session install, whatever its binary is named. + Manifest: `{"name": "@scope/tool", "bin": {"cli": "cli.js"}}`, + Readme: "# tool\n\n```sh\nnpm install @scope/tool\nnpm install -g @scope/tool\n```\n\n" + + "```sh\ncli --version\n```\n", + WantInstalls: []string{"npm install @scope/tool", "npm install -g @scope/tool"}, + WantSession: []string{"npm install @scope/tool"}, + }, { // Test 6: Any truthy private value keeps a package off the registry. + Manifest: `{"name": "demo", "private": "true", "bin": {"demo": "cli.js"}}`, + Readme: "# demo\n\n```sh\nnpm install demo\n```\n", + WantNoRoute: true, + }} + for testNum, test := range tests { + t.Run(fmt.Sprintf("test %d", testNum), func(t *testing.T) { + t.Parallel() + dir := filepath.Join(t.TempDir(), "checkout") + if err := os.MkdirAll(dir, 0o750); err != nil { + t.Fatalf("mkdir: %v", err) + } + writeRepoFile(t, dir, "package.json", test.Manifest) + writeRepoFile(t, dir, "README.md", test.Readme) + if test.Install != "" { + writeRepoFile(t, dir, "INSTALL.md", test.Install) + } + steps, plans, problems := collect([]string{dir}, true) + var installs []string + for _, s := range steps { + if s.Run && pkgKinds[s.Kind].Ecosystem != "" { + installs = append(installs, s.Raw) + } + } + if diff := cmp.Diff(test.WantInstalls, installs, cmpopts.EquateEmpty()); diff != "" { + t.Errorf("install steps mismatch (-want +got):\n%s", diff) + } + var session []string + for _, p := range plans { + for _, in := range p.Installs { + session = append(session, in.Cmd) + } + } + if diff := cmp.Diff(test.WantSession, session, cmpopts.EquateEmpty()); diff != "" { + t.Errorf("session installs mismatch (-want +got):\n%s", diff) + } + noRoute := len(problems) == 1 && problems[0].Reason == ReasonNoInstallRoute + if noRoute != test.WantNoRoute || len(problems) > 1 { + t.Errorf("no-install-route row = %v, want %v: %+v", noRoute, test.WantNoRoute, problems) + } + }) + } +} + +// TestOwnedNamesNpm checks which package.json files name a node package the +// repository publishes, and which binaries each one is recorded with. +func TestOwnedNamesNpm(t *testing.T) { + t.Parallel() + tests := []struct { + Files map[string]string + WantResult map[string][]string + }{{ // Test 0: The root manifest and its binary. + Files: map[string]string{"package.json": `{"name": "zx", "bin": {"zx": "cli.js"}}`}, + WantResult: map[string][]string{"zx": {"zx"}}, + }, { // Test 1: Dependencies, tests, and fixtures declare nothing the repository publishes. + Files: map[string]string{ + "package.json": `{"name": "zx"}`, + "node_modules/dep/package.json": `{"name": "dep", "bin": {"dep": "d.js"}}`, + "test/package.json": `{"name": "sample"}`, + "fixtures/package.json": `{"name": "fixture"}`, + }, + WantResult: map[string][]string{"zx": nil}, + }, { // Test 2: A manifest that does not parse declares nothing. + Files: map[string]string{"package.json": `{"name": "zx", `}, + WantResult: nil, + }, { // Test 3: Two manifests of one name pool their binaries in name order. + Files: map[string]string{ + "package.json": `{"name": "tool", "bin": {"b": "b.js"}}`, + "packages/tool/package.json": `{"name": "tool", "bin": {"a": "a.js"}}`, + }, + WantResult: map[string][]string{"tool": {"a", "b"}}, + }, { // Test 4: A private package is never published, so it is left out. + Files: map[string]string{ + "package.json": `{"name": "tool-monorepo", "private": true}`, + "demo/package.json": `{"name": "demo", "private": true, "bin": {"demo": "d.js"}}`, + "cli/package.json": `{"name": "tool", "bin": "cli.js"}`, + }, + WantResult: map[string][]string{"tool": {"tool"}}, + }, { // Test 5: A workspace member two folders down is read, and one three down is not. + Files: map[string]string{ + "packages/cli/package.json": `{"name": "@scope/cli", "bin": "cli.js"}`, + "packages/group/lib/package.json": `{"name": "@scope/lib"}`, + }, + WantResult: map[string][]string{"@scope/cli": {"cli"}}, + }, { // Test 6: A false private flag leaves the package published. + Files: map[string]string{"package.json": `{"name": "zx", "private": false}`}, + WantResult: map[string][]string{"zx": nil}, + }, { // Test 7: A null private flag does too. + Files: map[string]string{"package.json": `{"name": "zx", "private": null}`}, + WantResult: map[string][]string{"zx": nil}, + }, { // Test 8: A truthy number is private, as npm reads it. + Files: map[string]string{"package.json": `{"name": "zx", "private": 1, "bin": "c.js"}`}, + WantResult: nil, + }, { // Test 9: A truthy string is private, the word false included. + Files: map[string]string{"package.json": `{"name": "zx", "private": "false"}`}, + WantResult: nil, + }, { // Test 10: A binary name that is not a plain word is not a binary. + Files: map[string]string{"package.json": `{"name": "zx", "bin": {"zx": "c.js", + "x'; touch /tmp/owned; '": "e.js", "a\u0000b": "f.js", "-V": "g.js", "a b": "h.js"}}`}, + WantResult: map[string][]string{"zx": {"zx"}}, + }} + for testNum, test := range tests { + t.Run(fmt.Sprintf("test %d", testNum), func(t *testing.T) { + t.Parallel() + dir := filepath.Join(t.TempDir(), "checkout") + for name, body := range test.Files { + if err := os.MkdirAll(filepath.Join(dir, filepath.Dir(name)), 0o750); err != nil { + t.Fatalf("mkdir: %v", err) + } + writeRepoFile(t, dir, name, body) + } + got := ownedNames(dir, "checkout").npm + if diff := cmp.Diff(test.WantResult, got, cmpopts.EquateEmpty()); diff != "" { + t.Errorf("npm packages mismatch (-want +got):\n%s", diff) + } + }) + } +} + +// TestRepoNamesOwnsNpmVersion checks that a version on a node package request +// does not hide the repository's own package, scoped or not. +func TestRepoNamesOwnsNpmVersion(t *testing.T) { + t.Parallel() + rn := repoNames{names: map[string]bool{}, ecosystems: map[string]bool{"node": true}} + rn.addPackageJSON([]byte(`{"name": "@scope/tool"}`)) + rn.addPackageJSON([]byte(`{"name": "zx", "bin": {"zx": "cli.js"}}`)) + tests := []struct { + In string + WantResult bool + }{{ // Test 0: A scoped name. + In: "@scope/tool", WantResult: true, + }, { // Test 1: A scoped name with a version. + In: "@scope/tool@2", WantResult: true, + }, { // Test 2: A scoped name with a tag. + In: "@scope/tool@latest", WantResult: true, + }, { // Test 3: A plain name with a version. + In: "zx@8.8.5", WantResult: true, + }, { // Test 4: Another package with a version. + In: "left-pad@1.3.0", WantResult: false, + }, { // Test 5: Another scope's package of another name. + In: "@other/thing@2", WantResult: false, + }, { // Test 6: An alias installs another package under the repository's name. + In: "@scope/tool@npm:evil", WantResult: false, + }} + for testNum, test := range tests { + t.Run(fmt.Sprintf("test %d", testNum), func(t *testing.T) { + t.Parallel() + got := rn.owns(InstallStep{Kind: "npm-install", Module: test.In, Run: true}) + if diff := cmp.Diff(test.WantResult, got); diff != "" { + t.Errorf("owns mismatch (-want +got):\n%s", diff) + } + }) + } +} diff --git a/report.go b/report.go index 37af0d9..f5d0efe 100644 --- a/report.go +++ b/report.go @@ -406,13 +406,20 @@ func dedupe(in []string) []string { // sharedCauseKey identifies a result by everything except which document it // came from, so rows that say the same thing about different documents group // together. The document prefix is stripped from the detail, since that is the -// only part of an otherwise identical message that differs. +// only part of an otherwise identical message that differs. A local npm +// install keeps its own key, since it and a global install of the same package +// are two commands and often share a document. func sharedCauseKey(r Result) string { detail := r.Detail if r.Step.doc != "" { detail = strings.TrimPrefix(detail, r.Step.doc+": ") } - return strings.Join([]string{r.Step.Repo, r.Step.Kind, string(r.Status), string(r.Reason), detail}, "\x00") + kind := r.Step.Kind + if localNpmRoute(r.Step) { + kind += "|local" + } + parts := []string{r.Step.Repo, kind, string(r.Status), string(r.Reason), detail} + return strings.Join(parts, "\x00") } // versionNote returns "behind " when the installed version is older than the diff --git a/report_test.go b/report_test.go index 2b02d78..1b7514d 100644 --- a/report_test.go +++ b/report_test.go @@ -255,6 +255,27 @@ func TestSharedCauseKeyIgnoresVersion(t *testing.T) { } } +// TestSharedCauseKeySplitsNpmRoutes checks that a local and a global install +// of one package stay two rows. They are two commands in one document, and +// counting them as one row across two documents misstated both. +func TestSharedCauseKeySplitsNpmRoutes(t *testing.T) { + t.Parallel() + row := func(raw, doc string) Result { + return Result{ + Step: InstallStep{Repo: "zx", Kind: "npm-install", Module: "zx", Raw: raw, doc: doc}, + Status: StatusVerified, Detail: "zx 1.0", + } + } + local, global := row("npm install zx", "README.md"), row("npm install -g zx", "README.md") + if sharedCauseKey(local) == sharedCauseKey(global) { + t.Error("a local and a global install share one key") + } + other := row("npm install -g zx", "docs/setup.md") + if diff := cmp.Diff(sharedCauseKey(global), sharedCauseKey(other)); diff != "" { + t.Errorf("one route in two documents split (-readme +setup):\n%s", diff) + } +} + // TestReportTriage checks that the triage listing prints every result that is // not working, whole, and leaves out the VERIFIED result and VERIFIED lines. func TestReportTriage(t *testing.T) { diff --git a/run.go b/run.go index 388015c..dae97ea 100644 --- a/run.go +++ b/run.go @@ -147,7 +147,10 @@ func (d *DockerRunner) Run(ctx context.Context, step InstallStep) Result { ctx, cancel := context.WithTimeout(ctx, d.Timeout+60*time.Second) defer cancel() - image := d.imageFor(step) + image, blocked := d.imageFor(step) + if blocked != "" { + return Result{Step: step, Status: StatusBlocked, Reason: ReasonMissingDependency, Detail: blocked} + } start := time.Now() name := sandbox.Name() // The official Go images pin GOTOOLCHAIN=local, so a module asking for a @@ -226,19 +229,29 @@ var reNetworkError = regexp.MustCompile( // the image that provides the toolchain its commands assume, so a Rust or Node // project builds with the tools its docs were written for. Everything else runs // in the configured default. -func (d *DockerRunner) imageFor(step InstallStep) string { +func (d *DockerRunner) imageFor(step InstallStep) (image, blocked string) { if pk, ok := pkgKinds[step.Kind]; ok { if tc, known := toolchains[pk.Ecosystem]; known && tc.Image != "" { - return tc.Image + return toolchainImage(tc, step.dir) } - return d.Image + return d.Image, "" } if step.Kind != "git-clone" { - return d.Image + return d.Image, "" } tc, ok := detectToolchain(step.Block, step.dir) if !ok || tc.Image == "" { - return d.Image + return d.Image, "" + } + return toolchainImage(tc, step.dir) +} + +// toolchainImage returns the image a toolchain's step runs in for the checkout +// at dir. Only Python narrows it, to the newest release the project states it +// supports, and blocked is set when the stated range allows no image at all. +func toolchainImage(tc toolchain, dir string) (image, blocked string) { + if tc.Name == "python" { + return pythonImage(dir) } - return tc.Image + return tc.Image, "" } diff --git a/run_test.go b/run_test.go index fc7bbdc..a59050b 100644 --- a/run_test.go +++ b/run_test.go @@ -895,6 +895,15 @@ func TestPkgScriptPrefersNamedBinary(t *testing.T) { if strings.Contains(evil, "rm -rf") { t.Error("shell syntax from the docs reached the owned-bins lookup") } + // A binary name read from a manifest in the checkout is no more trusted + // than a package name read from a document. + for _, name := range []string{"x'; touch /tmp/owned; '", "a b", "$(id)", "x\\", "a\nb", "a\x00b", "a\tb"} { + bad := pkgScriptFor(InstallStep{ + Kind: "npm-install", Raw: "npm install zx", Module: "zx", Binary: name}, 300) + if strings.Contains(bad, name) { + t.Errorf("binary name %q reached the script", name) + } + } if strings.Contains(script, "pip show") { t.Error("pipx script gained a lookup only pip needs") } @@ -1048,6 +1057,104 @@ func TestPkgScriptOwnedBinaries(t *testing.T) { } } +// TestPkgScriptLocalNpmBinaries runs the local npm script against a fake npm. +// The installed package's own manifest decides which binary is smoke-tested: +// the scratch project's bin directory also holds every dependency's tool, and +// a library that ships no binary must not borrow one. +func TestPkgScriptLocalNpmBinaries(t *testing.T) { + t.Parallel() + for _, tool := range []string{"bash", "timeout", "comm", "node"} { + if _, err := exec.LookPath(tool); err != nil { + t.Skipf("no %s", tool) + } + } + tests := []struct { + Raw string + Package string + Manifest string + Bins []string + Binary string + WantLine string + WantNoBin bool + }{{ // Test 0: A library with no binary does not borrow a dependency's tool. + Raw: "npm install chalk", Package: "chalk", Manifest: `{"name": "chalk"}`, + Bins: []string{"loose-envify"}, WantNoBin: true, + }, { // Test 1: A declared binary is smoke-tested, not the dependency sorted before it. + Raw: "npm install zx # add -g to install globally", Package: "zx", + Manifest: `{"name": "zx", "bin": {"zx": "build/cli.js"}}`, + Bins: []string{"acorn", "zx"}, Binary: "zx", WantLine: "SMOKELINE=zx 1.0", + }, { // Test 2: A single-path bin is named after the scoped package, with no name documented. + Raw: "npm i @scope/tool@2", Package: "@scope/tool", + Manifest: `{"name": "@scope/tool", "bin": "cli.js"}`, + Bins: []string{"acorn", "tool"}, WantLine: "SMOKELINE=tool 1.0", + }, { // Test 3: The documented name wins among several declared binaries. + Raw: "npm install multi", Package: "multi", + Manifest: `{"name": "multi", "bin": {"aaa": "a.js", "multi": "m.js"}}`, + Bins: []string{"aaa", "multi"}, Binary: "multi", WantLine: "SMOKELINE=multi 1.0", + }, { // Test 4: A manifest that cannot be read still stands as an answer of none. + Raw: "npm install broken", Package: "broken", Manifest: `{"name": `, + Bins: []string{"acorn"}, WantNoBin: true, + }} + for testNum, test := range tests { + t.Run(fmt.Sprintf("test %d", testNum), func(t *testing.T) { + t.Parallel() + dir := t.TempDir() + stage := filepath.Join(dir, "stage", "node_modules") + for _, d := range []string{filepath.Join(dir, "fake"), filepath.Join(stage, ".bin"), + filepath.Join(stage, filepath.FromSlash(test.Package))} { + if err := os.MkdirAll(d, 0o755); err != nil { + t.Fatal(err) + } + } + manifest := filepath.Join(stage, filepath.FromSlash(test.Package), "package.json") + if err := os.WriteFile(manifest, []byte(test.Manifest), 0o600); err != nil { + t.Fatal(err) + } + for _, b := range test.Bins { + tool := fmt.Sprintf("#!/bin/sh\necho '%s 1.0'\n", b) + //nolint:gosec // Test fixture must be executable. + if err := os.WriteFile(filepath.Join(stage, ".bin", b), []byte(tool), 0o755); err != nil { + t.Fatal(err) + } + } + npm := fmt.Sprintf("#!/bin/sh\ncase \"$1\" in\ninit) echo '{}' > package.json ;;\n"+ + "install|i|add) cp -R %q . ;;\n*) exit 2 ;;\nesac\n", stage) + //nolint:gosec // Test fixture must be executable. + if err := os.WriteFile(filepath.Join(dir, "fake", "npm"), []byte(npm), 0o755); err != nil { + t.Fatal(err) + } + script := pkgScriptFor(InstallStep{ + Kind: "npm-install", Raw: test.Raw, Module: test.Package, Binary: test.Binary}, 60) + cmd := exec.Command("bash", "-c", script) + cmd.Env = append(os.Environ(), "HOME="+filepath.Join(dir, "home"), + "PATH="+filepath.Join(dir, "fake")+":"+os.Getenv("PATH")) + out, err := cmd.CombinedOutput() + if err != nil { + t.Fatalf("script: %v\n%s", err, out) + } + got := string(out) + if !strings.Contains(got, "BUILDCODE=0\n") { + t.Fatalf("install did not run clean:\n%s", got) + } + if test.WantNoBin != strings.Contains(got, "NOBIN=1") { + t.Errorf("NOBIN = %v, want %v\n%s", !test.WantNoBin, test.WantNoBin, got) + } + if test.WantLine != "" && !strings.Contains(got, test.WantLine+"\n") { + t.Errorf("missing %q\n%s", test.WantLine, got) + } + // An install that ran and left no binary of its own is reported as + // run, never as verified and never as failed. + status := classify(InstallStep{Kind: "npm-install", Raw: test.Raw}, got, 0).Status + if test.WantNoBin && status != StatusRan { + t.Errorf("status = %s, want %s\n%s", status, StatusRan, got) + } + if !test.WantNoBin && status != StatusVerified { + t.Errorf("status = %s, want %s\n%s", status, StatusVerified, got) + } + }) + } +} + // TestCloneScriptFunctionProduct runs a generated clone script whose recipe // sources a file defining the product as a shell function, and checks that the // function is found in the recipe's own shell and reported with its version. diff --git a/runclassify.go b/runclassify.go index 25b081a..9bade32 100644 --- a/runclassify.go +++ b/runclassify.go @@ -212,13 +212,6 @@ var reErrorDeclaration = regexp.MustCompile( // line printed, which npm reserves for the path of a debug log. `^npm error\b`) -// failureLine returns the most informative line of a failure. Reporting a -// wrapper's summary hides the error a reader needs, so the summary is skipped -// in favor of the last line that says what actually broke. A tool that answers -// a bad argument by printing its whole usage screen is the other direction of -// the same mistake: the last line there is the final flag's description, which -// tells a reader nothing, so an error the parser declared above the screen -// wins over anything below it. // buildFailLine picks the line that best explains a failed build. A real error // declaration, such as cargo's "error[E0432]: unresolved import", is preferred // over a build tool's trailing summary, since the summary names the target @@ -234,6 +227,15 @@ func buildFailLine(lines []string) string { return failureLine(lines) } +// failureLine returns the most informative line of a failure. Reporting a +// wrapper's summary hides the error a reader needs, so the summary is skipped +// in favor of the last line that says what actually broke. A tool that answers +// a bad argument by printing its whole usage screen is the other direction of +// the same mistake: the last line there is the final flag's description, which +// tells a reader nothing, so an error the parser declared above the screen +// wins over anything below it. A tail that is only a location, a stack frame +// or a path, gives way to the line that says what went wrong: lux ends a Go +// panic on runtime/asm_arm64.s:1447, and rtk ends on the path it looked in. func failureLine(lines []string) string { for i, line := range lines { line = strings.TrimSpace(line) @@ -254,7 +256,52 @@ func failureLine(lines []string) string { if line == "" || reWrapperSummary.MatchString(line) { continue } + if reLocationLine.MatchString(line) { + if first := explainingLine(lines); first != "" { + return first + } + } return line } return lastLine(lines) } + +// reLocationLine matches a line that only says where something happened: a +// source location with nothing after it, a stack frame, a traceback header, or +// a bare path. A log line that opens with a location and goes on to say what +// failed, as Go tests print them, is not one. +var reLocationLine = regexp.MustCompile( + `^\S+\.(go|s|rs|py|c|cc|cpp):\d+(:\d+)?( \+0x[0-9a-f]+)?$|^goroutine \d+|^at .+\(.+:\d+\)$` + + `|^Traceback|^/[^\s:]+$`) + +// explainingLine returns the first error declaration in lines, or failing +// that the first line that is not itself a location, empty when there is none. +// A line that ends in a colon introduces the next one, as lux prints +// "Downloading error:" above the cause, so the two are joined. +func explainingLine(lines []string) string { + first := -1 + for i, line := range lines { + line = strings.TrimSpace(line) + if reErrorDeclaration.MatchString(line) { + return line + } + if first < 0 && line != "" && !reLocationLine.MatchString(line) { + first = i + } + } + if first < 0 { + return "" + } + out := strings.TrimSpace(lines[first]) + if strings.HasSuffix(out, ":") { + for _, next := range lines[first+1:] { + if next = strings.TrimSpace(next); next != "" { + if !reLocationLine.MatchString(next) { + out += " " + next + } + break + } + } + } + return out +} diff --git a/runclassify_test.go b/runclassify_test.go new file mode 100644 index 0000000..d7408f4 --- /dev/null +++ b/runclassify_test.go @@ -0,0 +1,47 @@ +package main + +import ( + "fmt" + "strings" + "testing" + + "github.com/google/go-cmp/cmp" +) + +// TestFailureLineSkipsLocationTail checks that a failure ending on a stack +// frame or a bare path is explained by the line that says what went wrong, +// and that an ordinary last line is still the one reported. +func TestFailureLineSkipsLocationTail(t *testing.T) { + t.Parallel() + tests := []struct { + Output string + WantLine string + }{{ // Test 0: lux's Go panic ends on a runtime source location. + Output: "Downloading https://www.youtube.com/watch?v=x error:\nunable to find basejs URL in playerConfig\n" + + "main.main\n\t/go/pkg/mod/github.com/iawia002/lux@v0.24.1/main.go:13\n" + + "runtime.goexit\n\t/usr/local/go/src/runtime/asm_arm64.s:1447", + WantLine: "Downloading https://www.youtube.com/watch?v=x error: unable to find basejs URL in playerConfig", + }, { // Test 1: rtk ends on the path it looked in. + Output: "rtk: rtk-proxy not found.\nThe daemon ships as its own release archive.\n" + + "Looked in:\n /root/.local/bin/rtk-proxy", + WantLine: "rtk: rtk-proxy not found.", + }, { // Test 2: an error declaration wins over an earlier plain line. + Output: "starting\nError: config missing\n at load (/app/x.js:3:1)", + WantLine: "Error: config missing", + }, { // Test 3: a plain two-line error keeps its last line. + Output: "reading config\ncannot parse key 'x'", + WantLine: "cannot parse key 'x'", + }, { // Test 4: a Go test log line that names a location and then the cause keeps the cause. + Output: "go: downloading github.com/google/go-cmp v0.7.0\npostgres.go:26: PostgreSQL is not installed", + WantLine: "postgres.go:26: PostgreSQL is not installed", + }} + for testNum, test := range tests { + t.Run(fmt.Sprintf("test %d", testNum), func(t *testing.T) { + t.Parallel() + got := failureLine(strings.Split(test.Output, "\n")) + if diff := cmp.Diff(test.WantLine, got); diff != "" { + t.Errorf("line mismatch (-want +got):\n%s", diff) + } + }) + } +} diff --git a/runscript.go b/runscript.go index 948b70e..bdb64e8 100644 --- a/runscript.go +++ b/runscript.go @@ -23,18 +23,95 @@ import ( func pkgScriptFor(step InstallStep, timeoutSecs int) string { pk := pkgKinds[step.Kind] body := shellCommand(step.Raw) + name := step.Module + if step.Kind == "npm-install" && localNpmInstall(body) { + body = npmLocalCmd(body) + pk.BinDir = `"` + npmLocalBin + `"` + pk.OwnedBins = npmLocalOwnedBins + _, name = localNpmPackage(step.Raw) + } if pk.Bootstrap != "" { body = pk.Bootstrap + "\n" + body } body = strings.ReplaceAll(body, "'", `'\''`) // The package name came out of a document, so it is interpolated into the // owned-bins lookup only when it carries no shell syntax. Without the - // lookup the script simply falls back to the bin diff. + // lookup the script simply falls back to the bin diff. The expected binary + // can come out of a manifest in the checkout, so it is held to the same + // test, and without it the first candidate is smoke-tested. own := "" - if pk.OwnedBins != "" && !strings.ContainsAny(step.Module, "$\x60;&|<>()'\" ") { - own = fmt.Sprintf(pk.OwnedBins, step.Module) + if pk.OwnedBins != "" && name != "" && !strings.ContainsAny(name, shellSyntax) { + own = fmt.Sprintf(pk.OwnedBins, name) + } + bin := step.Binary + if strings.ContainsAny(bin, shellSyntax) { + bin = "" + } + return fmt.Sprintf(pkgScript, pk.BinDir, timeoutSecs, body, bin, own) +} + +// shellSyntax holds the characters that keep a name out of a generated script. +const shellSyntax = "$\x60;&|<>()'\"\\ \n\t\r\x00" + +// npmLocalOwnedBins is the owned-bins lookup for a local npm install. The +// installed package's own manifest says which binaries it provides, and that +// answer stands even when it is none: the scratch project's bin directory +// also holds every dependency's tool, and the first new file there once stood +// to be smoke-tested in the name of a library that ships no binary. +const npmLocalOwnedBins = `ownknown=1 +own=$(node -e ' +const fs = require("fs"), path = require("path"); +let pkg = {}; +try { pkg = JSON.parse(fs.readFileSync(process.argv[1], "utf8")); } catch (e) {} +let bin = pkg.bin || {}; +if (typeof bin === "string") bin = { [String(pkg.name).split("/").pop()]: bin }; +for (const name of Object.keys(bin)) console.log(path.join(process.argv[2], name)); +' "` + npmLocalDir + `/node_modules/%s/package.json" "$BINDIR" 2>/dev/null)` + +const ( + // npmLocalDir is the scratch project a local npm install runs in. Run in + // the checkout instead, zx's `npm install zx` met zx's own workspace and + // failed with `Tracker "idealTree" already exists`, and a reader runs it + // in a project of their own. + npmLocalDir = "$HOME/kibble-npm" + // npmLocalBin is where that project's installed binaries land. + npmLocalBin = npmLocalDir + "/node_modules/.bin" +) + +// localNpmInstall reports whether cmd is a single npm install of named +// packages into the current project rather than globally. A bare npm install +// installs the checkout's own dependencies and is left alone. +func localNpmInstall(cmd string) bool { + if strings.ContainsAny(cmd, ";&|<>`$()") { + return false + } + f := strings.Fields(cmd) + if len(f) < 3 || f[0] != "npm" || (f[1] != "install" && f[1] != "i" && f[1] != "add") { + return false + } + pkgs := 0 + for _, a := range f[2:] { + switch { + case a == "-g" || a == "--global" || strings.HasPrefix(a, "--location=global"): + return false + case !strings.HasPrefix(a, "-"): + pkgs++ + } } - return fmt.Sprintf(pkgScript, pk.BinDir, timeoutSecs, body, step.Binary, own) + return pkgs > 0 +} + +// localNpmRoute reports whether a step is a local npm install, the route kept +// apart from a global install of the same package. +func localNpmRoute(s InstallStep) bool { + return s.Kind == "npm-install" && localNpmInstall(shellCommand(s.Raw)) +} + +// npmLocalCmd runs a local npm install in the scratch project, creating it +// with npm init -y first. +func npmLocalCmd(cmd string) string { + return `mkdir -p "` + npmLocalDir + `" && cd "` + npmLocalDir + `" && ` + + `{ [ -f package.json ] || npm init -y >/dev/null; } && ` + cmd } // shellCommand returns a documented line in the form a shell can run: the diff --git a/session.go b/session.go index c1b739a..8738341 100644 --- a/session.go +++ b/session.go @@ -138,7 +138,7 @@ func sessionScript(plan *kplan.Plan, installSecs, lineSecs int) (string, map[str // BLOCKED on the next from the same code. b.WriteString(`exec 2>&1 export GOBIN="$(go env GOPATH 2>/dev/null || echo /root/go)/bin" -export PATH="$GOBIN:$HOME/.local/bin:${CARGO_HOME:-$HOME/.cargo}/bin:$PATH" +export PATH="$GOBIN:$HOME/.local/bin:${CARGO_HOME:-$HOME/.cargo}/bin:$PATH:` + npmLocalBin + `" export EDITOR=true VISUAL=true GIT_EDITOR=` + gitEditorPath + ` cat > ` + gitEditorPath + ` <<'KIBBLE-EDITOR' ` + commitEditor + `KIBBLE-EDITOR @@ -174,6 +174,9 @@ printf '\nKIBBLE-PKGS CODE=%%d\n' "$?" fmt.Fprintf(&b, "%s >/dev/null 2>&1 || true\n", in.Bootstrap) } cmd := in.Cmd + if localNpmInstall(shellCommand(cmd)) { + cmd = npmLocalCmd(shellCommand(cmd)) + } if in.Dir != "" { cmd = "cd " + in.Dir + "\n" + cmd } diff --git a/sessionparse.go b/sessionparse.go index 2f8e5be..98f7e6d 100644 --- a/sessionparse.go +++ b/sessionparse.go @@ -4,6 +4,7 @@ import ( "fmt" "path" "regexp" + "slices" "strconv" "strings" "time" @@ -50,9 +51,15 @@ type lineOutcome struct { resolver bool // missing marks a line whose needed file did not exist just before it ran. missing bool + // notFound marks a line that invoked a documented binary and exited with a + // code the documents say means nothing was found. + notFound bool // member marks a line that ran in a directory an earlier line's own words // declared a member of the checkout's workspace. member bool + // truncated marks a line that printed more than the session keeps, so the + // end of output is not the end of what the line said. + truncated bool // creators maps each path an earlier line of the same document was to // create, and did not pass, to that line's command. creators map[string]string @@ -106,9 +113,14 @@ func classifyExample(step InstallStep, plan *kplan.Plan, out string, wrapped map var chunk []string aborted, done, noBin := false, false, false pkgCode := 0 + cut := false keep := func(s string) { - if strings.TrimSpace(s) != "" && len(chunk) < 200 { + switch { + case strings.TrimSpace(s) == "": + case len(chunk) < 200: chunk = append(chunk, s) + default: + cut = true } } for _, line := range strings.Split(out, "\n") { @@ -127,19 +139,20 @@ func classifyExample(step InstallStep, plan *kplan.Plan, out string, wrapped map case strings.Contains(line, "KIBBLE-PKGS CODE="): _, code, _ := markerTail(line, "KIBBLE-PKGS CODE=") pkgCode, _ = strconv.Atoi(strings.TrimSpace(code)) - chunk = nil + chunk, cut = nil, false case strings.Contains(line, "KIBBLE-BUILD CODE="): - chunk = nil + chunk, cut = nil, false case strings.Contains(line, "KIBBLE-ABORT"): aborted = true case strings.Contains(line, "KIBBLE-STEP "): - chunk = nil + chunk, cut = nil, false case strings.Contains(line, "KIBBLE-DONE"): done = true case reLineMarker.MatchString(line): m := reLineMarker.FindStringSubmatch(line) keep(line[:len(line)-len(m[0])]) o := lineOutcome{code: -1, output: strings.Join(chunk, "\n"), missing: absent[m[1]+":"+m[2]]} + o.truncated = cut switch { case m[3] != "": o.code, _ = strconv.Atoi(m[3]) @@ -150,7 +163,7 @@ func classifyExample(step InstallStep, plan *kplan.Plan, out string, wrapped map o.ready = ready == 0 } outcomes[m[1]+":"+m[2]] = o - chunk = nil + chunk, cut = nil, false default: keep(line) } @@ -279,6 +292,7 @@ func buildOutcomes(plan *kplan.Plan, outcomes map[string]lineOutcome, wrapped ma if bin, _ := kplan.InvokedBinary(docblock.Flatten(l.Cmd), bins); bin != "" { o.ownProject = kplan.CommandEcosystem[bin] != "" o.resolver = o.ownProject || resolverTools[bin] + o.notFound = slices.Contains(plan.NotFoundCodes, o.code) } o.member = members[o.cwd] recordMembers(members, o) @@ -352,7 +366,7 @@ func trackCreators(pending map[string]map[string]string, doc string, l kplan.Pla // when unplanned is set the code block kibble did not run. func settleNeed(lr lineResult, l kplan.PlanLine, o lineOutcome, blocker string, unplanned bool) lineResult { lr.Code = o.code - lr.output = o.output + lr.output, lr.cwd = o.output, o.cwd if blocker == "" { lr.Status = StatusGap lr.Reason = ReasonMissingFixture diff --git a/sessionparse_test.go b/sessionparse_test.go index 6361dc0..6dbf0f6 100644 --- a/sessionparse_test.go +++ b/sessionparse_test.go @@ -668,6 +668,12 @@ func TestClassifyExampleOwnProject(t *testing.T) { Bin: "uvx", Cmd: "uvx nope-zzz-pkg-kibble", Cwd: "/work/repo", Code: 1, Output: uvxRun, WantStatus: StatusFail, + }, { // Test 15: witr finding no such process found no data. + Bin: "witr", Cmd: "witr node", Cwd: "/work/repo", Code: 2, + Output: "no running process or service named \"node\"\n\n" + + "No matching process or service found. Please check your query or try a different " + + "name/port/PID.\nFor usage and options, run: witr --help", + WantStatus: StatusSkipped, WantReason: ReasonNoDataExpected, }} for testNum, test := range tests { t.Run(fmt.Sprintf("test %d", testNum), func(t *testing.T) { diff --git a/toolchain.go b/toolchain.go index 7e59b85..7f54b91 100644 --- a/toolchain.go +++ b/toolchain.go @@ -1,11 +1,14 @@ package main import ( - kplan "github.com/dcadolph/kibble/internal/plan" + "fmt" "os" "path/filepath" "regexp" + "strconv" "strings" + + kplan "github.com/dcadolph/kibble/internal/plan" ) // toolchain is a language ecosystem and the container image that provides it. @@ -209,6 +212,11 @@ func missingCommand(out string) (string, bool) { return name, true } } + // acme.sh refuses to install without cron and says so as an offer: + // "Please add '--force' and try install again to go without crontab." + if m := reWithoutCommand.FindStringSubmatch(line); m != nil { + return m[1], true + } for _, marker := range []string{": command not found", ": not found"} { idx := strings.Index(line, marker) if idx < 0 { @@ -236,3 +244,118 @@ var reExecNotFound = regexp.MustCompile(`exec: "?([^":]+)"?: executable file not // from the phrases a tool also uses about its own optional pieces. var reRequiredCommand = regexp.MustCompile( `(?i)missing required commands?:\s*([A-Za-z0-9._+-]+)`) + +// reWithoutCommand matches an installer offering to go on without a system +// program the image lacks, which names that program as plainly as a missing +// command list does. +var reWithoutCommand = regexp.MustCompile(`(?i)--force.{0,40}\bwithout (crontab|cron)\b`) + +// pythonImage returns the image a Python step in the checkout at dir runs in. +// A project that states an upper bound on Python, such as aider's +// requires-python ">=3.10,<3.15", gets the newest image the bound allows, +// since the default python:3 tag moves past a supported release and a reader +// on a supported Python would install fine. With no bound it returns the +// default image. When the stated range allows no Python 3 release at all, +// image is empty and blocked says why. +func pythonImage(dir string) (image, blocked string) { + def := toolchains["python"].Image + stated, maxMinor, bounded := statedPython(dir) + switch { + case !bounded: + return def, "" + case maxMinor < 0: + return "", fmt.Sprintf("the project states Python %s, which no Python 3 image satisfies", stated) + } + return fmt.Sprintf("python:3.%d", maxMinor), "" +} + +var ( + // rePyprojectPython matches the requires-python setting of a pyproject.toml. + rePyprojectPython = regexp.MustCompile(`(?m)^\s*requires-python\s*=\s*["']([^"']+)["']`) + // reSetupCfgPython matches the python_requires setting of a setup.cfg. + reSetupCfgPython = regexp.MustCompile(`(?m)^\s*python_requires\s*=\s*(\S[^\n]*)$`) + // reReadmePython matches a README sentence such as markitdown's "requires + // Python 3.10 through 3.14". + reReadmePython = regexp.MustCompile(`(?i)\bPython (3\.\d+) through 3\.(\d+)\b`) + // rePythonSpec parses one version specifier into its operator and version. + rePythonSpec = regexp.MustCompile(`^(<=|<|==|~=|>=|>|!=)\s*(\d+)(?:\.(\d+|\*))?(?:\.(\d+|\*))?$`) +) + +// statedPython returns the Python range a checkout states, the highest +// Python 3 minor release it allows, and whether it states an upper bound at +// all. The project's own metadata wins over prose: the root pyproject.toml, +// then setup.cfg, then a README sentence naming a first and last release. +// maxMinor is -1 when the bound excludes every Python 3 release. +func statedPython(dir string) (stated string, maxMinor int, bounded bool) { + if dir == "" { + return "", 0, false + } + for _, src := range []struct { + file string + re *regexp.Regexp + }{{"pyproject.toml", rePyprojectPython}, {"setup.cfg", reSetupCfgPython}} { + b, err := os.ReadFile(filepath.Join(dir, src.file)) + if err != nil { + continue + } + if m := src.re.FindStringSubmatch(string(b)); m != nil { + spec := strings.TrimSpace(m[1]) + maxMinor, bounded = pythonUpperBound(spec) + return spec, maxMinor, bounded + } + } + text, _, err := readREADME(dir) + if err != nil { + return "", 0, false + } + if m := reReadmePython.FindStringSubmatch(text); m != nil { + last, _ := strconv.Atoi(m[2]) + return m[1] + " through 3." + m[2], last, true + } + return "", 0, false +} + +// pythonUpperBound reads a PEP 440 specifier list and returns the highest +// Python 3 minor release it allows, and whether it bounds Python 3 at all. +// The compatible-release form ~=3.12 allows every later 3.x and so bounds +// nothing, while ~=3.12.0 stops at 3.12. Lower bounds and exclusions are +// ignored: the newest allowed image is the one picked, and python:3.N for an +// allowed N always exists. +func pythonUpperBound(spec string) (int, bool) { + maxMinor, bounded := 0, false + limit := func(n int) { + if !bounded || n < maxMinor { + maxMinor = n + } + bounded = true + } + for _, part := range strings.Split(spec, ",") { + m := rePythonSpec.FindStringSubmatch(strings.TrimSpace(part)) + if m == nil { + continue + } + op := m[1] + major, _ := strconv.Atoi(m[2]) + minor, minorErr := strconv.Atoi(m[3]) + patch, patchErr := strconv.Atoi(m[4]) + hasMinor, hasPatch := minorErr == nil, patchErr == nil + switch { + case major < 3: + if op == "<" || op == "<=" || op == "==" { + limit(-1) + } + case major > 3: + case op == "<" && !hasMinor: + limit(-1) + case op == "<" && hasPatch && patch > 0: + limit(minor) + case op == "<": + limit(minor - 1) + case (op == "<=" || op == "==") && hasMinor: + limit(minor) + case op == "~=" && hasMinor && m[4] != "": + limit(minor) + } + } + return maxMinor, bounded +} diff --git a/toolchain_test.go b/toolchain_test.go index 5833cee..4555911 100644 --- a/toolchain_test.go +++ b/toolchain_test.go @@ -7,6 +7,8 @@ import ( "testing" "github.com/google/go-cmp/cmp" + + kplan "github.com/dcadolph/kibble/internal/plan" ) // TestCommandWords checks that each simple command on a line yields its @@ -134,6 +136,12 @@ func TestMissingCommand(t *testing.T) { }, { // Test 5: the Go toolchain's exec-not-found is a missing build tool. In: `lib/results.go:272: running "easyjson": exec: "easyjson": executable file not found in $PATH`, WantOK: true, WantName: "easyjson", + }, { // Test 6: acme.sh offers to go on without the crontab the image lacks. + In: "[Wed Nov 11 15:26:35 UTC 2026] Please add '--force' and try install again to go " + + "without crontab.\n[Wed Nov 11 15:26:35 UTC 2026] Pre-check failed, cannot install.", + WantOK: true, WantName: "crontab", + }, { // Test 7: a force flag with no missing program named is not one. + In: "Please add '--force' to overwrite the existing file.", WantOK: false, }} for testNum, test := range tests { t.Run(fmt.Sprintf("test %d", testNum), func(t *testing.T) { @@ -170,3 +178,86 @@ func TestEscapeAnnotation(t *testing.T) { }) } } + +// TestPythonImage checks that a stated upper bound on Python picks the newest +// image it allows, that an unbounded or absent range keeps the default, and +// that a range no Python 3 image satisfies is blocked rather than run. +func TestPythonImage(t *testing.T) { + t.Parallel() + tests := []struct { + Files map[string]string + WantImage string + WantBlocked bool + }{{ // Test 0: aider's pyproject bound picks python:3.14. + Files: map[string]string{"pyproject.toml": "[project]\nrequires-python = \">=3.10,<3.15\"\n"}, + WantImage: "python:3.14", + }, { // Test 1: an inclusive bound picks that release. + Files: map[string]string{"pyproject.toml": "requires-python = '<=3.14'\n"}, + WantImage: "python:3.14", + }, { // Test 2: a compatible release on a minor bounds nothing. + Files: map[string]string{"pyproject.toml": "requires-python = \"~=3.12\"\n"}, + WantImage: "python:3", + }, { // Test 3: a compatible release on a patch stops at its minor. + Files: map[string]string{"pyproject.toml": "requires-python = \"~=3.12.0\"\n"}, + WantImage: "python:3.12", + }, { // Test 4: a lower bound alone keeps the default. + Files: map[string]string{"pyproject.toml": "requires-python = \">=3.10\"\n"}, + WantImage: "python:3", + }, { // Test 5: no stated range keeps the default. + Files: map[string]string{"README.md": "# Tool\n"}, + WantImage: "python:3", + }, { // Test 6: setup.cfg is read when there is no pyproject.toml. + Files: map[string]string{"setup.cfg": "[options]\npython_requires = >=3.9, <3.13\n"}, + WantImage: "python:3.12", + }, { // Test 7: markitdown's README range is used when metadata says nothing. + Files: map[string]string{"README.md": "Requires Python 3.10 through 3.14.\n"}, + WantImage: "python:3.14", + }, { // Test 8: pyproject wins over the README. + Files: map[string]string{"pyproject.toml": "requires-python = \"<3.13\"\n", + "README.md": "Python 3.10 through 3.14\n"}, + WantImage: "python:3.12", + }, { // Test 9: a bound below Python 3 is blocked. + Files: map[string]string{"pyproject.toml": "requires-python = \"<3\"\n"}, + WantBlocked: true, + }, { // Test 10: a patch above zero still allows its minor. + Files: map[string]string{"pyproject.toml": "requires-python = \">=3.8,<3.13.2\"\n"}, + WantImage: "python:3.13", + }} + for testNum, test := range tests { + t.Run(fmt.Sprintf("test %d", testNum), func(t *testing.T) { + t.Parallel() + dir := t.TempDir() + for name, body := range test.Files { + if err := os.WriteFile(filepath.Join(dir, name), []byte(body), 0o600); err != nil { + t.Fatal(err) + } + } + image, blocked := pythonImage(dir) + if diff := cmp.Diff(test.WantImage, image); diff != "" { + t.Errorf("image mismatch (-want +got):\n%s", diff) + } + if got := blocked != ""; got != test.WantBlocked { + t.Errorf("blocked = %q, want blocked %v", blocked, test.WantBlocked) + } + }) + } +} + +// TestPythonStepImage checks that a pip install step and a Python example +// session both run on the image the checkout's range picks. +func TestPythonStepImage(t *testing.T) { + t.Parallel() + dir := t.TempDir() + if err := os.WriteFile(filepath.Join(dir, "pyproject.toml"), + []byte("requires-python = \">=3.10,<3.15\"\n"), 0o600); err != nil { + t.Fatal(err) + } + d := &DockerRunner{Image: "golang:1"} + if image, _ := d.imageFor(InstallStep{Kind: "pip-install", dir: dir}); image != "python:3.14" { + t.Errorf("install image = %q, want python:3.14", image) + } + plan := &kplan.Plan{Installs: []kplan.PlanInstall{{Ecosystem: "python"}}} + if image, ok, _ := d.exampleImage(plan, dir); !ok || image != "python:3.14" { + t.Errorf("session image = %q ok=%v, want python:3.14", image, ok) + } +}