Build tracing #324
Build tracing #324
Conversation
max-schaefer
left a comment
max-schaefer
left a comment
There was a problem hiding this comment.
Thank you very much for persisting with this! It's pleasantly simple, I only have a couple of fairly minor questions and suggestions.
| @@ -0,0 +1,2 @@ | |||
| lgtm,codescanning | |||
| * The extractor can now be used for build tracing, allowing users to supply a build command when creating databases with the CodeQL CLI or via configuration. It currently only supports projects that use Go modules. | |||
There was a problem hiding this comment.
This should mention that build tracing is opt-in, but we can add that once we have decided the exact details of how the opting-in works.
There was a problem hiding this comment.
@sauyon noting this is still to do. Do we have a firm answer on how a user would opt in?
There was a problem hiding this comment.
I believe that that was likely just the discussion of variable naming.
There was a problem hiding this comment.
Cool, in that case mention how it's done in the change-note
smowton
left a comment
smowton
left a comment
There was a problem hiding this comment.
Looks like all my comments apply to commits that are later improved (consider squashing)
| } else if !mimic && args[i] == "--help" { | ||
| usage() | ||
| os.Exit(0) | ||
| } else if args[i] == "--mimic" { |
There was a problem hiding this comment.
For regularity we should accept --mimic=
| } | ||
| } | ||
| } else { | ||
| log.Fatalf("Invalid --mimic: no compiler specified") |
There was a problem hiding this comment.
Give an example in the error message, like --mimic must take an argument, e.g. --mimic go
| i++ | ||
| compiler := args[i] | ||
| log.Printf("Compiler: %s", compiler) | ||
| if i+1 < len(args) { |
There was a problem hiding this comment.
Worth printing a warning if this isn't true, since the command argument is required
| log.Printf("Intercepting build") | ||
| // skip `-o output` and `i`, if applicable | ||
| for i+1 < len(args) { | ||
| if args[i+1] == "-o" { |
There was a problem hiding this comment.
If we're skipping these we should skip them wherever they occur in the arg list, not break after the first good argument.
sauyon
force-pushed
the
tracing
branch
2 times, most recently
from
b3744ec to
9aab773
Compare
|
I've applied all the relevant changes and tried to squash to a sensible history. Performance evaluation was very wobbly. I'm trying again, but may have to throw out the idea of performance testing. I should probably test on Windows again, all things considered... |
|
As discussed elsewhere, could we rename the environment variable for turning on build tracing from |
|
The experiments I ran did nothing to improve the Kubernetes situation, so I think merging this now is a sensible thing to do. |
smowton
left a comment
smowton
left a comment
There was a problem hiding this comment.
Highlighted some of Max's outstanding comments, plus one of mine noted below, otherwise as you say let's get this merged. Could you write an issue brain-dumping what you know of the Kubernetes situation?
Co-authored-by: Chris Smowton <smowton@github.com>
|
I believe I have now actually addressed all comments. |
smowton
left a comment
smowton
left a comment
There was a problem hiding this comment.
Nice, looks good! @max-schaefer are you happy to drop your block on this?
|
Yep, let's give this a try! |
|
@sauyon please link the issue here when you've got a writeup |
3 participants
I'd forgotten quite how long dist-compares take, especially since the kubernetes dependencies aren't being dropped (because codeql doesn't support the
lgtm.ymlfiles).I've put this up for review anyway, since that seemed like a good idea.