-
Notifications
You must be signed in to change notification settings - Fork 333
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
Install wrapper script for Go on Linux to support tracing for Go 1.21 and above #1909
Conversation
b5e2270
to
d4722a8
Compare
Can we already add a feature gate/version flag to that, so that we automatically stop this wrapper script installation at some point in the future? Or a turn-off switch from the CLI? Edit: Maybe we can use the new version feature already? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This broadly looks good but still needs some sort of version gating mechanism so that we don't start double-tracing if and when the CLI starts handling this situation for itself.
71ef220
to
5933ae0
Compare
5933ae0
to
0696c1b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As a general comment, are we happy with the test coverage here? Specifically, do we want to add some unit tests or some specific assertions to the PR checks? Or do the existing checks passing on Go 1.21.1 give us enough confidence?
- Change parameter name - Add more documentation
0696c1b
to
68d0b65
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I'd like to hear what Chris thinks about how we should behave in the absence of file
, but otherwise this looks good to me.
31af127
to
faf7528
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice!
Co-authored-by: Henry Mercer <[email protected]>
cb79d4d
to
235bdca
Compare
This PR modifies the
init
Action to add ago
wrapper script to thePATH
if we are running on Linux in order to better cope with tracing builds for Go 1.21 and above. This works as follows:go
binary.file
command."statically linked"
, we install the wrapper script to abin
directory in the CodeQL temporary directory.bin
directory to the systemPATH
CODEQL_ACTION_GO_BINARY
(we could probably omit this since the path can be reconstructed in theanalyze
step)Additionally, we verify that the result of
which go
points to our wrapper script in theanalyze
step by checking it against the value ofCODEQL_ACTION_GO_BINARY
to make sure that users did not e.g. use asetup-go
step after theinit
action.Merge / deployment checklist