Pass large ldflags to cgo via response file instead of env variable.#4386
Pass large ldflags to cgo via response file instead of env variable.#4386fmeum merged 9 commits intobazel-contrib:masterfrom
Conversation
mering
left a comment
There was a problem hiding this comment.
Nit: The indentation look off. Note that golang uses tabs for indentation.
Done! Thanks for pointing out. |
|
Also, needs a test in |
I think we can keep supporting them as long as it's convenient. In this case, we can gate the new functionality on |
- Use tempfile for ldflags response file. - Only use response file for go 1.23+. - Add a test to confirm that ldflags are progapated correctly.
Added a new test though some cgo existing tests were also sufficient. |
It fails with "The filename or extension is too long" error, which is due to windows compiler and unrelated to cgo.
|
The change looks good to me. However, CI is failing on Windows due to the command-line length limit. It looks like we're getting the Go version out of |
My hunch is that the new cgo plumbing is working correctly (because cgo execution is not failing). The error is happening in the hand-off from cgo to the windows compiler where arguments are being passed via command line vs params/response file. That seems unrelated to this specific change. In the latest commit, I restricted the test target to macos and linux. We can relax it if we are able to find and fix the underlying issue. |
|
@jayconrod We should be preferring rules_go/go/private/go_mod.bzl Line 61 in 98940c3 |
|
So then @dgoel may be right that there is a problem between cgo and the Windows compiler. I don't like that we're disabling the test on Windows, but if that's true, then it can't work on Windows. Unfortunately I don't have more bandwidth this week to confirm this. Let's merge this now. |
|
@fmeum could you enable squash + automerge for this PR if you are okay with the final set of changes? |
|
Thanks @fmeum! Would it be possible to push a new release to bcr? |
What type of PR is this?
Bug fix
What does this PR do? Why is it needed?
Pass ldflags to cgo tool via a response file instead of
CGO_LDFLAGSenv variable for go 1.23+. This avoids the "argument list too long" error, which can happen ifCGO_LDFLAGSis so large that it exceeds the system limits.Support to accept ldflags via file was added to
gomore than a year ago. References:Which issues(s) does this PR fix?
Fixes #2654