Merge pull request 'The linker is told once, not twice' (#159) from fix/161-one-ldflags-not-two into main

internal/builder/builder.go

@@ -877,13 +877,20 @@ func compile(ctx context.Context, run Runner, tree string, chain Toolchain,
 		base,
 	}
 	invocation = append(invocation, chain.Compile...)
-	// What it was built for, linked in. The compile line carries `-ldflags` already; this appends a
-	// second one, which the Go linker accepts and merges. A host with no system refuses every
-	// declaration before it applies anything (novox/hq 04-ISSUES/161), and it is the artifact that
-	// knows — the target is a property of the artifact rather than of the recipe (ADR 0142).
+	// **One `-ldflags`, composed here.** A repeated flag is not a merged one: the Go command takes
+	// the last and drops the first, so passing the toolchain's flags and then the system stamp as a
+	// second `-ldflags` produced a binary that knew its system and had lost `-s -w` — half again the
+	// size, with its debug info (novox/hq 04-ISSUES/161).
+	//
+	// What it was built for is the one thing taken from the artifact, and ADR 0142 says why: the
+	// target is a property of the artifact rather than of the recipe. A host with no system refuses
+	// every declaration before it applies anything.
+	linker := append([]string(nil), chain.LinkerFlags...)
 	if chain.SystemStamp != "" && strings.TrimSpace(a.System) != "" {
-		invocation = append(invocation, "-ldflags",
-			"-X "+chain.SystemStamp+"="+strings.TrimSpace(a.System))
+		linker = append(linker, "-X", chain.SystemStamp+"="+strings.TrimSpace(a.System))
+	}
+	if len(linker) > 0 {
+		invocation = append(invocation, "-ldflags", strings.Join(linker, " "))
 	}
 	if chain.OutputFlag != "" {
 		// A compiler pointed at a package is told the file to write, not the directory: the name a

internal/builder/system_stamp_test.go

@@ -47,3 +47,30 @@ func TestTheStampIsTheOneThingTakenFromTheArtifact(t *testing.T) {
 		t.Fatalf("the compile line takes something from the module: %q", joined)
 	}
 }
+
+func TestTheLinkerIsToldOnceNotTwice(t *testing.T) {
+	// A repeated flag is not a merged one: the Go command takes the last -ldflags and drops the
+	// first. Passing the toolchain's flags and then the stamp separately produced a binary that knew
+	// its system and had lost -s -w — 12.2MB against 8.5MB, with its debug info (04-ISSUES/161).
+	chain, err := ToolchainFor("go")
+	if err != nil {
+		t.Fatal(err)
+	}
+	for _, arg := range chain.Compile {
+		if arg == "-ldflags" {
+			t.Fatal("the compile line carries -ldflags, so composing one here makes two")
+		}
+	}
+	if len(chain.LinkerFlags) == 0 {
+		t.Fatal("the go toolchain passes no linker flags, so the binary keeps its debug info")
+	}
+	var stripped bool
+	for _, f := range chain.LinkerFlags {
+		if f == "-s" {
+			stripped = true
+		}
+	}
+	if !stripped {
+		t.Fatalf("the go toolchain does not strip: %v", chain.LinkerFlags)
+	}
+}

internal/builder/toolchain.go

@@ -49,6 +49,14 @@ type Toolchain struct {
 	// is named as it will be FOUND, inside the unpacked bundle, so the source is the same path with
 	// the output directory taken off the front and this on the end.
 	SourceExt string
+	// LinkerFlags are passed to the linker as one flag, together with the system stamp below.
+	//
+	// **Separate from Compile because a repeated flag is not a merged one.** They were in the compile
+	// line, and appending the stamp as a second `-ldflags` meant the Go command took the last and
+	// dropped the first — so the binary gained its system and lost `-s -w`, growing by half and
+	// carrying its debug info. The mistake was believing a comment rather than reading the file it
+	// produced (novox/hq 04-ISSUES/161).
+	LinkerFlags []string
 	// SystemStamp is the variable this language's linker fills with the artifact's declared system,
 	// for a language whose binaries are pinned to one at link time (novox/hq ADR 0005).
 	//
@@ -126,9 +134,12 @@ var toolchains = []Toolchain{
 		// rather than from the linker: two builds of one commit produce the same bytes.
 		Compile: []string{
 			"env", "CGO_ENABLED=0", "GOFLAGS=-trimpath",
-			"go", "build", "-ldflags", "-s -w",
+			"go", "build",
 		},
-		OutputFlag: "-o",
+		// Stripped of symbols and debug info: what a machine holds is a file it runs, not one it
+		// debugs, and the difference measured 12.2MB against 8.5MB.
+		LinkerFlags: []string{"-s", "-w"},
+		OutputFlag:  "-o",
 		// Pointed at the package the artifact is built `from`, compiled whole. Go writes the binary
 		// into the output directory, named after the package — so the bundle a machine unpacks is a
 		// directory holding one executable, which is what the delivery mechanism expects