Convert init, autobuild, and analyze actions to CLI / runner mode #162

robertbrignull · 2020-08-25T17:20:15Z

This PR adds a CLI interface to the rest of the actions. This should get us to a state where we have a working product, though it's very rough around the edges and there will be some tech debt cleanup to do.
(Currently depends on #159 but I'll rebase once that PR is merged)

This PR is currently quite rough and not very pretty, but I think it largely works. I've done a single manual test and was about to run the init and analyze actions on a javascript repository.

Problems I have with this current situation are:

Some methods have far too many arguments and they are all the same type so it's easy to get them mixed up. We should look into some way of doing named arguments, or packaging related args up into objects.
There's an awful hack of setting RUNNNER_TEMP so we can use the existing toolcache code. We shouldn't be doing this and should instead write our own implementation for use outside of actions.
The temp directory situation is still a bit awkward but we're discussing how to improve that.

@sampart, @joshhale, @chrisgavin, @jhutchings1

Merge / deployment checklist

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

robertbrignull · 2020-08-25T17:22:17Z

Realised just after posting I've missed the bit with outputting the env vars to a file so you can source them. I'll do that tomorrow morning.

robertbrignull · 2020-08-26T13:05:53Z

Added code to generate the env file and instructions for how to use it. Also had to inline a couple of files that we depended on just existing, because now we have to write them out to a temporary location ourselves prior to using them.

I've tested this on linux building some javascript code. Unfortunately it doesn't work for me tracing a compiled language as the tracer is not intercepting so it claims nothing has been built. I will investigate but I haven't been able to work out what's up yet.

robertbrignull · 2020-08-26T13:08:15Z

Things still to do:

Work out why the environment vars aren't working to inject the tracer.
Inline inject-tracer.ps1 file
Check at start of autobuild/analze commands that env vars are set, to avoid wasting time and improve error message

robertbrignull · 2020-08-26T15:48:16Z

I've managed to analyze a cpp project on linux now. Had some errors in the setting of env vars.

sampart · 2020-08-27T13:28:38Z

We should look into some way of doing named arguments, or packaging related args up into objects.

I like both of these approaches. You could also consider introducing some new types for things like pathString, urlString etc. These would simply be defined as string, but would allow nice type checking.

robertbrignull · 2020-08-27T13:36:57Z

I like both of these approaches.

I just want to say that although I would like to do these too I am worried they will eat up a lot of time as we try to find better solutions. If it's acceptable then I would like to go for a correct implementation for now and look at improving it in later pull requests.

You could also consider introducing some new types for things like pathString, urlString etc. These would simply be defined as string, but would allow nice type checking.

Typescript unfortunately doesn't work that way. If you define new types like type pathString = string and type urlString = string then both are interchangeable and it won't be an error to pass one instead of the other.

sampart · 2020-08-27T13:58:55Z

If it's acceptable then I would like to go for a correct implementation for now and look at improving it in later pull requests.

Absolutely - don't add any more in this PR!

Typescript unfortunately doesn't work that way. If you define new types like type pathString = string and type urlString = string then both are interchangeable and it won't be an error to pass one instead of the other.

Oh, rubbish.

sampart

Thakns for all you work here. A number of comments, but they're all small ones.

Do you think it would have been feasible to do this as 3 PRs, one for each of init, autobuild and analyze, or would the first one have had a lot of "baggage" that the others would need and so have been huge anyway?

sampart · 2020-08-27T13:24:28Z

src/codeql.ts

-  // e.g. our integration tests which use the Action code from the current checkout.
-  if (relativeScriptPath.startsWith("..") || path.isAbsolute(relativeScriptPath)) {
+function getCodeQLActionRepository(mode: util.Mode): string {
+  if (mode === 'actions') {


You could avoid nesting here by using a guard clause. Start with

if (mode !== 'actions') { return CODEQL_DEFAULT_ACTION_REPOSITORY; }

and then the rest of the code needn't be in a block.

This has the downside of introducing a ! condition, but the upsides of reducing nesting and also preventing the reader getting to the else and thinking "which condition does this relate to again?" (as I did)

I agree it's nicer code. The thing that held me back initially is that I'm not sure the unguarded-action-lib query will understand this, but really that just means we should improve the query.

sampart · 2020-08-27T13:26:29Z

src/codeql.ts

 }

-// We have to download CodeQL manually because the toolcache doesn't support Accept headers.
-// This can be removed once https://github.com/actions/toolkit/pull/530 is merged and released.


Did you mean to remove this comment? The PR it mentions hasn't been merged yet.

Not really. I am worried we may not be able to go back to using the toolcache and may need to carry on implementing this ourselves even once that PR is eventually merged. However I'm not trying to change that assumption in this PR, so I'll revert this to add back the comment.

sampart · 2020-08-27T13:30:16Z

src/codeql.ts

+  mode: util.Mode,
+  logger: Logger): Promise<CodeQL> {
+
+  // Setting these two env vars makes the toolcache code safe to use,


Suggested change

// Setting these two env vars makes the toolcache code safe to use,

// Setting these two env vars makes the toolcache code safe to use outside of actions,

You'll probably want to implement this change yourself (assuming you agree with it), though, so you can make the lines line up.

Without a change like this, it implies that the toolcache code is never safe to use!

sampart · 2020-08-27T13:32:54Z

src/codeql.ts

    getTracerEnv: async function(databasePath: string) {
-      let envFile = path.resolve(databasePath, 'working', 'env.tmp');
+      // Write tracer-env.js to a temp location. When running in CLI mode we can't rely
+      // on this file existing so we have to create it ourselves.


Where does it come from in Actions land? It would be good to mention that here so that future developers can compare the file contents below with the canonical version.

There's no difference in behaviour on actions, so I'll delete that bit of the comment about when running in CLI mode as it always applies.

What I meant is that currently we rely on there being a checkout of this repository present so we can reference arbitrary files in it. That's what we can't do when running in CLI mode.

sampart · 2020-08-27T13:41:17Z

src/init-action.ts

      util.getRequiredEnvParam('RUNNER_TEMP'),
      util.getRequiredEnvParam('RUNNER_TOOL_CACHE'),
-      codeql);
-    analysisPaths.includeAndExcludeAnalysisPaths(config);


You don't call this in initConfig (although you do call analysisPaths.printPathFiltersWarning(config, logger);). Should you?

This call has been moved to createdDBForScannedLanguages in analyze.ts as that's where the env vars are actually used. It's replaced here with printPathFiltersWarning so we still print the warning in the init step where it comes currently.

sampart · 2020-08-27T14:52:49Z

src/runner.ts

+  .option('--queries <queries>', 'Comma-separated list of additional queries to run. By default, this overrides the same setting in a configuration file.')
+  .option('--config-file <file>', 'Path to config file')
+  .option('--codeql-path <path>', 'Path to a copy of the CodeQL CLI executable to use. Otherwise downloads a copy.')
+  .option('--temp-dir <dir>', 'Directory to use for temporary files. By default will use current working directory.')


Suggested change

.option('--temp-dir <dir>', 'Directory to use for temporary files. By default will use current working directory.')

.option('--temp-dir <dir>', 'Directory to use for temporary files. By default will use a subdirectory of the current working directory.')

sampart · 2020-08-27T14:53:02Z

src/runner.ts

+  .command('autobuild')
+  .description('Attempts to automatically build code')
+  .option('--language <language>', 'The language to build. By default will try to detect the dominant language.')
+  .option('--temp-dir <dir>', 'Directory to use for temporary files. By default will use current working directory.')


Suggested change

.option('--temp-dir <dir>', 'Directory to use for temporary files. By default will use current working directory.')

.option('--temp-dir <dir>', 'Directory to use for temporary files. By default will use a subdirectory of the current working directory.')

sampart · 2020-08-27T14:57:06Z

src/runner.ts

+      } else {
+        language = determineAutobuildLanguage(config, logger);
+      }
+      if (language !== undefined) {


Do you need an else here - a conditional after the call to determineAutobuildLanguage above - to print a warning if the language is undefined?

It's a bit convoluted but no, because determineAutobuildLanguage prints a message saying "None of the languages in this project require extra build steps" and then the intention is that we exit gracefully and successfully.

sampart · 2020-08-27T14:58:01Z

src/runner.ts

+  .option('--checkout-path <path>', 'Checkout path (default: current working directory)')
+  .option('--no-upload', 'Do not upload results after analysis', false)
+  .option('--output-dir <dir>', 'Directory to output SARIF files to. By default will use temp directory.')
+  .option('--temp-dir <dir>', 'Directory to use for temporary files. By default will use current working directory.')


Suggested change

.option('--temp-dir <dir>', 'Directory to use for temporary files. By default will use current working directory.')

.option('--temp-dir <dir>', 'Directory to use for temporary files. By default will use a subdirectory of the current working directory.')

sampart · 2020-08-27T15:01:25Z

src/runner.ts

+        config,
+        logger);
+    } catch (e) {
+      logger.error('Upload failed');


Suggested change

logger.error('Upload failed');

logger.error('Analyze failed');

sampart

A couple of typos

sampart · 2020-08-28T09:37:47Z

src/runner.ts

+      const config = await getConfig(getTempDir(cmd.tempDir), logger);
+      if (config === undefined) {
+        throw new Error("Config file could not be found at expected location. " +
+          "Was the 'init' command run with the same '--temp-dir' argument as this command.");


Suggested change

"Was the 'init' command run with the same '--temp-dir' argument as this command.");

"Was the 'init' command run with the same '--temp-dir' argument as this command?");

sampart · 2020-08-28T09:37:58Z

src/runner.ts

+      const config = await getConfig(getTempDir(cmd.tempDir), logger);
+      if (config === undefined) {
+        throw new Error("Config file could not be found at expected location. " +
+          "Was the 'init' command run with the same '--temp-dir' argument as this command.");


Suggested change

"Was the 'init' command run with the same '--temp-dir' argument as this command.");

"Was the 'init' command run with the same '--temp-dir' argument as this command?");

robertbrignull · 2020-08-28T12:22:41Z

Bugs that have been discovered and need fixing:

It breaks if there's a space in the path (possibly only on windows)

Things to do:

Make it so autobuild adds all env vars internalls so you don't need to explicitly source them if using the autobuild command.

robertbrignull · 2020-08-28T16:05:23Z

The issue with the spaces in the filepath was due to exec from @actions/exec "helpfully" splitting the command string on spaces. It implements escaping so we possibly could have wrapped our command in quotes, but that seems like a route for future problems. Instead I've replaced the uses with ToolRunner from @actions/exec/lib/toolrunner which is what exec uses internally and doesn't do the splitting.

robertbrignull · 2020-08-28T16:24:03Z

I had the idea that in the case that people are able to use the autobuilder it would be nice if they didn't have to source the env explicitly. Doing this is largely fine, but it makes the check at the start of the analyze command a bit odd, so I've removed that for now. I'll test and see what the error message from CodeQL is like in the case that you forget to source the env.

robertbrignull · 2020-08-28T16:50:59Z

Currently getting a weird error on windows. Not sure if it's something I've done wrong in the configuration or not

[command]"C:\Users\User\My Documents\codeql-runner-tools\CodeQL\0.0.0-20200826\x64\codeql\codeql.exe" resolve queries cpp-code-scanning.qls --format=bylanguage
Warning: C:\Users\User\My Documents\codeql-runner-tools\CodeQL\0.0.0-20200826\x64\codeql\.codeqlmanifest.json: Could not create real path
(eventual cause: AccessDeniedException "C:\Users\User\My Documents\codeql-runner-tools\CodeQL\0.0.0-20200826\x64\codeql\.codeqlmanifest.json")
Warning: C:\Users\User\My Documents\codeql-runner-tools\CodeQL\0.0.0-20200826\x64\codeql-go\.codeqlmanifest.json: Could not create real path
(eventual cause: AccessDeniedException "C:\Users\User\My Documents\codeql-runner-tools\CodeQL\0.0.0-20200826\x64\codeql-...")
Warning: C:\Users\User\My Documents\codeql-runner-tools\CodeQL\0.0.0-20200826\x64\ql\.codeqlmanifest.json: Could not create real path
(eventual cause: AccessDeniedException "C:\Users\User\My Documents\codeql-runner-tools\CodeQL\0.0.0-20200826\x64\ql\.codeqlmanifest.json")
A fatal error occurred: cpp-code-scanning.qls is not a well-known query suite.
Init failed
Error: The process 'C:\Users\User\My Documents\codeql-runner-tools\CodeQL\0.0.0-20200826\x64\codeql\codeql.exe' failed with exit code 2
    at ExecState._setResult (C:\snapshot\dist\codeql-runner.js)
    at ExecState.CheckComplete (C:\snapshot\dist\codeql-runner.js)
    at ChildProcess.<anonymous> (C:\snapshot\dist\codeql-runner.js)
    at ChildProcess.emit (events.js:315:20)
    at maybeClose (internal/child_process.js:1021:16)
    at Process.ChildProcess._handle.onexit (internal/child_process.js:286:5)

Possibly it's another problem with having a space in the path, as I specifically put the tools dir under My Documents to test this.

robertbrignull · 2020-09-01T14:01:31Z

Although the work on the runner for 3rd party CI/CD is not yet finished (mostly unfinished on windows) I'm moderately confident this doesn't break anything on actions, so we should likely merge this now so we get the code in main. It won't go to v1 for another week so we'll get maximum amount of time for pasive testing.

greysteil · 2020-09-14T10:53:33Z

@robertbrignull you merged this with outstanding code scanning results on it. What was your thinking on doing that rather than marking those results as won't fix / false positives (or, indeed, fixing them if they were accurate)?

robertbrignull · 2020-09-14T11:09:48Z

you merged this with outstanding code scanning results on it. What was your thinking on doing that rather than marking those results as won't fix / false positives (or, indeed, fixing them if they were accurate)?

It was purely a matter of forgetfulness. I saw the restults and noted that none of them we're serious enough to block merging, but then forgot to follow up later. I'll go through the alerts now and see which can be closed or fixed.

greysteil · 2020-09-14T12:04:51Z

👍 just curious if there's anything we can learn from it as we polish the UI

robertbrignull added 6 commits August 26, 2020 16:20

Convert rest of the actions

217483d

Add code to output required env to files

3ffe4b7

Fix use of wrong URL

f5d645f

inline tracer-env.js

1c004b9

add newline to output

1f2bd10

inline inject-tracer.ps1

e9bfa56

robertbrignull force-pushed the runner_analyze branch from 27fd829 to e9bfa56 Compare August 26, 2020 15:20

fix exporting env vars on linux

688df28

Remove dependence of GITHUB_REPOSITORY env var

39b361e

robertbrignull force-pushed the runner_analyze branch from 854b2a7 to 39b361e Compare August 27, 2020 10:06

sampart self-requested a review August 27, 2020 11:23

robertbrignull added 2 commits August 27, 2020 14:08

Update temp dir and tools dir

b42ed69

make --language optional to autobuild and detect dominant language

a542021

robertbrignull force-pushed the runner_analyze branch from 00463b1 to a542021 Compare August 27, 2020 13:09

robertbrignull added 2 commits August 27, 2020 14:22

check environment before running autobuild or analyze

f80d660

add debug mode to limit output

6f422a4

sampart approved these changes Aug 27, 2020

View reviewed changes

robertbrignull added 6 commits August 27, 2020 16:34

address review comments

1fd45d7

Remove process of auth

3dfaa88

Log that we're clearing the temp dir

6c8f96d

Remove hash from temp dir

57f03d3

Make runner arg descriptions more consistent

37bac22

improve error message when config is not found

80e2c4f

sampart approved these changes Aug 28, 2020

View reviewed changes

robertbrignull mentioned this pull request Aug 28, 2020

Run npm test on linux and macos #164

Merged

2 tasks

use ToolRunner directly instead of exec wrapper

c3d6602

automatically import env in autobuild

aa7e2fe

robertbrignull added 2 commits August 28, 2020 17:28

fix tests

a0b54fc

Merge remote-tracking branch 'origin/main' into runner_analyze

1548b77

robertbrignull added 2 commits September 1, 2020 13:53

Add logger to checkoutExternalRepository

8a821a9

Remove a call to getActionsApiClient

09fb3ec

robertbrignull force-pushed the runner_analyze branch from d718a59 to 5bf581b Compare September 1, 2020 13:18

Add --ram and --threads args

4c00c68

robertbrignull force-pushed the runner_analyze branch from 5bf581b to 6882791 Compare September 1, 2020 13:28

whitelist @actions/exec/lib/toolrunner

b4d142e

robertbrignull force-pushed the runner_analyze branch from 6882791 to b4d142e Compare September 1, 2020 13:44

robertbrignull merged commit 68c6069 into main Sep 1, 2020

robertbrignull deleted the runner_analyze branch September 1, 2020 14:01

github-actions bot mentioned this pull request Sep 7, 2020

Merge main into v1 #171

Merged

aliscco mentioned this pull request Apr 25, 2023

[Snyk] Fix for 13 vulnerabilities aliscco/codeql-action#142

Open

	// Setting these two env vars makes the toolcache code safe to use,
	// Setting these two env vars makes the toolcache code safe to use outside of actions,

	.option('--temp-dir <dir>', 'Directory to use for temporary files. By default will use current working directory.')
	.option('--temp-dir <dir>', 'Directory to use for temporary files. By default will use a subdirectory of the current working directory.')

	logger.error('Upload failed');
	logger.error('Analyze failed');

	"Was the 'init' command run with the same '--temp-dir' argument as this command.");
	"Was the 'init' command run with the same '--temp-dir' argument as this command?");

Convert init, autobuild, and analyze actions to CLI / runner mode #162

Convert init, autobuild, and analyze actions to CLI / runner mode #162

Uh oh!

Conversation

robertbrignull commented Aug 25, 2020

Merge / deployment checklist

Uh oh!

robertbrignull commented Aug 25, 2020

Uh oh!

robertbrignull commented Aug 26, 2020

Uh oh!

robertbrignull commented Aug 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

robertbrignull commented Aug 26, 2020

Uh oh!

sampart commented Aug 27, 2020

Uh oh!

robertbrignull commented Aug 27, 2020

Uh oh!

sampart commented Aug 27, 2020

Uh oh!

sampart 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!

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!

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!

sampart 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!

robertbrignull commented Aug 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

robertbrignull commented Aug 28, 2020

Uh oh!

robertbrignull commented Aug 28, 2020

Uh oh!

robertbrignull commented Aug 28, 2020

Uh oh!

robertbrignull commented Sep 1, 2020

Uh oh!

greysteil commented Sep 14, 2020

Uh oh!

robertbrignull commented Sep 14, 2020

Uh oh!

greysteil commented Sep 14, 2020

Uh oh!

Reviewers

robertbrignull commented Aug 26, 2020 •

edited

Loading

robertbrignull commented Aug 28, 2020 •

edited

Loading