Skip to content

Commit 1ca6201

Browse files
authored
fix(process): Program.Find used a SECOND, unfixed copy of lookPath (#10)
v0.16.2 fixed executable resolution on Windows and did not fix executable resolution on Windows, because this module carried TWO copies of it: exec/exec.go lookPath <- fixed in v0.16.2 os_exec_link.go lookPath <- byte-identical, untouched Program.Find calls the second. os_exec_link.go:45 still read `return info.Mode()&0111 != 0` — the exact defect v0.16.2's own commit message spends three paragraphs on — so every consumer resolving through Program.Find kept failing with `Program.Find: "git": not found in PATH`, on a release that claimed the opposite. go-inference's windows lane measured it: 41 occurrences before v0.16.2, 41 after. A fix is not landed until you have grepped for its siblings. Byte-identical duplicates are how a fixed defect stays live. So the fix is not a second copy of the fix. internal/lookpath now holds ONE implementation and both consumers call it — because the duplication IS the defect's cause, and patching in place would guarantee a third divergence. The three Windows defects it carries are unchanged from v0.16.2: no %PATHEXT% expansion, a mode&0111 test the platform can never satisfy, and a path-vs-name check that missed '/'. commandContext no longer discards the resolution failure either. It records it on Cmd.Err exactly as exec.Command does with its own LookPath error — Start returns it without running anything. Deferring to Start rather than returning early is deliberate: an early return skipped Service.start's exited-event broadcast, which TestService_Actions/broadcasts_exited_event_on_start_failure caught. The callers' failure handling is untouched. Receipts — macOS, GOWORK=off (what CI runs): go test -count=1 ./... ok process 13.478s · exec 0.733s · internal/lookpath 0.499s · pkg/api 2.800s golangci-lint run ./... 0 issues gofmt -l · go vet: clean TestProgram_Find_UsesSharedResolution pins the miss itself: Program.Find must land on the same path the shared resolver reports. It fails the moment a third copy appears or the two drift apart again — which a version bump could not detect and did not. The resolution tests move with the code into internal/lookpath, keeping the fixture-PATH + fake-%PATHEXT% receipts that prove the Windows rules on a POSIX runner.
1 parent 8e0532a commit 1ca6201

8 files changed

Lines changed: 562 additions & 509 deletions

File tree

go.work.sum

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,12 +22,16 @@ github.com/youmark/pkcs8 v0.0.0-20240726163527-a2c0da244d78 h1:ilQV1hzziu+LLM3zU
2222
github.com/youmark/pkcs8 v0.0.0-20240726163527-a2c0da244d78/go.mod h1:aL8wCCfTfSfmXjznFBSZNN13rSJjlIOI1fUNAtF7rmI=
2323
golang.org/x/mod v0.32.0 h1:9F4d3PHLljb6x//jOyokMv3eX+YDeepZSEo3mFJy93c=
2424
golang.org/x/mod v0.32.0/go.mod h1:SgipZ/3h2Ci89DlEtEXWUk/HteuRin+HHhN+WbNhguU=
25+
golang.org/x/mod v0.37.0/go.mod h1:m8S8VeM9r4dzDwjrKO0a1sZP3YjeMamRRlD+fmR2Q/0=
2526
golang.org/x/sync v0.19.0 h1:vV+1eWNmZ5geRlYjzm2adRgW2/mcpevXNg50YZtPCE4=
2627
golang.org/x/sync v0.19.0/go.mod h1:9KTHXmSnoGruLpwFjVSX0lNNA75CykiMECbovNTZqGI=
28+
golang.org/x/sync v0.22.0/go.mod h1:9xrNwdLfx4jkKbNva9FpL6vEN7evnE43NNNJQ2LF3+0=
2729
golang.org/x/term v0.40.0 h1:36e4zGLqU4yhjlmxEaagx2KuYbJq3EwY8K943ZsHcvg=
2830
golang.org/x/term v0.40.0/go.mod h1:w2P8uVp06p2iyKKuvXIm7N/y0UCRt3UfJTfZ7oOpglM=
31+
golang.org/x/term v0.45.0/go.mod h1:9aqxs0blBcrm/n0L9QW0aRVD+ktan8ssZromtqJC43w=
2932
golang.org/x/tools v0.41.0 h1:a9b8iMweWG+S0OBnlU36rzLp20z1Rp10w+IY2czHTQc=
3033
golang.org/x/tools v0.41.0/go.mod h1:XSY6eDqxVNiYgezAVqqCeihT4j1U2CCsqvH3WhQpnlg=
34+
golang.org/x/tools v0.47.0/go.mod h1:dFHnyTvFWY212G+h7ZY4Vsp/K3U4/7W9TyVaAul8uCA=
3135
gopkg.in/check.v1 v1.0.0-20201130134442-10cb98267c6c h1:Hei/4ADfdWqJk1ZMxUNpqntNwaWcugrBjAiHlqqRiVk=
3236
gopkg.in/check.v1 v1.0.0-20201130134442-10cb98267c6c/go.mod h1:JHkPIbrfpd72SG/EVd6muEfDQjcINNoR0C8j2r3qZ4Q=
3337
rsc.io/pdf v0.1.1 h1:k1MczvYDUvJBe93bYd7wrZLLUEcLZAuF824/I4e5Xr4=

go/exec/exec.go

Lines changed: 2 additions & 154 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ import (
44
"context"
55

66
core "dappco.re/go"
7+
"dappco.re/go/process/internal/lookpath"
78
goio "io"
89
)
910

@@ -298,7 +299,7 @@ func (c *Cmd) logError(msg string, failure core.Result) {
298299
// "git" is reported as `exec: "C:\...\some work dir\git"` — a confusing error
299300
// naming a path nobody asked for, in place of the honest "not found on PATH".
300301
func commandContext(ctx context.Context, name string, arg ...string) core.Result {
301-
resolved := lookPath(name)
302+
resolved := lookpath.Look(name)
302303
if !resolved.OK {
303304
return resolved
304305
}
@@ -307,156 +308,3 @@ func commandContext(ctx context.Context, name string, arg ...string) core.Result
307308
Args: append([]string{name}, arg...),
308309
})
309310
}
310-
311-
// defaultPathExt is the extension list Windows itself assumes when %PATHEXT%
312-
// is unset.
313-
const defaultPathExt = ".COM;.EXE;.BAT;.CMD"
314-
315-
// lookPath resolves file to a runnable path, searching PATH when file carries
316-
// no directory component.
317-
//
318-
// On Windows a command is named without its extension — "git", not "git.exe" —
319-
// so every candidate is also tried with each %PATHEXT% suffix. Without that,
320-
// no Windows executable is ever found by its bare name.
321-
func lookPath(file string) core.Result {
322-
return lookPathWith(file, executableExtensions())
323-
}
324-
325-
// lookPathWith is lookPath with the extension list supplied rather than read
326-
// from the environment. Taking it as an argument is what lets the Windows
327-
// resolution rules be pinned on a POSIX runner — the tests drive it with a
328-
// fixture PATH and a fake %PATHEXT%, so no Windows box is needed to prove the
329-
// logic and the CI lane is left to prove only the wiring.
330-
func lookPathWith(file string, extensions []string) core.Result {
331-
if file == "" {
332-
return core.Fail(core.E("lookPath", "executable file not found in PATH", nil))
333-
}
334-
if containsSeparator(file) {
335-
if path, ok := firstExecutable(file, extensions); ok {
336-
return core.Ok(path)
337-
}
338-
return core.Fail(core.E("lookPath", core.Sprintf("executable file %q not found", file), nil))
339-
}
340-
341-
for _, dir := range core.Split(core.Getenv("PATH"), string(core.PathListSeparator)) {
342-
if dir == "" {
343-
dir = "."
344-
}
345-
if path, ok := firstExecutable(core.PathJoin(dir, file), extensions); ok {
346-
return core.Ok(path)
347-
}
348-
}
349-
return core.Fail(core.E("lookPath", core.Sprintf("executable file %q not found in PATH", file), nil))
350-
}
351-
352-
// firstExecutable returns the first of base's candidate spellings that names a
353-
// runnable file.
354-
func firstExecutable(base string, extensions []string) (string, bool) {
355-
for _, candidate := range executableCandidates(base, extensions) {
356-
if isExecutableWith(candidate, extensions) {
357-
return candidate, true
358-
}
359-
}
360-
return "", false
361-
}
362-
363-
// executableCandidates returns the spellings of base to try, in order. With no
364-
// extensions in play — every POSIX case — base stands alone. On Windows a base
365-
// that already ends in a listed extension also stands alone; anything else is
366-
// tried once per extension, so "git" becomes "git.com", "git.exe" and so on.
367-
func executableCandidates(base string, extensions []string) []string {
368-
if len(extensions) == 0 || hasExecutableExtension(base, extensions) {
369-
return []string{base}
370-
}
371-
candidates := make([]string, 0, len(extensions))
372-
for _, extension := range extensions {
373-
candidates = append(candidates, base+extension)
374-
}
375-
return candidates
376-
}
377-
378-
// hasExecutableExtension reports whether base already ends in one of the
379-
// listed extensions. Windows filenames are case-insensitive, so the comparison
380-
// is too.
381-
func hasExecutableExtension(base string, extensions []string) bool {
382-
lowered := core.Lower(base)
383-
for _, extension := range extensions {
384-
if core.HasSuffix(lowered, extension) {
385-
return true
386-
}
387-
}
388-
return false
389-
}
390-
391-
// executableExtensions returns the %PATHEXT% list, or nil off Windows where a
392-
// command name is used exactly as written.
393-
func executableExtensions() []string {
394-
if string(core.PathSeparator) != `\` {
395-
return nil
396-
}
397-
return parsePathExt(core.Getenv("PATHEXT"))
398-
}
399-
400-
// parsePathExt normalises a %PATHEXT% value into lower-cased, dot-prefixed
401-
// extensions, dropping blanks and duplicates. An unset or unusable value falls
402-
// back to the set Windows assumes, so a stripped environment still resolves
403-
// the common executables.
404-
func parsePathExt(value string) []string {
405-
extensions := make([]string, 0, 8)
406-
seen := make(map[string]bool, 8)
407-
for _, field := range core.Split(value, ";") {
408-
extension := core.Lower(core.Trim(field))
409-
if extension == "" || extension == "." {
410-
continue
411-
}
412-
if !core.HasPrefix(extension, ".") {
413-
extension = "." + extension
414-
}
415-
if seen[extension] {
416-
continue
417-
}
418-
seen[extension] = true
419-
extensions = append(extensions, extension)
420-
}
421-
if len(extensions) == 0 && value != defaultPathExt {
422-
return parsePathExt(defaultPathExt)
423-
}
424-
return extensions
425-
}
426-
427-
// containsSeparator reports whether file carries a directory component under
428-
// either convention. Windows accepts '/' as well as '\', so a name spelled
429-
// "bin/tool" there is a path to check directly, not a name to hunt on PATH.
430-
func containsSeparator(file string) bool {
431-
if core.Contains(file, "/") {
432-
return true
433-
}
434-
separator := string(core.PathSeparator)
435-
return separator != "/" && core.Contains(file, separator)
436-
}
437-
438-
func isExecutable(path string) bool {
439-
return isExecutableWith(path, executableExtensions())
440-
}
441-
442-
// isExecutableWith applies the platform's own rule for "this can be run".
443-
//
444-
// POSIX asks the mode bits. Windows has no execute bit — os.Stat synthesises
445-
// 0666, or 0444 for a read-only file — so mode&0111 is never set there and a
446-
// mode test rejects every file, git.exe included. Under a non-empty extension
447-
// list the question becomes whether the suffix is one %PATHEXT% names, which
448-
// is what Windows itself keys on.
449-
func isExecutableWith(path string, extensions []string) bool {
450-
stat := core.Stat(path)
451-
if !stat.OK {
452-
return false
453-
}
454-
info, ok := stat.Value.(core.FsFileInfo)
455-
if !ok || info.IsDir() {
456-
return false
457-
}
458-
if len(extensions) > 0 {
459-
return hasExecutableExtension(path, extensions)
460-
}
461-
return info.Mode()&0111 != 0
462-
}

0 commit comments

Comments
 (0)