Skip to content

cmd/go2nix: preserve quoted ldflags#42

Closed
ryantm wants to merge 1 commit intonumtide:mainfrom
replit:ryantm/fix-link-binary-ldflags
Closed

cmd/go2nix: preserve quoted ldflags#42
ryantm wants to merge 1 commit intonumtide:mainfrom
replit:ryantm/fix-link-binary-ldflags

Conversation

@ryantm
Copy link
Copy Markdown
Contributor

@ryantm ryantm commented Apr 8, 2026

Parse ldflags with shell-style quoting before invoking the linker so values like -extldflags '-static -L…' stay intact.

Parse ldflags with shell-style quoting before invoking the linker so values like -extldflags '-static -L…' stay intact.
@zimbatm zimbatm requested a review from aldoborrero April 8, 2026 07:30
Copy link
Copy Markdown
Member

@aldoborrero aldoborrero left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for tackling this — quoted -extldflags was a real gap.

One blocking issue: the new splitShellFields treats \ as an escape character (both unquoted and inside "..."), which diverges from go build -ldflags and is a regression vs main:

input main (strings.Fields) / go build this PR
-X main.Path=C:\Users\me main.Path=C:\Users\me main.Path=C:Usersme
-extldflags "-L C:\lib" -L C:\lib -L C:lib

go build -ldflags parses with cmd/internal/quoted.Split, whose doc comment is explicit: "There is no unescaping or other processing within quoted fields." (quoted.go:21-22). It only handles '/" delimiters and \ \t\n\r separators — no backslash escaping at all. Wired to -ldflags at load/flag.go:71.

There's a second, smaller divergence: quoted.Split only treats a quote as special at the start of a field (mid-token quotes are literal), whereas splitShellFields toggles quoting anywhere. So a"b c"d parses differently too.

Suggestion: port cmd/internal/quoted.Split directly (~35 lines, BSD-3; Go itself copies it verbatim in cmd/dist/quoted.go). That gives exact go build parity, fixes the backslash regression, and is simpler than the sh-style parser. The motivating -extldflags '-static -L/foo/lib' case works fine under quoted.Split since the quote is at field start.

Worth adding test cases for: literal backslash (would have caught the above), mid-token quote, adjacent quotes, and the unterminated-escape error path.

case r == '\'' || r == '"':
tokenStarted = true
quote = r
case r == '\\':
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This (and the case '\\': escaped = true at L376 inside "...") is the regression: go build -ldflags uses cmd/internal/quoted.Split, which has no escape handling — \ is always literal. Any ldflags containing a literal backslash (Windows path in -X, etc.) now gets it silently stripped, where both main and go build preserved it.

return out, nil
}

func splitShellFields(s string) ([]string, error) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider replacing this with a port of cmd/internal/quoted.Split (with attribution) so we match go build -ldflags exactly rather than POSIX-sh semantics — that's the contract buildGoModule users are used to. It also avoids the backslash issue and the mid-token-quote divergence.

name: "double-quoted values are preserved",
flags: []string{`-X "main.Message=hello world"`},
want: []string{"-X", "main.Message=hello world"},
},
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be good to add a case with a literal backslash, e.g. {flags: []string{"-X main.Path=C:\\Users"}, want: []string{"-X", "main.Path=C:\\Users"}} — that's the case that regresses with the current escape handling.

@aldoborrero
Copy link
Copy Markdown
Member

Thanks again for this — picked it up in #66 since the branch couldn't be pushed to. Your commit is cherry-picked as-is (authorship preserved); the follow-up commit swaps the splitter for a port of cmd/internal/quoted.Split per the review above. Closing in favour of #66.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants