Build tracing by sauyon · Pull Request #324 · github/codeql-go

sauyon · 2020-09-07T07:21:59Z

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.yml files).

I've put this up for review anyway, since that seemed like a good idea.

max-schaefer

Thank you very much for persisting with this! It's pleasantly simple, I only have a couple of fairly minor questions and suggestions.

max-schaefer · 2020-09-07T07:43:33Z

@@ -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.


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.

@sauyon noting this is still to do. Do we have a firm answer on how a user would opt in?

I believe that that was likely just the discussion of variable naming.

Cool, in that case mention how it's done in the change-note

smowton

Looks like all my comments apply to commits that are later improved (consider squashing)

smowton · 2020-09-07T10:33:05Z

+ } else if !mimic && args[i] == "--help" {
 usage()
 os.Exit(0)
+ } else if args[i] == "--mimic" {


For regularity we should accept --mimic=

smowton · 2020-09-07T10:34:28Z

+ }
+ }
+ } else {
+ log.Fatalf("Invalid --mimic: no compiler specified")


Give an example in the error message, like --mimic must take an argument, e.g. --mimic go

smowton · 2020-09-07T10:42:25Z

+ i++
+ compiler := args[i]
+ log.Printf("Compiler: %s", compiler)
+ if i+1 < len(args) {


Worth printing a warning if this isn't true, since the command argument is required

Except this one looks real

smowton · 2020-09-07T10:45:55Z

+ log.Printf("Intercepting build")
+ // skip `-o output` and `i`, if applicable
+ for i+1 < len(args) {
+ if args[i+1] == "-o" {


If we're skipping these we should skip them wherever they occur in the arg list, not break after the first good argument.

sauyon · 2020-09-10T09:46:27Z

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...

max-schaefer · 2020-09-22T14:13:28Z

As discussed elsewhere, could we rename the environment variable for turning on build tracing from CODEQL_EXTRACTOR_GO_TRACE to CODEQL_EXTRACTOR_GO_BUILD_TRACING for clarity?

sauyon · 2020-10-14T15:46:12Z

The experiments I ran did nothing to improve the Kubernetes situation, so I think merging this now is a sensible thing to do.

smowton

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?

See https://github.com/github/semmle-code/pull/37764

Co-authored-by: Chris Smowton <smowton@github.com>

sauyon · 2020-10-14T16:46:11Z

I believe I have now actually addressed all comments.

smowton

Nice, looks good! @max-schaefer are you happy to drop your block on this?

max-schaefer · 2020-10-15T10:26:34Z

Yep, let's give this a try!

smowton · 2020-10-15T10:27:31Z

@sauyon please link the issue here when you've got a writeup

sauyon changed the title ~~Blejd~~Build tracingSep 7, 2020

max-schaefer suggested changes Sep 7, 2020
View reviewed changes

smowton approved these changes Sep 7, 2020
View reviewed changes

max-schaefer mentioned this pull request Sep 7, 2020
Add support for build tracing #178
Closed

sauyon force-pushed the tracing branch 2 times, most recently from b3744ec to 9aab773Compare September 10, 2020 07:50

max-schaefer reviewed Sep 10, 2020
View reviewed changes

Comment threadcodeql-tools/autobuild.cmd Outdated

sauyon force-pushed the tracing branch from 440dab2 to 2cea9bfCompare September 10, 2020 09:39

sauyon force-pushed the tracing branch from 8bae556 to 2cea9bfCompare September 10, 2020 09:58

robertbrignull mentioned this pull request Sep 18, 2020
Check CODEQL_EXTRACTOR_GO_BUILD_TRACING and treat Go as a traced language github/codeql-action#222
Merged
2 tasks

Sauyon Lee added 6 commits October 8, 2020 23:31

Add package directory as a wanted root
1679652

Enable Go modules while determining module directory
eaf5342

extractor: factor out run from autobuilder
2da89c6

extractor: revamp argument parsing
cd63ea8

autobuilder: extract out attempted build commands
de0582a

Add a new binary for tracing
85c9225

sauyon force-pushed the tracing branch from 2fb3348 to 633a12cCompare October 9, 2020 06:31

smowton reviewed Oct 14, 2020
View reviewed changes

Comment threadextractor/cli/go-extractor/go-extractor.go Outdated

Sauyon Lee added 3 commits October 14, 2020 09:42

Add change note for build tracing
3addb96

Don't trace through problem binaries on OS X
3c6626c
See https://github.com/github/semmle-code/pull/37764

autobuilder: Clarify error message
25eebe9

sauyon force-pushed the tracing branch from 633a12c to 25eebe9Compare October 14, 2020 16:42

go-extractor: clarify --mimic error message
e5afd1d
Co-authored-by: Chris Smowton <smowton@github.com>

smowton approved these changes Oct 15, 2020
View reviewed changes

max-schaefer approved these changes Oct 15, 2020
View reviewed changes

smowton merged commit 2b07e6a into github:mainOct 15, 2020

sauyon deleted the tracing branch November 9, 2020 10:04

smowton added the has-change-note label Nov 20, 2020

		@@ -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.

Uh oh!

Conversation

sauyon commented Sep 7, 2020• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

max-schaefer left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

smowton left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sauyon commented Sep 10, 2020

Uh oh!

max-schaefer commented Sep 22, 2020

Uh oh!

sauyon commented Oct 14, 2020

Uh oh!

smowton left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sauyon commented Oct 14, 2020

Uh oh!

smowton left a comment

Choose a reason for hiding this comment

Uh oh!

max-schaefer commented Oct 15, 2020

Uh oh!

smowton commented Oct 15, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

sauyon commented Sep 7, 2020•
edited
Loading