diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 6089f7081..b6b146378 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -131,6 +131,18 @@ jobs: with: go-version-file: go.mod - run: go build -o gortex ./cmd/gortex/ + # The unused-method check from build-linux-static, linked the way + # release.yml links these platforms' binaries: no build tags, and on + # windows the mingw runtime linked in statically. Compilation is cached + # from the build above, so this costs one more link. + - name: Keep unused methods prunable + shell: bash + run: | + set -o pipefail + extld= + if [ "$RUNNER_OS" = Windows ]; then extld=-extldflags=-static; fi + go build -ldflags "-s -w $extld -dumpdep" \ + -o "$RUNNER_TEMP/gortex-dumpdep" ./cmd/gortex/ 2>&1 | go run ./scripts/ci-dumpdep smoke: needs: plan @@ -183,6 +195,22 @@ jobs: file gortex-static sh scripts/verify-static-elf.sh gortex-static + # Executing a text/template or html/template, like any other reflect + # method lookup by a run-time name, makes the linker keep every exported + # method of every reachable type: ~6 MB of the stripped binary when the + # service unit files were rendered with text/template. Relink (the + # compile above is cached) with -dumpdep, which lists what the linker + # keeps and why; scripts/ci-dumpdep fails with the call chain that + # reaches such a lookup. + - name: Keep unused methods prunable + env: + CGO_ENABLED: "1" + run: | + set -o pipefail + go build -tags netgo,osusergo,static_link \ + -ldflags '-s -w -extldflags "-static" -dumpdep' \ + -o /dev/null ./cmd/gortex/ 2>&1 | go run ./scripts/ci-dumpdep + # The static-only code path (custom grammars declining instead of # dlopen'ing) is compiled out of every other job, so it needs its tests # run under the tag or they never execute anywhere. diff --git a/cmd/gortex/daemon_service.go b/cmd/gortex/daemon_service.go index 663408f26..152e9aa68 100644 --- a/cmd/gortex/daemon_service.go +++ b/cmd/gortex/daemon_service.go @@ -1,7 +1,6 @@ package main import ( - "bytes" "encoding/xml" "errors" "fmt" @@ -12,7 +11,6 @@ import ( "runtime" "slices" "strings" - "text/template" "github.com/spf13/cobra" @@ -255,15 +253,18 @@ func runDaemonServiceStatus(cmd *cobra.Command, _ []string) error { // EnvironmentVariables carries the installing shell's PATH with missing // Homebrew / system defaults appended, plus any XDG_* overrides in effect at // install time, and GORTEX_DAEMON_LOGFILE — see servicePath and xdgServiceEnv. +// +// The verbs take, in order: label, executable, log path (twice), PATH, and +// the rendered / lines of the other variables. const launchdPlistTemplate = ` Label - {{.Label}} + %s ProgramArguments - {{.Exe}} + %s daemon start @@ -275,17 +276,13 @@ const launchdPlistTemplate = ` StandardOutPath - {{.LogPath}} + %s StandardErrorPath - {{.LogPath}} + %s EnvironmentVariables PATH - {{.Path}} -{{- range .EnvVars}} - {{.Key}} - {{.Value}} -{{- end}} + %s%s @@ -294,25 +291,18 @@ const launchdPlistTemplate = ` // renderLaunchdPlist fills launchdPlistTemplate. String values are // XML-escaped so a path containing an XML metacharacter can't produce a // malformed, unloadable plist. -func renderLaunchdPlist(label, exe, logPath string, env []serviceEnvVar) (string, error) { - data := struct { - Label, Exe, LogPath, Path string - EnvVars []serviceEnvVar - }{ - Label: xmlEscape(label), - Exe: xmlEscape(exe), - LogPath: xmlEscape(logPath), - Path: xmlEscape(servicePath([]string{"/usr/local/bin", "/opt/homebrew/bin", "/usr/bin", "/bin"})), - EnvVars: make([]serviceEnvVar, len(env)), - } - for i, e := range env { - data.EnvVars[i] = serviceEnvVar{Key: e.Key, Value: xmlEscape(e.Value)} - } - var buf bytes.Buffer - if err := template.Must(template.New("plist").Parse(launchdPlistTemplate)).Execute(&buf, data); err != nil { - return "", err +// +// Unit files are rendered with fmt, not text/template: executing a template +// looks fields up by reflection, which makes the linker keep every exported +// method of every reachable type (see scripts/ci-dumpdep). +func renderLaunchdPlist(label, exe, logPath string, env []serviceEnvVar) string { + var vars strings.Builder + for _, e := range env { + fmt.Fprintf(&vars, "\n %s\n %s", e.Key, xmlEscape(e.Value)) } - return buf.String(), nil + logPath = xmlEscape(logPath) + path := servicePath([]string{"/usr/local/bin", "/opt/homebrew/bin", "/usr/bin", "/bin"}) + return fmt.Sprintf(launchdPlistTemplate, xmlEscape(label), xmlEscape(exe), logPath, logPath, xmlEscape(path), vars.String()) } func launchdPlistPath() (string, error) { @@ -337,10 +327,7 @@ func installLaunchd(w io.Writer, exe string) error { return err } stderrLog := daemon.StderrLogPath(logFile) - plist, err := renderLaunchdPlist(daemonServiceName, exe, stderrLog, xdgServiceEnv(logFile)) - if err != nil { - return fmt.Errorf("render plist: %w", err) - } + plist := renderLaunchdPlist(daemonServiceName, exe, stderrLog, xdgServiceEnv(logFile)) if err := os.WriteFile(path, []byte(plist), 0o644); err != nil { return fmt.Errorf("write plist: %w", err) } @@ -625,6 +612,9 @@ func noteRunningDaemon(w io.Writer, daemonRunning func() bool) { // PATH, XDG_* overrides, and GORTEX_DAEMON_LOGFILE at install time so the // daemon resolves the same paths as the installing shell — see servicePath // and xdgServiceEnv. +// +// The verbs take, in order: executable, the rendered Environment= lines, and +// the log path (twice). const systemdUnitTemplate = `[Unit] Description=Gortex code intelligence daemon Documentation=https://github.com/zzet/gortex @@ -632,14 +622,11 @@ After=network.target [Service] Type=simple -ExecStart={{.Exe}} daemon start -{{- range .EnvVars}} -Environment={{.}} -{{- end}} +ExecStart=%s daemon start%s Restart=on-failure RestartSec=2 -StandardOutput=append:{{.LogPath}} -StandardError=append:{{.LogPath}} +StandardOutput=append:%s +StandardError=append:%s [Install] WantedBy=default.target @@ -647,28 +634,19 @@ WantedBy=default.target // renderSystemdUnit fills systemdUnitTemplate, quoting Environment= // assignments that need it. -func renderSystemdUnit(exe, logPath string, env []serviceEnvVar) (string, error) { +func renderSystemdUnit(exe, logPath string, env []serviceEnvVar) string { if path := servicePath(nil); path != "" { env = append([]serviceEnvVar{{Key: "PATH", Value: path}}, env...) } - data := struct { - Exe, LogPath string - EnvVars []string - }{ - Exe: exe, - // systemd expands % specifiers in StandardOutput= and StandardError= - // paths too, so escape them as systemdEnvValue does. - LogPath: strings.ReplaceAll(logPath, "%", "%%"), - EnvVars: make([]string, len(env)), - } - for i, e := range env { - data.EnvVars[i] = systemdEnvValue(e.Key + "=" + e.Value) - } - var buf bytes.Buffer - if err := template.Must(template.New("unit").Parse(systemdUnitTemplate)).Execute(&buf, data); err != nil { - return "", err + var vars strings.Builder + for _, e := range env { + vars.WriteString("\nEnvironment=") + vars.WriteString(systemdEnvValue(e.Key + "=" + e.Value)) } - return buf.String(), nil + // systemd expands % specifiers in StandardOutput= and StandardError= + // paths too, so escape them as systemdEnvValue does. + logPath = strings.ReplaceAll(logPath, "%", "%%") + return fmt.Sprintf(systemdUnitTemplate, exe, vars.String(), logPath, logPath) } func systemdUnitPath() (string, error) { @@ -693,10 +671,7 @@ func installSystemd(w io.Writer, exe string) error { return err } stderrLog := daemon.StderrLogPath(logFile) - unit, err := renderSystemdUnit(exe, stderrLog, xdgServiceEnv(logFile)) - if err != nil { - return fmt.Errorf("render unit: %w", err) - } + unit := renderSystemdUnit(exe, stderrLog, xdgServiceEnv(logFile)) if err := os.WriteFile(path, []byte(unit), 0o644); err != nil { return fmt.Errorf("write unit: %w", err) } diff --git a/cmd/gortex/daemon_service_render_test.go b/cmd/gortex/daemon_service_render_test.go new file mode 100644 index 000000000..ea3a1563f --- /dev/null +++ b/cmd/gortex/daemon_service_render_test.go @@ -0,0 +1,129 @@ +package main + +import ( + "os" + "strings" + "testing" + + "github.com/stretchr/testify/require" +) + +// These pin whole unit files, byte for byte, so a change to how they are +// rendered cannot move a newline, an indent or an escape unnoticed. The +// render tests in daemon_service_test.go check how the installing shell's +// PATH and daemon log override reach them. + +// launchdDefaultPATH is the plist PATH when the installing shell's PATH +// contributes nothing: the launchd defaults, joined with the host's list +// separator. +func launchdDefaultPATH() string { + return strings.Join([]string{"/usr/local/bin", "/opt/homebrew/bin", "/usr/bin", "/bin"}, string(os.PathListSeparator)) +} + +func TestRenderLaunchdPlist_Golden(t *testing.T) { + const head = ` + + + + Label + com.zzet.gortex + ProgramArguments + + /opt/a&b/gortex + daemon + start + + RunAtLoad + + KeepAlive + + SuccessfulExit + + + StandardOutPath + /var/o'neil/<logs>/daemon.stderr.log + StandardErrorPath + /var/o'neil/<logs>/daemon.stderr.log + EnvironmentVariables + + PATH + ` + const tail = ` + + + +` + for _, tc := range []struct { + name string + env []serviceEnvVar + vars string + }{ + {name: "no env"}, + { + name: "env", + env: []serviceEnvVar{ + {Key: "XDG_CONFIG_HOME", Value: `/srv/a&b/"config"`}, + {Key: "GORTEX_DAEMON_LOGFILE", Value: "/srv/a&b/daemon.log"}, + }, + vars: ` + XDG_CONFIG_HOME + /srv/a&b/"config" + GORTEX_DAEMON_LOGFILE + /srv/a&b/daemon.log`, + }, + } { + t.Run(tc.name, func(t *testing.T) { + t.Setenv("PATH", "") + out := renderLaunchdPlist("com.zzet.gortex", "/opt/a&b/gortex", "/var/o'neil//daemon.stderr.log", tc.env) + require.Equal(t, head+launchdDefaultPATH()+""+tc.vars+tail, out) + assertWellFormedXML(t, out) + }) + } +} + +func TestRenderSystemdUnit_Golden(t *testing.T) { + const head = `[Unit] +Description=Gortex code intelligence daemon +Documentation=https://github.com/zzet/gortex +After=network.target + +[Service] +Type=simple +ExecStart=/srv/u/.local/bin/gortex daemon start` + const tail = ` +Restart=on-failure +RestartSec=2 +StandardOutput=append:/srv/u/100%% logs/daemon.stderr.log +StandardError=append:/srv/u/100%% logs/daemon.stderr.log + +[Install] +WantedBy=default.target +` + for _, tc := range []struct { + name string + env []serviceEnvVar + vars string + }{ + {name: "no env"}, + { + name: "env", + env: []serviceEnvVar{ + {Key: "XDG_CACHE_HOME", Value: "/srv/u/.cache"}, + {Key: "XDG_DATA_HOME", Value: "/srv/u/has space"}, + {Key: "XDG_CONFIG_HOME", Value: `/srv/u/"q"\x`}, + {Key: "GORTEX_DAEMON_LOGFILE", Value: "/srv/u/100% logs/daemon.log"}, + }, + vars: ` +Environment=XDG_CACHE_HOME=/srv/u/.cache +Environment="XDG_DATA_HOME=/srv/u/has space" +Environment="XDG_CONFIG_HOME=/srv/u/\"q\"\\x" +Environment="GORTEX_DAEMON_LOGFILE=/srv/u/100%% logs/daemon.log"`, + }, + } { + t.Run(tc.name, func(t *testing.T) { + t.Setenv("PATH", "") + out := renderSystemdUnit("/srv/u/.local/bin/gortex", "/srv/u/100% logs/daemon.stderr.log", tc.env) + require.Equal(t, head+tc.vars+tail, out) + }) + } +} diff --git a/cmd/gortex/daemon_service_test.go b/cmd/gortex/daemon_service_test.go index b8fc9a8dd..52e72c7dc 100644 --- a/cmd/gortex/daemon_service_test.go +++ b/cmd/gortex/daemon_service_test.go @@ -31,122 +31,6 @@ func assertWellFormedXML(t *testing.T, s string) { } } -// TestRenderLaunchdPlist_NoXDG proves the launchd plist is valid and -// carries the substitutions it must — binary path, log path, label — -// and that with no XDG override captured it has no extra env entries -// (the pre-fix baseline behaviour, unchanged for the common case). -func TestRenderLaunchdPlist_NoXDG(t *testing.T) { - out, err := renderLaunchdPlist( - "com.zzet.gortex", - "/usr/local/bin/gortex", - "/Users/testuser/.gortex/cache/daemon.stderr.log", - nil, - ) - require.NoError(t, err) - - assert.Contains(t, out, "Label\n com.zzet.gortex") - assert.Contains(t, out, "/usr/local/bin/gortex") - assert.Contains(t, out, "daemon") - assert.Contains(t, out, "start") - assert.Contains(t, out, "RunAtLoad") - assert.Contains(t, out, "KeepAlive") - assert.Contains(t, out, "StandardOutPath\n /Users/testuser/.gortex/cache/daemon.stderr.log") - assert.Contains(t, out, "StandardErrorPath\n /Users/testuser/.gortex/cache/daemon.stderr.log") - // Homebrew paths must be on PATH so launchd can find a Homebrew- - // installed gortex binary even when the LaunchAgent env is minimal. - assert.Contains(t, out, "/opt/homebrew/bin") - assert.NotContains(t, out, "XDG_", "no env entries when none captured") - assertWellFormedXML(t, out) -} - -// TestRenderLaunchdPlist_PropagatesXDG is the regression test for the -// service-ignores-XDG bug: when an XDG override is in effect at install -// time it must be baked into the plist so the supervised daemon resolves -// the same paths as the installing shell instead of falling back to -// ~/.gortex. -func TestRenderLaunchdPlist_PropagatesXDG(t *testing.T) { - env := []serviceEnvVar{ - {Key: "XDG_CONFIG_HOME", Value: "/Users/testuser/.config"}, - {Key: "XDG_DATA_HOME", Value: "/Users/testuser/.local/share"}, - } - out, err := renderLaunchdPlist( - "com.zzet.gortex", - "/usr/local/bin/gortex", - "/Users/testuser/.config/gortex/daemon.stderr.log", - env, - ) - require.NoError(t, err) - - assert.Contains(t, out, "XDG_CONFIG_HOME\n /Users/testuser/.config") - assert.Contains(t, out, "XDG_DATA_HOME\n /Users/testuser/.local/share") - // PATH must still be present alongside the captured XDG vars. - assert.Contains(t, out, "PATH") - assertWellFormedXML(t, out) -} - -// TestRenderLaunchdPlist_EscapesXML guards against a home path with an -// XML metacharacter producing a malformed, unloadable plist. -func TestRenderLaunchdPlist_EscapesXML(t *testing.T) { - env := []serviceEnvVar{{Key: "XDG_CONFIG_HOME", Value: "/Users/a&b/.config"}} - out, err := renderLaunchdPlist("com.zzet.gortex", "/opt/a&b/gortex", "/log&path", env) - require.NoError(t, err) - - assert.Contains(t, out, "/Users/a&b/.config") - assert.Contains(t, out, "/opt/a&b/gortex") - assert.NotContains(t, out, "a&b", "raw ampersand must be escaped") - assertWellFormedXML(t, out) -} - -// TestRenderSystemdUnit_NoXDG confirms the Type=simple + Restart=on-failure -// contract and that no Environment= line is emitted when nothing was -// captured. -func TestRenderSystemdUnit_NoXDG(t *testing.T) { - t.Setenv("PATH", "") - out, err := renderSystemdUnit( - "/home/u/.local/bin/gortex", - "/home/u/.gortex/cache/daemon.stderr.log", - nil, - ) - require.NoError(t, err) - - assert.Contains(t, out, "ExecStart=/home/u/.local/bin/gortex daemon start") - assert.Contains(t, out, "Type=simple") - assert.Contains(t, out, "Restart=on-failure") - assert.Contains(t, out, "StandardOutput=append:/home/u/.gortex/cache/daemon.stderr.log") - assert.Contains(t, out, "StandardError=append:/home/u/.gortex/cache/daemon.stderr.log") - assert.Contains(t, out, "WantedBy=default.target") - assert.NotContains(t, out, "Environment=", "no env line when none captured") -} - -// TestRenderSystemdUnit_PropagatesXDG is the Linux analog of the launchd -// regression test, and also covers systemd quoting for a value with a -// space. -func TestRenderSystemdUnit_PropagatesXDG(t *testing.T) { - env := []serviceEnvVar{ - {Key: "XDG_CACHE_HOME", Value: "/home/u/.cache"}, - {Key: "XDG_DATA_HOME", Value: "/home/u/has space"}, - } - out, err := renderSystemdUnit( - "/home/u/.local/bin/gortex", - "/home/u/.cache/gortex/daemon.stderr.log", - env, - ) - require.NoError(t, err) - - assert.Contains(t, out, "Environment=XDG_CACHE_HOME=/home/u/.cache") - // An assignment containing whitespace is double-quoted as a whole, - // because systemd recognizes a quote only at the start of an item. - assert.Contains(t, out, `Environment="XDG_DATA_HOME=/home/u/has space"`) - - // Environment lines must sit inside [Service], ahead of [Install]. - svcStart := strings.Index(out, "[Service]") - installStart := strings.Index(out, "[Install]") - require.Greater(t, svcStart, -1) - require.Greater(t, installStart, svcStart) - svc := out[svcStart:installStart] - assert.Contains(t, svc, "Environment=XDG_CACHE_HOME=") -} - // TestXDGServiceEnv_OnlyAbsoluteSet pins the capture contract: only XDG // vars that are set to an absolute path are propagated. Unset and // relative values are ignored (the latter per the XDG spec, matching @@ -226,8 +110,7 @@ func TestRenderLaunchdPlist_CapturesPATH(t *testing.T) { custom := filepath.Join(t.TempDir(), "a&b", "100% tools") t.Setenv("PATH", custom) t.Setenv("GORTEX_DAEMON_HTTP_TOKEN", "must-not-be-captured") - out, err := renderLaunchdPlist("com.zzet.gortex", "/bin/gortex", "/log", nil) - require.NoError(t, err) + out := renderLaunchdPlist("com.zzet.gortex", "/bin/gortex", "/log", nil) want := strings.ReplaceAll(custom, "&", "&") + sep + strings.Join([]string{"/usr/local/bin", "/opt/homebrew/bin", "/usr/bin", "/bin"}, sep) @@ -240,8 +123,7 @@ func TestRenderLaunchdPlist_CapturesPATH(t *testing.T) { func TestRenderLaunchdPlist_EmptyPATH(t *testing.T) { t.Setenv("PATH", "") - out, err := renderLaunchdPlist("com.zzet.gortex", "/bin/gortex", "/log", nil) - require.NoError(t, err) + out := renderLaunchdPlist("com.zzet.gortex", "/bin/gortex", "/log", nil) // This is exactly the old PATH on launchd's Unix host; adapt only the // path-list separator when these render tests run on Windows. @@ -258,8 +140,7 @@ func TestRenderSystemdUnit_CapturesPATH(t *testing.T) { second := filepath.Join(root, "bin") t.Setenv("PATH", first+sep+second) t.Setenv("GORTEX_DAEMON_HTTP_TOKEN", "must-not-be-captured") - out, err := renderSystemdUnit("/bin/gortex", "/log", nil) - require.NoError(t, err) + out := renderSystemdUnit("/bin/gortex", "/log", nil) // systemd quotes whitespace and escapes backslashes within a quoted // value (relevant to this native fixture when tests run on Windows). @@ -276,8 +157,7 @@ func TestRenderSystemdUnit_EmptyPATH(t *testing.T) { for _, path := range []string{"", ".", string(os.PathListSeparator) + "relative/bin"} { t.Run(path, func(t *testing.T) { t.Setenv("PATH", path) - out, err := renderSystemdUnit("/bin/gortex", "/log", nil) - require.NoError(t, err) + out := renderSystemdUnit("/bin/gortex", "/log", nil) assert.NotContains(t, out, "PATH=") }) } @@ -392,8 +272,7 @@ func TestRenderLaunchdPlist_DaemonLogOverride(t *testing.T) { for _, value := range []string{"", "/custom/a&b/daemon.log"} { t.Run(value, func(t *testing.T) { t.Setenv("GORTEX_DAEMON_LOGFILE", value) - out, err := renderLaunchdPlist("com.zzet.gortex", "/bin/gortex", daemon.StderrLogFilePath(), xdgServiceEnv(daemon.LogFilePath())) - require.NoError(t, err) + out := renderLaunchdPlist("com.zzet.gortex", "/bin/gortex", daemon.StderrLogFilePath(), xdgServiceEnv(daemon.LogFilePath())) assertWellFormedXML(t, out) if value == "" { assert.NotContains(t, out, "GORTEX_DAEMON_LOGFILE") @@ -409,8 +288,7 @@ func TestRenderSystemdUnit_DaemonLogOverride(t *testing.T) { for _, value := range []string{"", "/custom/100% logs/daemon.log"} { t.Run(value, func(t *testing.T) { t.Setenv("GORTEX_DAEMON_LOGFILE", value) - out, err := renderSystemdUnit("/bin/gortex", daemon.StderrLogFilePath(), xdgServiceEnv(daemon.LogFilePath())) - require.NoError(t, err) + out := renderSystemdUnit("/bin/gortex", daemon.StderrLogFilePath(), xdgServiceEnv(daemon.LogFilePath())) if value == "" { assert.NotContains(t, out, "GORTEX_DAEMON_LOGFILE") } else { diff --git a/docs/ci-testing.md b/docs/ci-testing.md index d3304050b..9ac4ec869 100644 --- a/docs/ci-testing.md +++ b/docs/ci-testing.md @@ -93,8 +93,9 @@ A small Linux race smoke uses the existing production wiring tests `TestDaemon_EndToEnd_ToolHandlerCanReturnItsDispatchSlot` and `TestComposeWiresTheLeaseManagerIntoTheRESTSurface` to supplement imports with runtime handler/transport coverage. All-OS default builds remain enabled (Linux -in the existing `build-onnx` job), along with static/tagged builds, complete lint, -module tidiness, CGO flag checks, benchmarks and the separate security workflows. +in the existing `build-onnx` job), along with static/tagged builds, a link check +that unused methods stay prunable (`scripts/ci-dumpdep`), complete lint, module +tidiness, CGO flag checks, benchmarks and the separate security workflows. Only full-suite Linux coverage goes to Codecov. Affected-run coverage is kept in explicitly named `affected-coverage-*` artifacts and is never presented as diff --git a/internal/graph/stub.go b/internal/graph/stub.go index ba3573fdf..a5888af18 100644 --- a/internal/graph/stub.go +++ b/internal/graph/stub.go @@ -73,11 +73,9 @@ func IsStub(id string) bool { // We match by looking for one of the known kind segments // anywhere in the first two "::"-separated positions. func StubKind(id string) string { - for _, k := range stubKinds { - // Without repo prefix: "::..." - if strings.HasPrefix(id, k+"::") { - return k - } + // Without repo prefix: "::..." + if k := stubKindSegment(id); k != "" { + return k } // With repo prefix: "::::..." // Find the second "::" segment. @@ -85,23 +83,27 @@ func StubKind(id string) string { if first < 0 { return "" } - rest := id[first+2:] - for _, k := range stubKinds { - if strings.HasPrefix(rest, k+"::") { - return k - } - } - return "" + return stubKindSegment(id[first+2:]) } -// stubKinds is the closed set of stub categories. Ordered by -// expected frequency so the lookup loop bails early in the -// common case. -var stubKinds = []string{ - StubKindStdlib, - StubKindExternalCall, - StubKindBuiltin, - StubKindModule, +// stubKindSegment returns the stub kind s starts with as a whole +// "::" segment, or "" if it starts with none. The resolver runs +// IsStub on every name-lookup candidate, so each "::" is a +// constant expression rather than a per-call k+"::" concatenation. +// Cases are ordered by expected frequency so the common case bails +// early. +func stubKindSegment(s string) string { + switch { + case strings.HasPrefix(s, StubKindStdlib+"::"): + return StubKindStdlib + case strings.HasPrefix(s, StubKindExternalCall+"::"): + return StubKindExternalCall + case strings.HasPrefix(s, StubKindBuiltin+"::"): + return StubKindBuiltin + case strings.HasPrefix(s, StubKindModule+"::"): + return StubKindModule + } + return "" } // IsStdlibStub etc are convenience predicates that don't make diff --git a/internal/graph/stub_test.go b/internal/graph/stub_test.go new file mode 100644 index 000000000..3e171103b --- /dev/null +++ b/internal/graph/stub_test.go @@ -0,0 +1,120 @@ +package graph + +import ( + "strings" + "testing" +) + +// stubKindByConcatenation is the original StubKind, which built +// k+"::" for every candidate kind on every call. It is the reference +// StubKind must keep agreeing with. +func stubKindByConcatenation(id string) string { + kinds := []string{StubKindStdlib, StubKindExternalCall, StubKindBuiltin, StubKindModule} + for _, k := range kinds { + if strings.HasPrefix(id, k+"::") { + return k + } + } + first := strings.Index(id, "::") + if first < 0 { + return "" + } + rest := id[first+2:] + for _, k := range kinds { + if strings.HasPrefix(rest, k+"::") { + return k + } + } + return "" +} + +// TestStubKind pins the stub-id grammar: a kind counts only as the +// first or second "::"-separated segment, and only when "::" follows it. +func TestStubKind(t *testing.T) { + t.Parallel() + + cases := []struct{ id, kind string }{ + // Legacy form, no repo prefix. + {"stdlib::fmt::Errorf", StubKindStdlib}, + {"external_call::requests.get", StubKindExternalCall}, + {"builtin::go::len", StubKindBuiltin}, + {"module::lodash", StubKindModule}, + // Per-repo form. + {"gortex::stdlib::fmt::Errorf", StubKindStdlib}, + {"gortex::external_call::requests.get", StubKindExternalCall}, + {"tree-sitter-dart::builtin::dart::print", StubKindBuiltin}, + {"gortex::module::lodash", StubKindModule}, + // The kind segment may end the id, and the repo prefix may be empty. + {"stdlib::", StubKindStdlib}, + {"gortex::module::", StubKindModule}, + {"::builtin::go::len", StubKindBuiltin}, + // A leading kind wins over a second-segment one. + {"builtin::stdlib::x", StubKindBuiltin}, + // Not stubs. + {"", ""}, + {"stdlib", ""}, + {"stdlib:", ""}, + {"stdlib:fmt", ""}, + {"gortex::stdlib", ""}, + {"gortex::stdlib:fmt", ""}, + {"Stdlib::fmt", ""}, + {"stdlibs::fmt", ""}, + {"xmodule::lodash", ""}, + {"a::b::stdlib::fmt", ""}, + {"gortex/internal/graph/stub.go::StubKind", ""}, + {"unresolved::Foo", ""}, + } + for _, c := range cases { + if got := StubKind(c.id); got != c.kind { + t.Errorf("StubKind(%q) = %q, want %q", c.id, got, c.kind) + } + if got := IsStub(c.id); got != (c.kind != "") { + t.Errorf("IsStub(%q) = %v, want %v", c.id, got, c.kind != "") + } + } +} + +// TestStubKindMatchesConcatenation checks StubKind against the +// concatenating original on every prefix of ids built from each kind, +// near-miss kinds and repo prefixes, so every truncation point around +// the "::" separators is covered. +func TestStubKindMatchesConcatenation(t *testing.T) { + t.Parallel() + + heads := []string{"", "gortex::", "::", ":", "a::b::", "stdlib::", "gortex/x.go:"} + kinds := []string{ + StubKindStdlib, StubKindExternalCall, StubKindBuiltin, StubKindModule, + "stdli", "stdlibs", "external", "Builtin", "unresolved", + } + tails := []string{"::fmt::Errorf", ":::x", "::", ":", "", "::module::x"} + for _, head := range heads { + for _, kind := range kinds { + for _, tail := range tails { + id := head + kind + tail + for i := 0; i <= len(id); i++ { + if got, want := StubKind(id[:i]), stubKindByConcatenation(id[:i]); got != want { + t.Errorf("StubKind(%q) = %q, want %q", id[:i], got, want) + } + } + } + } + } +} + +// BenchmarkStubKind measures the per-candidate stub check the resolver +// runs while filtering name-lookup results, which are mostly +// first-party ids. +func BenchmarkStubKind(b *testing.B) { + for _, bc := range []struct{ name, id string }{ + {"first_party", "gortex/internal/resolver/resolver.go::withoutAttributedTerminals"}, + {"repo_stub", "gortex::external_call::requests.get"}, + {"legacy_stub", "module::lodash"}, + } { + b.Run(bc.name, func(b *testing.B) { + b.ReportAllocs() + for b.Loop() { + StubKind(bc.id) + } + }) + } +} diff --git a/scripts/ci-dumpdep/main.go b/scripts/ci-dumpdep/main.go new file mode 100644 index 000000000..db13316b7 --- /dev/null +++ b/scripts/ci-dumpdep/main.go @@ -0,0 +1,148 @@ +// ci-dumpdep fails when a binary links code that makes the Go linker keep +// every exported method of every reachable type. +// +// The linker's dead-code pass drops methods that nothing can call. A call to +// reflect.Value.Method, or to MethodByName with a name that is not a +// compile-time constant, defeats it: the linker cannot know which method will +// be asked for, so it keeps all of them. Executing a text/template or +// html/template makes exactly that call to evaluate fields. Rendering the +// daemon's service unit files with text/template once kept about 6 MB of +// otherwise unused methods in the stripped gortex binary. +// +// It reads the reachability listing a -dumpdep link prints: +// +// go build -ldflags=-dumpdep -o /dev/null ./cmd/gortex/ 2>&1 | go run ./scripts/ci-dumpdep +// +// Each "parent -> symbol" line marks symbol reachable from parent. The linker +// appends to a function the compiler flagged as such a +// method lookup, and to a type converted to an interface. +// Every real program has such types, so a listing without that flag is not +// one this tool can read: it is rejected rather than reported clean. +package main + +import ( + "bufio" + "errors" + "fmt" + "io" + "os" + "strings" +) + +const ( + reflectMethodFlag = "" + usedInIfaceFlag = "" +) + +// templateExecution is the function every text/template and html/template +// execution runs through; Execute and ExecuteTemplate are wrappers the +// compiler may inline into their caller. It always reaches the flagged +// lookup, and reporting it puts the caller that executes a template at the +// end of the chain rather than template internals. +const templateExecution = "text/template.(*Template).execute" + +// finding is a reachable symbol that keeps all exported methods, with the +// chain of symbols that reached it, root first. +type finding struct { + symbol string + path []string +} + +// symbolName drops the attribute flags -dumpdep appends to a symbol. +func symbolName(s string) string { + for _, flag := range []string{reflectMethodFlag, usedInIfaceFlag} { + s = strings.TrimSuffix(s, flag) + } + return strings.TrimSuffix(s, " ") +} + +// maxOtherLines bounds how much non-listing input, such as a build failure, +// is echoed back when the input turns out not to be a link listing. +const maxOtherLines = 100 + +// check reads a -dumpdep listing and returns the reachable symbols that make +// the linker keep every exported method. Lines that are not dependency edges, +// such as the package header go build prints, are skipped. +func check(r io.Reader) ([]finding, error) { + parent := map[string]string{} + var offenders, other []string + flagged := 0 + scanner := bufio.NewScanner(r) + scanner.Buffer(make([]byte, 0, 64*1024), 1024*1024) + for scanner.Scan() { + from, to, ok := strings.Cut(scanner.Text(), " -> ") + if !ok { + if len(other) < maxOtherLines { + other = append(other, scanner.Text()) + } + continue + } + if strings.HasSuffix(to, usedInIfaceFlag) { + flagged++ + } + name := symbolName(to) + if _, seen := parent[name]; !seen { + parent[name] = symbolName(from) + } + if strings.HasSuffix(to, reflectMethodFlag) || name == templateExecution { + offenders = append(offenders, name) + } + } + if err := scanner.Err(); err != nil { + return nil, err + } + if _, ok := parent["main.main"]; !ok { + msg := "main.main is not in the input; pipe in the output of a -dumpdep link of a command" + if len(other) > 0 { + msg += ". The input began:\n" + strings.Join(other, "\n") + } + return nil, errors.New(msg) + } + // Without the attribute flags a could not be seen + // either, so a listing without them must not pass as clean. + if flagged == 0 { + return nil, errors.New("no symbol carries the " + usedInIfaceFlag + " flag, so this is not a -dumpdep listing this tool can read") + } + findings := make([]finding, 0, len(offenders)) + for _, name := range offenders { + path := []string{name} + for at := name; len(path) <= len(parent); { + from, ok := parent[at] + if !ok || from == "_" { + break + } + path = append(path, from) + at = from + } + for i, j := 0, len(path)-1; i < j; i, j = i+1, j-1 { + path[i], path[j] = path[j], path[i] + } + findings = append(findings, finding{symbol: name, path: path}) + } + return findings, nil +} + +func run(stdin io.Reader, stdout, stderr io.Writer) int { + findings, err := check(stdin) + if err != nil { + fmt.Fprintf(stderr, "ci-dumpdep: %v\n", err) + return 1 + } + if len(findings) == 0 { + fmt.Fprintln(stdout, "No reachable dynamic method lookup: the linker can drop unused methods.") + return 0 + } + fmt.Fprintf(stderr, "ci-dumpdep: %d reachable symbols make the linker keep every exported method of every reachable type.\n", len(findings)) + fmt.Fprintln(stderr, "Render text with fmt or strings rather than text/template or html/template, and look methods up by constant name.") + for _, f := range findings { + fmt.Fprintf(stderr, "\n%s, reached by:\n", f.symbol) + for _, hop := range f.path { + fmt.Fprintf(stderr, " %s\n", hop) + } + } + return 1 +} + +func main() { + os.Exit(run(os.Stdin, os.Stdout, os.Stderr)) +} diff --git a/scripts/ci-dumpdep/main_test.go b/scripts/ci-dumpdep/main_test.go new file mode 100644 index 000000000..2214a3da2 --- /dev/null +++ b/scripts/ci-dumpdep/main_test.go @@ -0,0 +1,137 @@ +package main + +import ( + "bytes" + "os" + "path/filepath" + "regexp" + "strings" + "testing" +) + +// prunedLink is an excerpt of a -dumpdep link in which the linker can still +// drop unused methods. MethodByName with a constant name is fine: the compiler +// records the name, so only matching methods are kept. +const prunedLink = `# github.com/zzet/gortex/cmd/gortex +_ -> runtime.main +_ -> main.main +main.main -> main.runDaemonInstallService +main.runDaemonInstallService -> main.renderSystemdUnit +main.renderSystemdUnit -> fmt.Fprintf +main.renderSystemdUnit -> type:main.serviceEnvVar +google.golang.org/protobuf/internal/descfmt.formatListOpt -> reflect.Value.MethodByName +` + +func runDumpdep(t *testing.T, input string) (int, string, string) { + t.Helper() + var stdout, stderr bytes.Buffer + code := run(strings.NewReader(input), &stdout, &stderr) + return code, stdout.String(), stderr.String() +} + +func TestDumpdepAcceptsALinkThatPrunesMethods(t *testing.T) { + code, stdout, stderr := runDumpdep(t, prunedLink) + if code != 0 || stderr != "" || !strings.Contains(stdout, "the linker can drop unused methods") { + t.Fatalf("clean link rejected: exit=%d\nstdout=%s\nstderr=%s", code, stdout, stderr) + } +} + +func TestDumpdepRejectsReachableMethodLookups(t *testing.T) { + input := prunedLink + `main.main -> main.installLaunchd +main.installLaunchd -> main.renderLaunchdPlist +main.renderLaunchdPlist -> text/template.(*Template).execute +text/template.(*Template).execute -> text/template.(*state).walk +text/template.(*state).walk -> text/template.(*state).evalField +text/template.(*state).evalField -> reflect.Value.MethodByName +type:reflect.Value -> reflect.(*Value).Method +` + code, stdout, stderr := runDumpdep(t, input) + if code == 0 || stdout != "" { + t.Fatalf("reachable method lookups accepted: exit=%d\nstdout=%s\nstderr=%s", code, stdout, stderr) + } + for _, want := range []string{ + "ci-dumpdep: 3 reachable symbols", + // Execute is usually inlined, so its caller reaches execute directly + // and the chain shows which first-party code runs a template. + "\ntext/template.(*Template).execute, reached by:\n" + + " main.main\n main.installLaunchd\n main.renderLaunchdPlist\n text/template.(*Template).execute\n", + "\ntext/template.(*state).evalField, reached by:\n" + + " main.main\n main.installLaunchd\n main.renderLaunchdPlist\n text/template.(*Template).execute\n" + + " text/template.(*state).walk\n text/template.(*state).evalField\n", + "\nreflect.(*Value).Method, reached by:\n type:reflect.Value\n reflect.(*Value).Method\n", + } { + if !strings.Contains(stderr, want) { + t.Errorf("report is missing %q:\n%s", want, stderr) + } + } + if strings.Contains(stderr, "MethodByName, reached by") { + t.Errorf("the lookup function itself is not flagged; only its dynamic callers are:\n%s", stderr) + } +} + +func TestDumpdepRejectsInputThatIsNotALinkListing(t *testing.T) { + for _, tc := range []struct{ name, input, echoed string }{ + {"empty", "", ""}, + // A failed build must still show why it failed. + {"build failure", "# github.com/zzet/gortex/cmd/gortex\n./main.go:1:1: expected 'package', found 'EOF'\n", + "The input began:\n# github.com/zzet/gortex/cmd/gortex\n./main.go:1:1: expected 'package', found 'EOF'\n"}, + {"library", "_ -> runtime.main\n", ""}, + } { + t.Run(tc.name, func(t *testing.T) { + code, stdout, stderr := runDumpdep(t, tc.input) + if code == 0 || stdout != "" || !strings.Contains(stderr, "main.main is not in the input") || + !strings.Contains(stderr, tc.echoed) { + t.Fatalf("non-listing accepted: exit=%d\nstdout=%s\nstderr=%s", code, stdout, stderr) + } + }) + } +} + +// TestDumpdepRejectsAListingWithoutFlags guards against a listing format the +// tool cannot read: if the linker stopped appending attribute flags, a +// would go unseen and every link would pass. +func TestDumpdepRejectsAListingWithoutFlags(t *testing.T) { + input := strings.ReplaceAll(prunedLink, " ", "") + code, stdout, stderr := runDumpdep(t, input) + if code == 0 || stdout != "" || !strings.Contains(stderr, "not a -dumpdep listing") { + t.Fatalf("listing without flags accepted: exit=%d\nstdout=%s\nstderr=%s", code, stdout, stderr) + } +} + +// TestCIWorkflowChecksTheReleaseLink keeps the check wired to a -dumpdep link +// with the release build tags and link flags of each platform, with a +// pipeline that fails when the link does. +func TestCIWorkflowChecksTheReleaseLink(t *testing.T) { + data, err := os.ReadFile(filepath.Join("..", "..", ".github", "workflows", "ci.yml")) + if err != nil { + t.Fatal(err) + } + text := strings.ReplaceAll(string(data), "\r\n", "\n") + for name, wants := range map[string][]string{ + "build-linux-static": { + "go build -tags netgo,osusergo,static_link", + "-dumpdep", + "-o /dev/null ./cmd/gortex/ 2>&1 | go run ./scripts/ci-dumpdep", + }, + // darwin and windows: no tags; windows links its runtime statically. + "build-native": { + "shell: bash", + "extld=-extldflags=-static", + `go build -ldflags "-s -w $extld -dumpdep"`, + "./cmd/gortex/ 2>&1 | go run ./scripts/ci-dumpdep", + }, + } { + _, job, found := strings.Cut(text, "\n "+name+":\n") + if !found { + t.Fatalf("ci.yml has no %s job", name) + } + if next := regexp.MustCompile(`\n [a-z][a-z0-9-]*:\n`).FindStringIndex(job); next != nil { + job = job[:next[0]] + } + for _, want := range append(wants, "set -o pipefail") { + if !strings.Contains(job, want) { + t.Errorf("%s is missing %q", name, want) + } + } + } +}