Refactor executor to pass shell arguments as array #57

Discookie · 2022-01-26T20:56:59Z

Closes #56.

Previously, we .join(' ')-ed the executor arguments. With this refactor, the executor no longer constructs the full command-line, but rather directly passes the args array to child_process.spawn.

For the user-facing input (arguments setting) and output (command-line preview), the shell-quote package is used.

steakhal

Looks great.

src/backend/executor/bridge.ts

steakhal · 2022-01-27T08:44:06Z

package.json

@@ -228,5 +226,8 @@
    "vsce": "^1.100.1",
    "webpack": "^5.19.0",
    "webpack-cli": "^4.4.0"
+  },
+  "dependencies": {
+    "shell-quote": "^1.7.3"


The type annotations are for a different version of this library.

As far as I can tell, the public API hasn't changed between the patch versions - .2 was a regression fix, and .3 was a CVE fix.
There's no newer version of the @types either, so it should be fine like this.

steakhal · 2022-01-27T08:45:59Z

package.json

          "description": "Path to a custom compilation database, in case of a custom build system",
          "default": null,
          "order": 3
        },
        "codechecker.executor.arguments": {
          "type": "string",
-          "description": "Additional arguments to CodeChecker analyze command. For supported arguments, run `CodeChecker analyze --help`. The command `CodeChecker: Show full command line` command shows the resulting command line.",
+          "description": "Additional arguments to CodeChecker analyze command. For supported arguments, run `CodeChecker analyze --help`. The command `CodeChecker: Show full command line` command shows the resulting command line. Also accepts arguments in a string-array form.",


Also accepts arguments in a string-array form.

What is the syntax for that? Should I use a brackets [ .. ] or only a comma-separated list?
Which should the user prefer?

Decided to remove the string-array input by the user in the end.

Basically I had 3 options:

Don't let the user input string-arrays

Let the user input the array in the textbox - but it's a minefield since there's no syntax or other kind of checks inside the textbox.

Let the user input them as literal arrays in the settings.json - but this way the user doesn't have a textbox to enter args, only a convoluted Edit in settings.json button.

And the other options both sacrifice considerable UX.

The user can still escape the characters properly themselves using one of their own tools if need be, and shell-quote should still be able to parse those args the same way.

How should one interpret the added sentence?
If there is no clear interpretation, why do we need that? Wouldn't it confuse the users?

Forgot to say the sentence was removed, but it is not there anymore, so it should be fine as is.

steakhal

Looks like the escape bug is fixed.
I have a couple of comments though.

steakhal · 2022-02-08T09:13:54Z

src/backend/executor/bridge.ts

+        };
+        for (const [key, val] of Object.entries(process.env)) {
+            if (val !== undefined) {
+                env[`env.${key}`] = val;


Shouldn't it be env[key] = val instead?

The default VS Code settings handle environment variables a bit differently (docs), so the env variables are accessible via ${env:<env>} in eg. other settings as well.

VSCode doesn't provide an auto-substitute function, that's the only reason it is done manually here.

(There's also a typo here, env. instead of env:, fixed.)

Thanks. It looks correct.

steakhal · 2022-02-08T09:18:05Z

src/backend/executor/bridge.ts

        const filePaths = files.length
-            ? `--file ${files.map((uri) => `"${uri.fsPath}"`).join(' ')}`
-            : '';
+            ? ['--file', ...files.map((uri) => uri.fsPath)]
+            : [];


Could the parse command contain an option specifying the skiplist file?
--file and --skip options are mutually exclusive options AFAIK.

Additionally to this, the file option will parse the file path and do a lookup in the compdb.json to find the corresponding entry for that TU. However, if there are symbolic links, the two might mismatch. It happens for me at least, and it basically a blocker for analyzing any files. However, we might need to fix this on the CodeChecker side. Im not sure.

Indeed they are, created an issue for this: #74.

I'm not sure if it's even possible to solve the second issue on our side without resolving all paths in the comp. db.

Yea, well. I saw once that the plugin copied the json. That being said, you can lookup all entries there and resolve than replace the occurrances. Basically patching the local copy db, where the canonical representation would be the resolved paths to the given TU. Keep in mind that the original db cannot be modified, since that might be generated by cmake for example. Alternatively you should document this behavior and even better, have a foolproof test in code and a popup-hint that there is a mismatch. You should 'log' and create a db.json using the resolved path as a working directory. BTW the CodeChecker ldlogger should be able to workaround this, but thats a different topic.

csordasmarton · 2022-02-16T10:57:37Z

@Discookie can you please rebase your branch. It contains some merge conflict. I would like to try it out and merge it if I don't find any problem.

csordasmarton

Resolve the merge conflict.

With multiple types, some string-settings did not show up as textboxes.

csordasmarton

Unfortunately I wasn't able to review this patch fully because when I tried it out I got a version check error when I started the VSCode from your branch.

csordasmarton · 2022-02-18T07:26:30Z

src/backend/executor/bridge.ts

-        const ccPath = getConfigAndReplaceVariables('codechecker.executor', 'executablePath')
-            ?? 'CodeChecker';
-
+    public getVersionCmdArgs(): string[] | undefined {


When I started VSCode with your branch I got the following error:

In the console I saw this:

>>> Process 'CodeChecker version' errored: spawn ~/CodeChecker/bin/CodeChecker ENOENT >>> CodeChecker error while checking version

In the settings I set the CodeChecker binary path to ~/CodeChecker/bin/CodeChecker. Previously it worked so I think there is some bug in your patch.

Also please change this line to analyzer-version because the error message above is a little bit misleading (we do not run CodeChecker version command but CodeChecker analyzer-version):

CodecheckerVSCodePlugin/src/backend/executor/process.ts

Line 17 in 7f628b8

version = 'CodeChecker version',

The problem is that my CodeChecker executable path started with ~ and the spawn doesn't work properly with such cases. See: nodejs/node#684

Updated the docs to say the setting requires an absolute path.

This is not enough. ~/CodeChecker/bin/CodeChecker is an absolute path and I still get this exception.

If we do not remove { shell: true } option from the spawn method it will work:

CodecheckerVSCodePlugin/src/backend/executor/process.ts

Line 113 in 1998f26

this.activeProcess = child_process.spawn(this.commandLine, { shell: true });

Otherwise we have to untildify the path.

Maybe the first option is better and simplier because in that case we don't have to untildify every path arguments.

We should still remove shell true.

csordasmarton

LGTM!

Discookie requested a review from csordasmarton January 26, 2022 20:56

Discookie added bugfix refactoring labels Jan 26, 2022

steakhal reviewed Jan 27, 2022

View reviewed changes

csordasmarton added this to the 1.1.1 milestone Jan 31, 2022

Discookie force-pushed the ericsson/fix-serialize-path branch 2 times, most recently from f85d611 to b8ab95a Compare February 2, 2022 02:48

steakhal approved these changes Feb 8, 2022

View reviewed changes

Discookie force-pushed the ericsson/fix-serialize-path branch from b8ab95a to 679704d Compare February 8, 2022 21:49

csordasmarton modified the milestones: 1.2.0, 1.3.0 Feb 14, 2022

csordasmarton suggested changes Feb 16, 2022

View reviewed changes

Discookie added 4 commits February 17, 2022 01:57

Refactor executor to escape characters in paths

6fa00c0

Remove multiple types from settings

9249b85

With multiple types, some string-settings did not show up as textboxes.

Fix 0-length arguments when parsing

7571f85

Fix replacing ${workspaceFolder} in analyze args

f95fbd6

Discookie force-pushed the ericsson/fix-serialize-path branch from 679704d to f95fbd6 Compare February 17, 2022 03:36

Discookie requested a review from csordasmarton February 17, 2022 03:36

csordasmarton suggested changes Feb 18, 2022

View reviewed changes

Discookie added 2 commits February 22, 2022 15:03

Update docs to reflect the full path requirement

7492352

Fix handling empty executablePath setting

0c23130

Discookie requested a review from csordasmarton February 22, 2022 14:17

csordasmarton approved these changes Feb 23, 2022

View reviewed changes

csordasmarton merged commit a458bc9 into Ericsson:main Feb 23, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor executor to pass shell arguments as array #57

Refactor executor to pass shell arguments as array #57

Discookie commented Jan 26, 2022

steakhal left a comment

steakhal Jan 27, 2022

Discookie Feb 1, 2022

steakhal Jan 27, 2022

Discookie Feb 1, 2022

Discookie Feb 1, 2022

steakhal Feb 8, 2022

Discookie Feb 8, 2022

steakhal left a comment

steakhal Feb 8, 2022

Discookie Feb 8, 2022

steakhal Feb 8, 2022

steakhal Feb 8, 2022

steakhal Feb 8, 2022

Discookie Feb 8, 2022

Discookie Feb 8, 2022

steakhal Feb 8, 2022

csordasmarton commented Feb 16, 2022

csordasmarton left a comment

csordasmarton left a comment

csordasmarton Feb 18, 2022

csordasmarton Feb 21, 2022

Discookie Feb 22, 2022

csordasmarton Feb 22, 2022

steakhal Feb 22, 2022

csordasmarton left a comment

Refactor executor to pass shell arguments as array #57

Refactor executor to pass shell arguments as array #57

Conversation

Discookie commented Jan 26, 2022

steakhal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

steakhal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

csordasmarton commented Feb 16, 2022

csordasmarton left a comment

Choose a reason for hiding this comment

csordasmarton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

csordasmarton left a comment

Choose a reason for hiding this comment