Skip to content

Conversation

@robertbrignull
Copy link
Contributor

@robertbrignull robertbrignull commented Sep 18, 2020

I think this should be all that's necessary to support CODEQL_EXTRACTOR_GO_BUILD_TRACING .

Shouldn't do anything yet. When github/codeql-go#324 is merged and we have a CodeQL bundle containing it then we can test this.

Merge / deployment checklist

  • Confirm this change is backwards compatible with existing workflows.
  • Confirm the readme has been updated if necessary.

@smowton
Copy link
Contributor

smowton commented Oct 21, 2020

The Go PR linked above has been merged

@robertbrignull
Copy link
Contributor Author

@smowton, @max-schaefer, it'll have to wait until we have a dist to test with, but how would I test that this is working? Do I just enable the env var and check that things still work as before? Do you have a demo project that could be built?

@max-schaefer
Copy link
Contributor

@sauyon is probably best placed to answer that question. Manual testing should be fairly straightforward: you can tell from the log whether it did build tracing or not. Automated testing could be a little trickier.

@robertbrignull robertbrignull force-pushed the robertbrignull/go_build_trace branch from 37788dd to aada19f Compare October 22, 2020 10:03
Co-authored-by: Max Schaefer <54907921+max-schaefer@users.noreply.github.com>
@robertbrignull robertbrignull force-pushed the robertbrignull/go_build_trace branch from aada19f to 2e550bb Compare October 30, 2020 16:44
@robertbrignull
Copy link
Contributor Author

This isn't working unfortunately. Were the Go tracing changes in CodeQL CLI v2.3.2?

@smowton
Copy link
Contributor

smowton commented Oct 30, 2020

Yes. Could you give an example of how you're trying to run it?

@robertbrignull
Copy link
Contributor Author

Yes. Could you give an example of how you're trying to run it?

There's an exmaple in the intergration test I add in this PR: https://github.com/github/codeql-action/pull/222/files#diff-3a1fea4e5bb8f47ffb16d851debf4b504b20e782df9dc03336b120d14126147d

@sauyon
Copy link

sauyon commented Nov 2, 2020

This seems to be an error originating in Javascript (init-action.js?). Is there something that we're meant to be doing that we're not?

@robertbrignull
Copy link
Contributor Author

The (slightly hacky) way that the codeql-action deals with tracing builds is it runs codeql database trace-command with a command that outputs the environment. We then set that same environment in the actions environment for when the actual build command happens later (that isn't controlled by us).

I think what's happening here that's causing the failure is that the ODASA_TRACER_CONFIGURATION env var is not being set on the traced command and we rely on it being there. Is that something that could be related to Go build tracing?

@sauyon
Copy link

sauyon commented Nov 2, 2020

We definitely don't unset any variables intentionally. If a traced command is passed the autobuilder is never called anyway, right?

Is there something we need to add to the tracer configuration?

@smowton
Copy link
Contributor

smowton commented Nov 3, 2020

I got to the bottom of this -- the problem is your action is still using CodeQL 2.3.0:

  /opt/hostedtoolcache/CodeQL/0.0.0-20201008/x64/codeql/codeql version --format=json
  {
    "productName" : "CodeQL",
    "vendor" : "GitHub",
    "version" : "2.3.0",
    "sha" : "429542918fde5ffd1caa14571bcdf6498db96ee4",
    "branches" : [
      "codeql-cli-2.3.0"
    ],
    "copyright" : "Copyright (C) 2019-2020 GitHub, Inc.",
    "unpackedLocation" : "/opt/hostedtoolcache/CodeQL/0.0.0-20201008/x64/codeql"
  }

I confirmed that the needed ODASA env variable is set by CodeQL 2.3.2 but not 2.3.0.

@robertbrignull
Copy link
Contributor Author

Ah right, I know why this isn't working even though the latest released bundle should be for 2.3.2. It's because the bundle is also included in the actions environment as a performance improvement, and that one is still at 2.3.0 so it's using that in preference instead of downloading a new one. I'll look at working around this.

@robertbrignull
Copy link
Contributor Author

On second thought, even if I do work around it here, we still can't release this change until 2.3.2 has reached https://github.com/actions/virtual-environments/ as otherwise it'll break for other users. I think this PR has to wait until the next actions environment update happens.

@robertbrignull robertbrignull mentioned this pull request Nov 4, 2020
2 tasks
@robertbrignull
Copy link
Contributor Author

So CodeQL 2.3.2 is now live, so I've updated, but unfortunately it still fails. It gets further this time and now fails because it says "no code was seen during the build". I did a test at https://github.com/github/codeql-action/runs/1395477783?check_suite_focus=true where I insert a call to env and you can see that the environment looks correct.

Is my call to go tool compile main.go incorrect, or anything else you can see? @smowton, @sauyon

@smowton
Copy link
Contributor

smowton commented Nov 13, 2020

I note during the compile command the printed environment has an unresolved variable in the LD_PRELOAD variable (/opt/hostedtoolcache/CodeQL/0.0.0-20201028/x64/codeql/tools/linux64/${LIB}trace.so), which disappears in the following step?

@smowton
Copy link
Contributor

smowton commented Nov 13, 2020

Ah also, even if that is ok, looks like when our compiler wrapper is called it only handles go build, go install and go run, not go tool compile, perhaps try using one of those?

Do you know where the extractor's logs are sent to when it's run as a compiler wrapper in this way? If this is happening you should see log entries like "Non-build command '...'; skipping"

@robertbrignull
Copy link
Contributor Author

I note during the compile command the printed environment has an unresolved variable in the LD_PRELOAD variable

It's a bit mad but I think that's how it's meant to work, with the explicit unesolved ${LIB} in there. Also, I only just now realised it prints the env for every step anyway, so I maybe didn't need to add an explicit printing, but it does show there's a difference in the LD_PRELOAD variable. In the env printed by actions on each step the ${LIB} is not resolved, but in the output from env it is resolved to LD_PRELOAD=/opt/hostedtoolcache/CodeQL/0.0.0-20201028/x64/codeql/tools/linux64/lib64trace.so. Is that what you mean by "it disappears"?

Ah also, even if that is ok, looks like when our compiler wrapper is called it only handles go build, go install and go run, not go tool compile, perhaps try using one of those?

I think this is the real issue. I'll try to update it. I wasn't sure the best way to do a minimal Go program.

Something else, I note in the CodeQL output from database finalize it says

For codebases written in Go, JavaScript, TypeScript, and Python, do not specify 
an explicit --command.

Should this be changed when Go build tracing is enabled?

@smowton
Copy link
Contributor

smowton commented Nov 13, 2020

I thought it might be that, just checking on the off chance a " ought to be a ` somewhere :)

On the last question I'll defer to @sauyon

@robertbrignull
Copy link
Contributor Author

Changing to go build main.go got everything working in my test. If that's working then I think that means this is good to go at last.

Copy link
Contributor

@chrisgavin chrisgavin left a comment

Choose a reason for hiding this comment

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

😆 I think I spent more time reading the discussion than reading the code.

Looks pretty simple. Thanks for the test!

@robertbrignull robertbrignull merged commit aafb457 into main Nov 19, 2020
@robertbrignull robertbrignull deleted the robertbrignull/go_build_trace branch November 19, 2020 18:01
@github-actions github-actions bot mentioned this pull request Nov 23, 2020
@robertbrignull robertbrignull changed the title Check CODEQL_EXTRACTOR_GO_TRACE and treat Go as a traced language Check CODEQL_EXTRACTOR_GO_BUILD_TRACING and treat Go as a traced language Nov 27, 2020
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.

6 participants