-
Notifications
You must be signed in to change notification settings - Fork 309
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
Make use of multi-language and indirect tracing #744
Conversation
9f5383d
to
3ec870a
Compare
I agree with your comment about |
Are you sure this step is part of the CodeQL Action rather than something that is added onto workflows being debugged in a separate step using codeql-action/src/tracer-config.ts Line 114 in 7112cda
We could certainly create a spec to put the log in the same place to not break anyone uploading the tracer logs, but since my understanding is the log uploading is only done temporarily while debugging a workflow, I don't think we want to do this as it seems overkill to define a new tracer spec just for this reason (previously, we needed to define one anyway to combine the environment). |
Ah, then I probably misremembered. Then keeping it as-is is fine! I'd rather now override anything in the spec file, as the whole point of moving this into the CLI was to not depend on tracer internals in the action anymore. CC @aibaars maybe you have something to contribute to this conversation? |
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.
Looks sensible, but we also need to change util.ts
such that the action doesn't pass CODEQL_FEATURE_MULTI_LANGUAGE
and CODEQL_FEATURE_SANDWICH
when it uses the CLI features. Those feature flags specifically indicate that the Action's DIY implementation is used.
# Note we want to make sure that the .win32env file is read correctly, so we unset the CODEQL_EXTRACTOR_CSHARP_ROOT from the .sh file. | ||
run: | | ||
cat ./codeql-runner/codeql-env.sh | Invoke-Expression | ||
$Env:CODEQL_EXTRACTOR_CSHARP_ROOT = "" |
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.
IIUC, the tracer is still supposed to be resistant towards build systems that clobber nonessential environment variables like this one. Even if the concrete regression this test was introduced to catch, wouldn't it be a good idea to keep it present, as defense in depth against tracer breakage?
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 is a good point - in fact, I'm not convinced this test was ever testing that the .win32env
file was doing what it should as the tracer is resistant towards the unsetting the environment variable as you say. I've left the code as it is, and updated the comment to better describe what is going on here.
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.
The .win32env
file is how the tracer resists unsetting ...
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.
Ah, right. So if I understand correctly, before this the test was checking that the .win32env
file that the Action created for the new compound environment it synthesised worked correctly. Now, the test is just checking that the tracer is resistant to environment variables being unset by whatever mechanism it chooses which happens to also be creating a .win32env
file.
3ec870a
to
cc955cc
Compare
Thanks @hmakholm, I totally forgot I should've removed those! I've done that now and also rebased to fix a merge conflict. |
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.
We don't want to remove the feature flags unconditionally. Starting from release 2.5.7, the CLI depends on seeing CODEQL_ACTION_FEATURE_MULTI_LANGUAGE
for enabling special-case code that will set CODEQL_SCRATCH_DIR
consistently between languages. So for release 2.5.7, 2.5.8 and 2.5.9 it is important that the action passes the variable since then it is actually doing DIY language combination then.
I can see it doesn't look entirely trivial to make initializeEnvironment
depend on the CodeQL version, but it's a problem that I think we'll need to solve at some point anyway.
A possible quick-and-dirty workaround could be to set the feature flag explicitly in Even though that doesn't set it for all |
Right, whoops. As you say, making The choices are I think two:
I think (1) is the more morally right solution, so I'll have a look at how tricky that is to implement before possibly resigning myself to the more hacky solution. |
cc955cc
to
1f4460b
Compare
Okay, so (1) above doesn't quite work because there are code paths where we care about some of the environment variables but don't actually have a CodeQL CLI available. In particular, the upload SARIF step doesn't currently use a CLI and it's my understanding that we may not want to make it need one because some users are using this as a way to upload SARIF from non-CodeQL tools to Code Scanning. However, this step also does care about some of the environment variables (in particular, it sends a status report that includes the version of the Action for telemetry purposes). Thus, I've gone for splitting the variables into two groups - one that we unconditionally set at the start of every run as we do now, and another that we set only on code paths where we use a CLI once we have enough information to know what CLI version we are going to use. |
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 looks like it has a chance of working :-)
I'm happy with merging this as is now, but just floating a suggestion: instead of needing to remember to call enrichEnvironment
in the right places, we could also make it a side effect of the first getCodeQL
that actually reads the CLI version.
Hmm, that does sound very sensible in principle. However, there's a small issue with that - we need to know whether we are running on Actions or the Runner, as we set the environment variables differently depending on which scenario we are in and that information isn't readily available in |
This PR changes the CodeQL Action to use the new
--db-cluster
(available in CLI 2.5.6+) and--begin-tracing
(available in CLI 2.6.0+) options. I think it makes sense to use these two options together or not at all, as the two are closely intertwined so if we wanted to support using just--db-cluster
in CLIs 2.5.6 to 2.6.0 this would add another path to the code. I do not think there is a need for this.Mostly, the tests do not need changing as the existing integration tests will use this functionality when testing against new versions of the CLI. One exception is the test modified by #495 to check that the
.win32env
file created by the action is read successfully. My understanding is that now that we have the proper solution to the above using multi-language tracing, we no longer need to create that file in the Action, nor do we need to have that modification to the test.Merge / deployment checklist