Fixes when there are spaces in test target name #47

lukester1975 · 2021-11-17T19:37:04Z

Hi

Great that this extension is now part of the meson project and will hopefully get some love.

Here are some patches to fix a few issues with spaces in test target names and to use meson rather than ninja for all commands (I use samurai).

I other less trivial ones but thought I'd see if these are of interest first.

Regards

dcbaker

Thanks for this! I have one question, but otherwise this looks good to me

dcbaker · 2021-11-18T04:22:00Z

src/meson/runners.ts

+  const title = name ? `Building target ${name}` : "Building project";
+
+  getOutputChannel().append(`\n${title}\n`);
+  const stream = execStream(`meson compile ${name ?? ""}`, { cwd: buildDir });


shouldn't this be quoted too.

I'd only done test targets as surely nobody puts spaces in binary names 😀

That said, the one you reference ends up spawning and not going via the shell:

vscode-meson/src/utils.ts

Lines 34 to 42 in fbede29

export function execStream(

command: string | string[],

options: cp.SpawnOptions

) {

//FIXME: Force string array and fix callers

if (typeof command === "string") {

command = command.split(" ");

}

const spawned = cp.spawn(command[0], command.slice(1), options);

So quoting breaks it there. The FIXME would solve the problem I suppose, though I get the feeling from looking at the code some greater refactoring to tidy up the multiple routes things can take is the right solution...

I'll update the PR for one other compile command that is going via the shell for now.

lukester1975 · 2021-11-18T15:33:48Z

Actually scratch that. Thought I'd better check what Windows does (well, Powershell) and it is pretty horrible.

That said, looks like ShellQuoting might come to the rescue.

* typescript 2.4.1 -> 4.4.4 * vscode 1.1.34 -> 1.1.37 * types/node 12.0.2 -> 16.11.7

lukester1975 · 2021-11-18T20:44:54Z

OK here's a revised work in progress that works with powershell. Still draft as there's a few things I need to look at, but any feedback welcome in the meantime!

The main fix was switching from ShellExecution -> ProcessExecution to just avoid any shell escaping issues. No variable expansion was required in the places I've made that change, AFAICT, so should be safe.

#47 (comment) is also done now.

TODO:

Seemed reasonable to be consistent and use ProcessExecution everywhere, but there are 2 ShellExecutions left that I need to investigate to ensure no breakage.
Haven't removed exec() calls, which are also going via the shell. Probably a separate PR as not related to the actual issue, really.
Building a target in a sub-directory doesn't work on Windows... but that's nothing to do with these changes (current main branch doesn't work either). Looks to me like meson itself doesn't like backslashes in the target name. I'll try to take since I'm in that area.

Cheers

Meson requires the separator between path and target name to be '/'. ALso, since the full target name can now end up as "path\to/targetName", don't show path\to in the tree view (it's implicit from the nodes anyway).

Don't do that. Also removed unused exec() function.

…presumably) intended.

…ate command/args parameters. Will allow spaces in target names to work cross-platform (avoiding shell escaping issues). No uses require shell variable expansion, etc.

Should allow target names with spaces.

I.e. Meson -> Run Unit Tests -> all, or specific test name with spaces. Uses ProcessExecution rather than ShellExecution for ninja/meson invocation to avoid shell space escaping issues. No uses require variable expansion, etc.

Samurai now works, via NINJA=samu.

…ssues.

lukester1975 · 2021-11-19T15:35:09Z

OK here's a "final" version I'm happy with, with one caveat:

2. Haven't removed `exec()` calls, which are also going via the shell. Probably a separate PR as not related to the actual issue, really.

I did this in 84a3be0. But I could see an issue - if someone's used a shell variable in their configure options, then previously it would've been expanded but now won't be. Would be nice if there was an API to support vscode variable expansion and use that (e.g. microsoft/vscode#46471) but I guess not. So maybe this commit should be binned? There's no real downside (aside from being nice to consistently avoid the shell) unless someone has spaces in their build dir, AFAICT...

3. Building a target in a sub-directory doesn't work on Windows... but that's nothing to do with these changes (current main branch doesn't work either). Looks to me like `meson` itself doesn't like backslashes in the target name. I'll try to take since I'm in that area.

That was a pretty trivial fix. Meson seems to be happy as long as the separator between dir and target name is a forward slash.

Hope it's useful. Makes the extension usable for me anyway!

I've got a few more other fixes but unrelated, so might as well get this in first.

dcbaker · 2021-11-19T17:55:50Z

Would that also prevent shell variables that meson interpreters internally stop working? I do rely on that (PKG_CONFIG_PATH, in particular)

lukester1975 · 2021-11-19T18:38:10Z

Which bit in particular do you mean? The avoiding of the shell when running meson commands in general or the specific case I mention in 84a3be0?

For 84a3be0, what I'm saying is if you had say mesonbuild.configureOptions = [ "${SOME_VARIABLE}" ] (or in whatever format appropriate for your shell, I suppose) then there would be no expansion of ${SOME_VARIABLE} by the shell (as there isn't one) before invoking meson. I.e. it would literally receive ${SOME_VARIABLE} as a command line argument rather than its value.

It's the only case in the extension I could see where there might be an issue. All the other uses are just meson <command> [--maybe a switch] maybe a target name].

For the first case - whatever environment VSCode has will be inherited by the meson it runs, it's just that there's no longer a shell process in the middle. So wherever meson is doing env['PKG_CONFIG_PATH'] should get the same result.

HTH!

dcbaker · 2021-11-20T00:06:21Z

Yeah, that should be fine. I use nix, so having environment variables is pretty important for things to work, lol

dcbaker requested changes Nov 18, 2021

View reviewed changes

lukester1975 force-pushed the fix-spaces-in-target-name branch from 9f5c418 to 067ac05 Compare November 18, 2021 09:39

lukester1975 marked this pull request as draft November 18, 2021 15:34

build: package updates.

ea87abc

* typescript 2.4.1 -> 4.4.4 * vscode 1.1.34 -> 1.1.37 * types/node 12.0.2 -> 16.11.7

lukester1975 force-pushed the fix-spaces-in-target-name branch from 067ac05 to 8828b01 Compare November 18, 2021 20:22

lukester1975 force-pushed the fix-spaces-in-target-name branch 2 times, most recently from 999b67e to b48d540 Compare November 18, 2021 20:53

lukester1975 added 10 commits November 19, 2021 15:18

fix: "meson benchmark" should be "meson test --benchmark".

694aeb3

fix: building a subdir target didn't work on Windows.

ba4544d

Meson requires the separator between path and target name to be '/'. ALso, since the full target name can now end up as "path\to/targetName", don't show path\to in the tree view (it's implicit from the nodes anyway).

fix: exec() would invoke reject() and resolve() on error.

1484f2c

Don't do that. Also removed unused exec() function.

fix: stop parseJSONFileIfExists() from throwing and return false as (…

68b9f9d

…presumably) intended.

refactor: execAsTask() now uses vscode.ProcessExecution and has separ…

17fc40c

…ate command/args parameters. Will allow spaces in target names to work cross-platform (avoiding shell escaping issues). No uses require shell variable expansion, etc.

refactor: execStream() now has separate command/args parameters.

5dd5131

Should allow target names with spaces.

fix: test target names with spaces and the test all target.

429e1f1

I.e. Meson -> Run Unit Tests -> all, or specific test name with spaces. Uses ProcessExecution rather than ShellExecution for ninja/meson invocation to avoid shell space escaping issues. No uses require variable expansion, etc.

refactor: use meson for all commands, not ninja.

77e6abe

Samurai now works, via NINJA=samu.

feat: show build start/end in output.

20030f8

refactor: use execFile() rather than exec() to avoid shell escaping i…

84a3be0

…ssues.

lukester1975 force-pushed the fix-spaces-in-target-name branch from b48d540 to 84a3be0 Compare November 19, 2021 15:27

lukester1975 marked this pull request as ready for review November 19, 2021 15:35

dcbaker approved these changes Nov 20, 2021

View reviewed changes

dcbaker merged commit fbd4fb6 into mesonbuild:main Nov 20, 2021

lukester1975 deleted the fix-spaces-in-target-name branch November 20, 2021 11:06

paveloom mentioned this pull request Nov 13, 2023

Meson isn't picked up from the PATH #181

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes when there are spaces in test target name #47

Fixes when there are spaces in test target name #47

lukester1975 commented Nov 17, 2021

dcbaker left a comment

dcbaker Nov 18, 2021

lukester1975 Nov 18, 2021

lukester1975 commented Nov 18, 2021

lukester1975 commented Nov 18, 2021

lukester1975 commented Nov 19, 2021

dcbaker commented Nov 19, 2021

lukester1975 commented Nov 19, 2021

dcbaker commented Nov 20, 2021

	export function execStream(
	command: string \| string[],
	options: cp.SpawnOptions
	) {
	//FIXME: Force string array and fix callers
	if (typeof command === "string") {
	command = command.split(" ");
	}
	const spawned = cp.spawn(command[0], command.slice(1), options);

Fixes when there are spaces in test target name #47

Fixes when there are spaces in test target name #47

Conversation

lukester1975 commented Nov 17, 2021

dcbaker left a comment

Choose a reason for hiding this comment

dcbaker Nov 18, 2021

Choose a reason for hiding this comment

lukester1975 Nov 18, 2021

Choose a reason for hiding this comment

lukester1975 commented Nov 18, 2021

lukester1975 commented Nov 18, 2021

lukester1975 commented Nov 19, 2021

dcbaker commented Nov 19, 2021

lukester1975 commented Nov 19, 2021

dcbaker commented Nov 20, 2021