Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cmd/go: support passing @args-file to -toolexec commands #70046

Open
RomainMuller opened this issue Oct 25, 2024 · 10 comments
Open

cmd/go: support passing @args-file to -toolexec commands #70046

RomainMuller opened this issue Oct 25, 2024 · 10 comments
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@RomainMuller
Copy link

RomainMuller commented Oct 25, 2024

Go version

go version go1.23.2 linux/arm64

Output of go env in your module/workspace:

GO111MODULE=''
GOARCH='arm64'
GOBIN=''
GOCACHE='/root/.cache/go-build'
GOENV='/root/.config/go/env'
GOEXE=''
GOEXPERIMENT=''
GOFLAGS=''
GOHOSTARCH='arm64'
GOHOSTOS='linux'
GOINSECURE=''
GOMODCACHE='/go/pkg/mod'
GONOPROXY=''
GONOSUMDB=''
GOOS='linux'
GOPATH='/go'
GOPRIVATE=''
GOPROXY='https://proxy.golang.org,direct'
GOROOT='/usr/local/go'
GOSUMDB='sum.golang.org'
GOTMPDIR=''
GOTOOLCHAIN='local'
GOTOOLDIR='/usr/local/go/pkg/tool/linux_arm64'
GOVCS=''
GOVERSION='go1.23.2'
GODEBUG=''
GOTELEMETRY='local'
GOTELEMETRYDIR='/root/.config/go/telemetry'
GCCGO='gccgo'
GOARM64='v8.0'
AR='ar'
CC='gcc'
CXX='g++'
CGO_ENABLED='1'
GOMOD='/Users/romain.marcadier/Development/Datadog/orchestrion/go.mod'
GOWORK=''
CGO_CFLAGS='-O2 -g'
CGO_CPPFLAGS=''
CGO_CXXFLAGS='-O2 -g'
CGO_FFLAGS='-O2 -g'
CGO_LDFLAGS='-O2 -g'
PKG_CONFIG='pkg-config'
GOGCCFLAGS='-fPIC -pthread -Wl,--no-gc-sections -fmessage-length=0 -ffile-prefix-map=/tmp/go-build89119916=/tmp/go-build -gno-record-gcc-switches'

What did you do?

The following command works as intended:

$ go build -a github.com/elastic/go-elasticsearch/v8/typedapi/types

However if you use any -toolexec value, it suddenly fails on Windows:

$ go build -toolexec="D:\\a\\orchestrion\\orchestrion\\bin\\orchestrion.exe" -a github.com/elastic/go-elasticsearch/v8/typedapi/types
fork/exec D:\\a\\orchestrion\\orchestrion\\bin\\orchestrion.exe: The filename or extension is too long.

This is because the github.com/elastic/go-elasticsearch/v8/typedapi/types package contains a metric ton of files, and results in an excessively long command line for Windows.

What did you see happen?

What we observe is that when there is no -toolexec the Windows arguments size limit (of 32KiB) is "worked around" by passing an @args-file value that contains all the arguments instead of passing the arguments list itself directly...

Unfortunately, the use of -toolexec appears to disable this mechanism, making it impossible to build packages with very large amounts of files in them on Windows.

I have verified this behavior using strace on a Linux machine... I expect the same behavior happens on Windows as well and explains our issue here...

What did you expect to see?

I expect a package that successfully builds without -toolexec to also be build-able with -toolexec, and for the -toolexec tool to actually also receive the arguments file stand-in when appropriate.

@RomainMuller
Copy link
Author

@RomainMuller
Copy link
Author

Also, the culprit is likely this code path:

switch prog {
case "compile", "link", "cgo", "asm", "cover":
default:
return false
}

Since in our case, the prog ends up being the -toolexec executable...

@seankhliao seankhliao changed the title cmd/go: impossible to build some packages with -toolexec on Windows cmd/go: support passing @args-file to -toolexec commands Oct 25, 2024
@prattmic prattmic added this to the Backlog milestone Oct 25, 2024
@prattmic
Copy link
Member

cc @matloob @samthanawalla

@prattmic prattmic added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Oct 25, 2024
@ianlancetaylor
Copy link
Member

It's fair to say that most Unix programs do not understand the @ syntax for response files. So it wouldn't be appropriate to use it by default.

Do most Windows programs understand that syntax?

@RomainMuller
Copy link
Author

RomainMuller commented Oct 28, 2024

I agree - this could be surprising to a bunch of tools; although... On Windows specifically, if the CLI arguments exceed 32KiB, then the command will basically just not run... It'll just fail differently if you pass a response file instead of the arguments list in that case...

But regardless; I think it would be a-okay (to me at least) to either have:

  • Another CLI flag to signal the -toolexec command understands response files
  • A "special" syntax in -toolexec that signifies it understands response files

It might also be relevant to document the syntax of these files publicly so that it feels less like copying CLI implementation details (apparently it supports line continuations, the response file can contain further response files, etc...)


An alternative to the "new flag" or "special syntax" solutions mentioned above would be to enact feature negociation between the go toolchain & the downstream tools... The toolchain already invokes all tools with -V=full to compute the build IDs. One could imagine using the output of that also to determine whether tools support response files or not... e.g:

  • Plain tool supports response-file, mentions it with a feature line saying it supports direct (in opposition to indirectly via a -toolexec) invocations with a response file:
    $ go tool compile -V=full
    compile version go1.23.2
    response-file=direct
  • Legacy -toolexec tool does not support it; just forwards output of compile -V=full:
    $ legacy.sh /opt/homebrew/Cellar/go/1.23.2/libexec/pkg/tool/darwin_arm64/compile -V=full
    compile version go1.23.2
    response-file=direct
  • Modern -toolexec tool forwards output of compile -V=full and replaces the value of the response-file feature with always, signifying that the -toolexec tool also understands response files
    $ legacy.sh /opt/homebrew/Cellar/go/1.23.2/libexec/pkg/tool/darwin_arm64/compile -V=full
    compile version go1.23.2
    response-file=always

One advantage to this approach is it removes the need for the command allow-list, and more commands cand eclare their support for this feature by just implementing it... It also allows -toolexec to support response files even if the underlying tool does not...

Obviously this is not exempt from issues either (is adding more content to the output of -V=full invocations risking to break existing tools? How much future proofing does this need? Should this influence build-id/caching (I guess not)? Etc...)


The -toolexec tool could also be "tested" at the start of the build:

  • Does not support response file
    $ tool.sh $(go tool -n echo) @response-file.txt
    @response-file.txt
  • Does support response file
    $ tool.sh $(go tool -n echo) @response-file.txt
    contents-of
    the
    file.txt

The idea here being that tools that were created before introduction of this mechanism would simply forward arguments to the echo tool; but tools opting into support for response file would instead parse out the @response-file.txt and pass that content to echo, resulting in different output...

@RomainMuller
Copy link
Author

I will add that today, if the -toolexec tool is named compile, link, cgo, asm, or cover, it'll receive the response file (and possibly crash if it does not support this). I have been able to verify this with orchestrion today.


In addition to this, I would say that today, any change to any tool's arguments could be seen as a breaking change for -toolexec tools that process said arguments... I think the contract of -toolexec is that it either must not care about the arguments being passed (just forwarding them as-is), or it must be ready to deal with anything the tool may receive; including a response file.

From this perspective, whether sending the @response-file arguments via -toolexec tools is a breaking change or not is somewhat debatable.

@mvdan
Copy link
Member

mvdan commented Oct 28, 2024

I think the contract of -toolexec is that it either must not care about the arguments being passed (just forwarding them as-is), or it must be ready to deal with anything the tool may receive; including a response file.

That is what I have been assuming with my own development of toolexec tools.

FWIW I already wrote generic response file support in Go at https://pkg.go.dev/mvdan.cc/responsefile, and you're welcome to reuse it as well.

@matloob
Copy link
Contributor

matloob commented Nov 12, 2024

I think the contract of -toolexec is that it either must not care about the arguments being passed (just forwarding them as-is), or it must be ready to deal with anything the tool may receive; including a response file.

That is what I have been assuming with my own development of toolexec tools.

I would have made the same assumption.

I'm wondering if there are any tools that already exist that examine the arguments passed in used in workflows that would break if we decided to start passing in response file arguments?

@mvdan
Copy link
Member

mvdan commented Nov 12, 2024

I don't think any toolexec tools written today would handle @-arg response files, because they simply have never been supplied before. But those same tools are already pretty broken today when very many files are being built at once, e.g. burrowers/garble#572, so I think we need to cross this bridge sooner or later.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

No branches or pull requests

6 participants