Skip to content
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.

Sign up for GitHub

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

Fix powershell ArgumentList bug #11659

Merged
merged 3 commits into from
Jan 8, 2025
Merged

Fix powershell ArgumentList bug #11659

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7b89d10
Fix powershell ArgumentList bug
jkrumbiegel Dec 11, 2024
c3ca827
replace backslashes in user-specified project path
jkrumbiegel Jan 7, 2025
f44e552
add changelog
jkrumbiegel Jan 8, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions news/changelog-1.7.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,12 @@ All changes included in 1.7:
that allows to check whether a node is empty, i.e., whether it's an
empty list, has no child nodes, and contains no text.

## Engines

### `julia`

- ([#11659](https://github.com/quarto-dev/quarto-cli/pull/11659)): Fix escaping bug where paths containing spaces or backslashes break server startup on Windows.

## Other Fixes and Improvements

- ([#11643](https://github.com/quarto-dev/quarto-cli/issues/11643)): Improve highlighting of nested code block inside markdown code block, i.e. using ` ```{{python}} ` or ` ```python ` inside ` ````markdown` fenced code block.
24 changes: 14 additions & 10 deletions src/execute/julia.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ import {
} from "../config/format.ts";
import { resourcePath } from "../core/resources.ts";
import { quartoRuntimeDir } from "../core/appdirs.ts";
import { normalizePath } from "../core/path.ts";
import { normalizePath, pathWithForwardSlashes } from "../core/path.ts";
import { isInteractiveSession } from "../core/platform.ts";
import { runningInCI } from "../core/ci-info.ts";
import { sleep } from "../core/async.ts";
Expand Down Expand Up @@ -248,6 +248,12 @@ export const juliaEngine: ExecutionEngine = {
},
};

function powershell_argument_list_to_string(...args: string[]): string {
// formats as '"arg 1" "arg 2" "arg 3"'
const inner = args.map((arg) => `"${arg}"`).join(" ");
return `'${inner}'`;
}
Comment on lines +251 to +255
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem is that we are double quoting every args here but it is not necessary.

Especially argument like --startup-file=no shouldn't be quoted

Suggested change
function powershell_argument_list_to_string(...args: string[]): string {
// formats as '"arg 1" "arg 2" "arg 3"'
const inner = args.map((arg) => `"${arg}"`).join(" ");
return `'${inner}'`;
}
function powershell_argument_list_to_string(...args: string[]): string {
// formats as 'arg1 arg2 arg3'
// https://learn.microsoft.com/en-us/powershell/module/microsoft.powershell.management/start-process?view=powershell-5.1#example-7-specifying-arguments-to-the-process
const inner = args.join(" ");
return `'${inner}'`;
}

This only seems to fix the problem I see.

It will generate this argument

'--startup-file=no --project=C:/Users/chris/Documents/test-dir/ C:\\Users\\chris\\Documents\\DEV_R\\quarto-cli\\src\\resources\\julia\\quartonotebookrunner.jl C:\\Users\\chris\\AppData\\Local\\quarto\\julia\\julia_transport.txt'

Each args that needs quoting should be when calling the function, specifically each path that could have space I think.


async function startOrReuseJuliaServer(
options: JuliaExecuteOptions,
): Promise<{ reused: boolean }> {
Expand All @@ -265,6 +271,7 @@ async function startOrReuseJuliaServer(
await ensureQuartoNotebookRunnerEnvironment(options);
juliaProject = juliaRuntimeDir();
} else {
juliaProject = pathWithForwardSlashes(juliaProject);
trace(
options,
`Custom julia project set via QUARTO_JULIA_PROJECT="${juliaProject}". Checking if QuartoNotebookRunner can be loaded.`,
Expand Down Expand Up @@ -310,15 +317,12 @@ async function startOrReuseJuliaServer(
"Start-Process",
options.julia_cmd,
"-ArgumentList",
// string array argument list, each element but the last must have a "," element after
"--startup-file=no",
",",
`--project=${juliaProject}`,
",",
resourcePath("julia/quartonotebookrunner.jl"),
",",
transportFile,
// end of string array
powershell_argument_list_to_string(
"--startup-file=no",
`--project=${juliaProject}`,
resourcePath("julia/quartonotebookrunner.jl"),
transportFile,
),
Comment on lines +320 to +325
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Quotes should be set for each path that could have space in them

Suggested change
powershell_argument_list_to_string(
"--startup-file=no",
`--project=${juliaProject}`,
resourcePath("julia/quartonotebookrunner.jl"),
transportFile,
),
powershell_argument_list_to_string(
"--startup-file=no",
`--project="${juliaProject}"`,
`"${resourcePath("julia/quartonotebookrunner.jl")}"`,
`"${transportFile}"`,
),

"-WindowStyle",
"Hidden",
],
Expand Down
Loading