227 lines
13 KiB
Markdown
227 lines
13 KiB
Markdown
# Host-native launcher review findings
|
||
|
||
Reviewed the implementation from `bf07bed` through `0269648` against the
|
||
[feature roadmap](host-native-launchers.md) and
|
||
[implementation plan](implementation.md).
|
||
|
||
The maintained launchers select the Go host before preparing and running the
|
||
tool. API-only recovery prepares its private module on the host; printed Go
|
||
handoffs discover the host again when run. The CLI retains the native fallback
|
||
and saved-record selection. Application build targets still come from the
|
||
profile, and the public Go facades and dependency boundaries are unchanged.
|
||
|
||
## Make the adoption smoke command runnable
|
||
|
||
**Completed.** The smoke command runs the saved launcher through `sh`; verified with a non-executable file and a foreign target.
|
||
|
||
**Evidence:** The [adoption guide](../consumers/adoption.md#cli-tool-dependency)
|
||
tells readers to save the launcher as `releasekit` and run
|
||
`./releasekit version`. A newly saved shell file ordinarily lacks its execute
|
||
bit. In a disposable directory, the documented command failed with status 126
|
||
and “Permission denied.” Invoking the same snippet through `sh` ran a
|
||
module-registered fixture tool on Linux/amd64 with inherited
|
||
`GOOS=windows GOARCH=arm64`.
|
||
|
||
**Fix:** Use `sh ./releasekit version` in the smoke instructions, or give an
|
||
explicit `chmod +x ./releasekit` step before direct execution. Verify the
|
||
published instructions from a newly created file under a foreign target.
|
||
|
||
## Limit the recovery guarantee to GOOS and GOARCH
|
||
|
||
**Completed.** The CLI reference names the two target variables and identifies the shell syntax.
|
||
|
||
**Evidence:** The [CLI contract](../cli.md) says Go handoffs select the host
|
||
“independently of inherited cross-compilation settings.” The
|
||
[command formatter](../../cmd/releasekit/main.go) overrides `GOOS` and
|
||
`GOARCH` and retains other build settings. For example, building the actual CLI
|
||
with the native target, `CGO_ENABLED=1`, and an unavailable C compiler named
|
||
by `CC` still fails in `runtime/cgo`. This is consistent with the roadmap's
|
||
scope, but the CLI wording implies broader independence.
|
||
|
||
**Fix:** Say that generated Go handoffs set `GOOS` and `GOARCH` from the
|
||
selected Go toolchain at invocation time. Preserve the other inherited settings
|
||
and the existing workspace and toolchain overrides. The documented handoff
|
||
uses POSIX shell syntax; identify that syntax when telling operators where to
|
||
paste the command.
|
||
|
||
## Exercise a profile target that differs from the launcher host
|
||
|
||
**Completed.** The binary delivery fixture selects a non-host Linux target and checks the uploaded ELF machine as well as the filename.
|
||
|
||
**Evidence:** The binary-delivery case in
|
||
[`TestMultiModuleWoodpeckerDelivery`](../../cmd/releasekit/multi_module_woodpecker_test.go)
|
||
supplies a foreign target to the workflow and checks the uploaded
|
||
`core-v2.1.0-linux-amd64` asset. The profile itself selects `linux/amd64`.
|
||
On the Linux/amd64 review host, the test cannot distinguish using the
|
||
profile target from accidentally using the normalized launcher target. The
|
||
current build code still applies the selected profile target explicitly.
|
||
|
||
**Fix:** In that fixture, select a supported profile target distinct from the
|
||
host and assert the uploaded asset matches it. Where practical, inspect the
|
||
uploaded executable header as well as the name, so the test catches a
|
||
host-built payload with a profile-derived filename. Keep the existing local
|
||
forge fixture and native version check; the cross-built asset need not run on
|
||
the host.
|
||
|
||
## Reconcile the feature roadmap with the completed implementation
|
||
|
||
**Completed.** The feature roadmap records implementation and labels the pre-change baseline. The release-note entry remains in the implementation plan.
|
||
|
||
**Evidence:** The [feature roadmap](host-native-launchers.md) still declares
|
||
“implementation pending” and describes the pre-change launchers as the current
|
||
baseline. All six stages in the [implementation plan](implementation.md) are
|
||
marked complete, and the maintained launchers and recovery code implement the
|
||
host selection described there.
|
||
|
||
**Fix:** Mark the roadmap implemented and label the old baseline as historical,
|
||
or retire the completed planning documents after review. Preserve the planned
|
||
release-note entry for the next selected release version.
|
||
|
||
## Verification
|
||
|
||
`go test ./...`, `go run ./internal/apicheck`, and syntax checks for the
|
||
maintained shell scripts passed. The Go suite exercised real tool preparation,
|
||
Woodpecker command blocks with local fixtures, and later execution of printed
|
||
recovery commands. Disposable smoke probes also verified the maintainer
|
||
launcher forwards the child status and the manual retry preserves the calling
|
||
shell's target variables. The review made no changes to production code or
|
||
tests.
|
||
|
||
## Integration audit
|
||
|
||
The audit below examines how the new bootstrap paths interact with existing
|
||
invocation recipes, product checks, and publication recovery.
|
||
|
||
### Use the external Go executable in printed recovery commands
|
||
|
||
**Completed.** All three Go invocations use external lookup through `PATH`. The handoff regression reproduces the shell-function failure before the fix and verifies reconciliation afterward.
|
||
|
||
**Evidence:** Before host normalization, the
|
||
[command formatter](../../cmd/releasekit/main.go) emitted
|
||
`env GOWORK=off GOTOOLCHAIN=local go ...`, which resolves Go through `PATH`.
|
||
[Recovery preparation](../../internal/gotool/recovery.go) still executes an
|
||
external Go command. The new printed command uses bare `go` for both host
|
||
queries and the later tool invocation, so a function named `go` in the
|
||
operator's shell can intercept it. In a local rejected-push fixture, the
|
||
actual printed command reconciled the saved record in an ordinary shell.
|
||
With `go() { GOOS=windows GOARCH=arm64 command go "$@"; }` defined, the same
|
||
command failed with an exec format error. The function overrode the final
|
||
assignments even though the host queries reported the correct host. In this
|
||
case both paths exited with status 1, but only one produced a reconciliation
|
||
result.
|
||
|
||
**Fix:** Restore external command lookup for the host queries and tool
|
||
invocation, for example by using `env go -C ...` throughout the subshell.
|
||
Keep selection through the operator's current `PATH`, including executable
|
||
shims, and keep the exact record and exit status. Exercise a generated
|
||
handoff from a POSIX shell that defines a cross-build Go function; assert
|
||
the reconciliation result as well as the status.
|
||
|
||
### Explain how host normalization affects configured product checks
|
||
|
||
**Completed.** The adoption guide explains inherited host variables and points checks that need an application target to the existing environment overlay.
|
||
|
||
**Evidence:** The updated launchers export `GOOS` and `GOARCH` to the
|
||
ReleaseKit process. Existing [configured checks](../config.md) inherit that
|
||
process environment unless their `env` maps override it;
|
||
[`RunChecks`](../../cmd/releasekit/internal/cli/config.go) adds the configured
|
||
and ReleaseKit-owned values to the child. This differs from built-in
|
||
application compilation, which chooses its target from the profile. A
|
||
disposable consumer's check using shell defaults for an application target
|
||
passed with the previous direct `go tool` invocation and no target
|
||
variables, then failed through the updated launcher because it observed
|
||
`linux/amd64`. Adding explicit `GOOS=windows` and `GOARCH=arm64` to that
|
||
check's `env` restored its behavior.
|
||
|
||
**Fix:** In the launcher adoption guidance, explain that host variables
|
||
reach configured child checks and that checks which need an application
|
||
target should set it explicitly in their existing `env` map or command.
|
||
Link to the configuration contract. Keep the current environment overlay
|
||
semantics and the separate profile selection for built-in builds.
|
||
|
||
### Align direct source and standalone CI recipes with the launcher guidance
|
||
|
||
**Completed.** The source examples link to host selection, and the standalone install recipe scopes the same settings to `go install`. Both paths were verified under a foreign target.
|
||
|
||
**Evidence:** The [README quick start](../../README.md) and
|
||
[CLI reference](../cli.md) still recommend bare `go run ./cmd/releasekit`
|
||
for source invocation. The [consumer delivery guide](../consumers/delivery.md#woodpecker-tag-workflows)
|
||
also recommends bare `go install ...@PIN` for consumers who install the
|
||
delivery executable separately. These older recipes remain target-sensitive
|
||
alongside the new host-native launchers. On Linux/amd64 with inherited
|
||
`GOOS=windows GOARCH=arm64`, the README's actual `version` command failed
|
||
with an exec format error. A versioned install from a disposable local
|
||
module proxy failed under the same variables with `GOBIN` set; the native
|
||
install succeeded and its binary ran.
|
||
|
||
**Fix:** Link these recipes to the host-selection instructions and make the
|
||
native target requirement explicit at the source invocation and at the
|
||
standalone installation step. Use the existing recipe where a copyable
|
||
command is useful. Keep the selected version, install location, and
|
||
`go run` exit-status caveat. An already installed native binary does not
|
||
need a new bootstrap step.
|
||
|
||
## Algorithmic audit
|
||
|
||
The host normalization itself adds a fixed number of Go invocations per
|
||
launch. The CLI renders a fixed argument list, so quoting and formatting take
|
||
time and space linear in the lengths of the directory and record paths. Go
|
||
dependency inspection and compilation retain their existing roles. The
|
||
findings below concern a pre-existing algorithm on the audited recovery path
|
||
and a new module-selection defect in the smoke recipe.
|
||
|
||
### Relocate recovery replacements in one pass without merging selectors
|
||
|
||
**Completed.** Recovery rewrites relative destinations in one pass. The CLI regression verifies both selectors, the selected tool, the rejected-push status, and later reconciliation.
|
||
|
||
**Evidence:** The API-only path in
|
||
[`RecoveryTool`](../../internal/gotool/recovery.go) walks the parsed
|
||
`go.mod` replacements. It calls `modfile.AddReplace` for each relative
|
||
local replacement. In the pinned `golang.org/x/mod` implementation,
|
||
each call scans the entire replacement slice. With `R` total directives
|
||
and `L` relative local directives, relocation visits `O(R × L)` entries,
|
||
and takes quadratic time when all or most entries require relocation.
|
||
This loop predates host normalization and still runs before the new host
|
||
query.
|
||
|
||
The same loop can panic on a valid module that declares both an unversioned
|
||
and a version-specific replacement for the same dependency. When the
|
||
unversioned directive appears first and uses a relative path, calling
|
||
`AddReplace` with an empty old version treats both directives as matches
|
||
and clears the version-specific entry. The outer loop then visits that
|
||
cleared entry as if it were another relative replacement. In a disposable
|
||
API-only consumer with both forms for ReleaseKit, Go accepted the module
|
||
and validation passed. After a local hook rejected the tag push, ReleaseKit
|
||
saved the publication record, then panicked in `modfile.addReplace`. It
|
||
exited with status 2 instead of reporting the rejected push and a recovery
|
||
handoff.
|
||
|
||
**Fix:** Rewrite each relative right-hand path on its existing parsed
|
||
directive in one pass, preserving the exact left-hand path and version
|
||
selector. For example, update the typed replacement and its corresponding
|
||
syntax token before formatting. This takes `O(R)` directive visits plus
|
||
the bytes processed for the paths and avoids selector merging. Keep the
|
||
existing relocation policy, the separate synthetic replacement for the
|
||
original module, and the private module lifetime. Add a recovery fixture
|
||
with distinct local destinations for a wildcard and a version-specific
|
||
replacement. Check that the handoff preserves both directives, selects the
|
||
version-specific destination, and reports the original publication status.
|
||
|
||
### Anchor smoke-check module paths to the intended checkout
|
||
|
||
**Completed.** Both nested paths use `./`; verified the full smoke recipe with a conflicting `CDPATH`.
|
||
|
||
**Evidence:** The [multi-module smoke recipe](../../examples/multi-module/README.md)
|
||
replaced `go -C integrations/...` with subshells that run
|
||
`cd integrations/...` before host discovery. A shell with `CDPATH` set can
|
||
resolve these relative names in another checkout. In a disposable probe
|
||
with matching module paths under two directories, the published snippet
|
||
sent both nested checks to the checkout named by `CDPATH` and returned
|
||
success. If the other checkout has valid profiles and tools, the smoke
|
||
result can appear valid for the wrong candidate.
|
||
|
||
**Fix:** Use `cd ./integrations/ckconfig` and `cd ./integrations/core/v2`,
|
||
or clear `CDPATH` for those two directory changes. The number of lookups is
|
||
already fixed; the change corrects which directory is selected. Verify the
|
||
copyable recipe with a conflicting `CDPATH` entry.
|