Fix powershell ArgumentList bug #11659
Conversation
I have so I'll try to test this. Thanks ! |
|
Bump :) Would be nice to get this in as this bug completely precludes some Windows users from using the native julia engine. |
|
Great timing ! Yeah I was working on that today and just saw your message while I was gonna report. Something is not working on my windows with this PR. No more errors, but not transport file created. ❯ quarto render index.qmd --execute-debug
- Transport file C:\Users\chris\AppData\Local\quarto\julia\julia_transport.txt doesn't exist
Starting julia control server process. This might take a while...
- Custom julia project set via QUARTO_JULIA_PROJECT="C:\Users\chris\Documents\test-dir with space\". Checking if QuartoNotebookRunner can be loaded.
The latest version of Julia in the `1.10` channel is 1.10.7+0.x64.w64.mingw32. You currently have `1.10.5+0.x64.w64.mingw32` installed. Run:
juliaup update
in your terminal shell to install Julia 1.10.7+0.x64.w64.mingw32 and update the `1.10` channel to that version.
- QuartoNotebookRunner could be loaded successfully.
- Starting detached julia server through powershell, once transport file exists, server should be running.
- Transport file did not exist, yet.
- Transport file did not exist, yet.
- Transport file did not exist, yet.
- Transport file did not exist, yet.
- Transport file did not exist, yet.
- Transport file did not exist, yet.
- Transport file did not exist, yet.
- Transport file did not exist, yet.
- Transport file did not exist, yet.
- Transport file did not exist, yet.
- Transport file did not exist, yet.
- Transport file did not exist, yet.
- Transport file did not exist, yet.
- Transport file did not exist, yet.
- Transport file did not exist, yet.
- Transport file did not exist, yet.
- Transport file did not exist, yet.
- Transport file did not exist, yet.
- Transport file did not exist, yet.
- Transport file did not exist, yet.
ERROR: Error
at renderFiles (file:///C:/Users/chris/Documents/DEV_R/quarto-cli/src/command/render/render-files.ts:350:23)
at eventLoopTick (ext:core/01_core.js:214:9)
at async renderProject (file:///C:/Users/chris/Documents/DEV_R/quarto-cli/src/command/render/project.ts:464:23)
at async Command.actionHandler (file:///C:/Users/chris/Documents/DEV_R/quarto-cli/src/command/render/cmd.ts:248:26)
at async Command.execute (https://deno.land/x/cliffy@v1.0.0-rc.3/command/command.ts:1948:7)
at async Command.parseCommand (https://deno.land/x/cliffy@v1.0.0-rc.3/command/command.ts:1780:14)
at async quarto (file:///C:/Users/chris/Documents/DEV_R/quarto-cli/src/quarto.ts:190:5)
at async file:///C:/Users/chris/Documents/DEV_R/quarto-cli/src/quarto.ts:219:5
at async mainRunner (file:///C:/Users/chris/Documents/DEV_R/quarto-cli/src/core/main.ts:36:5)
at async file:///C:/Users/chris/Documents/DEV_R/quarto-cli/src/quarto.ts:209:3Unsetting So I think I'll merge main into this PR and see if I can fix this |
|
Thanks @cderv! In the state of this PR, debugging problems with server startup is annoying because you never see errors happening in the julia server process. One thing you could try is to checkout #11803, then merge the changes from this one into it (I added one arg, the logfile, to the julia invocation which you'd have to add here too). Then maybe it prints out something useful after the transport file lookup times out. If the error is before that, like some syntax error with PowerShell, then that wouldn't help, though. |
|
One nice thing of being on windows is that I have access to powershell and can try Powershell command directly. I believe there is still quoting problem So something is not yet right in how the command is created when |
About this, just so we are on the same page, by calling PowerShell command, it will be PowerShell 5.1 called, not version 7. So the official doc will be this one Probably not that many difference for the call we want to make. Quoting in Powershell is really painfull, and here we need to take extra care because
I am convinced this is the issue - just writing the correct Powershell Syntax for this |
There was a problem hiding this comment.
So I get it to work by doing these adjustment below.
However, I am trying to test a few scenarios locally. It seems behavior is not the same between
- Backslash for path separator
$env:QUARTO_JULIA_PROJECT="C:\Users\chris\Documents\test-dir\" - Forwardslash for path separator
$env:QUARTO_JULIA_PROJECT="C:/Users/chris/Documents/test-dir/"
And so adding space like $env:QUARTO_JULIA_PROJECT="C:/Users/chris/Documents/test-dir with space/" also impact.
Also I noticed something I don't know why exactly but it seems the space does not cause issue at
https://github.com/cderv/quarto-cli/blob/7b89d103ce5ca128ad74568304b9fe618858186d/src/execute/julia.ts#L279-L282
I also wanted to do --project="${juliaProject}" but then Package QuartoNotebookRunner not found in current path.
as it seems Deno or something else do a modification that lead Julia to this Pkg.project().path
'C:\\Users\\chris\\Documents\\test-dir\\"\\Project.toml
'
So it is quite fragile right now depending on how project path is.
Also is this expected that --no-execute-daemon or --execute-daemon-restart does not remove the transport file ?
Is there a way we could trigger a new server for our test while running quarto render ?
It would help test different scenarios.
| 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}'`; | ||
| } |
There was a problem hiding this comment.
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
| 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.
| powershell_argument_list_to_string( | ||
| "--startup-file=no", | ||
| `--project=${juliaProject}`, | ||
| resourcePath("julia/quartonotebookrunner.jl"), | ||
| transportFile, | ||
| ), |
There was a problem hiding this comment.
Quotes should be set for each path that could have space in them
| 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}"`, | |
| ), |
I set up a github actions script to download deno and start a tmate session, then ssh'd into that and tried some things out. I found that multiple things could work, and they all don't require that args without spaces are unquoted. So now I'm a bit confused, one of them also looked pretty much like what I have in the PR Here's the output from part of my One concatenated Start-Process> let command_args = 'Start-Process julia -ArgumentList """--project=./dir with spaces"" ""--threads=auto"" ""-E 1+1""" -Wait -NoNewWindow'
undefined
> let command = new Deno.Command("PowerShell", {args: ["-Command", command_args]})
undefined
> let result = command.outputSync()
undefined
> new TextDecoder().decode(result.stdout)
"2\n"All args separated out, quoting with
|
|
All the above works for me locally to. This is indeed confusing... I'll start fresh on this to understand better. |
|
What I noticed when trying things out first was that it was even harder to try from bash or whatever the default shell on the runner was. Because bash already removes one layer of quoting. So maybe what you thought you tried locally wasn't exactly corresponding to what the Deno code does. As this seems to be fine, maybe the bug is in the script that starts the server, as you didn't get a valid transport file out so it must have crashed at some point. |
I'll follow your advice on merging this in #11803 to see if I get more information then. |
|
I tried your I also tried with a qmd file with space and that also worked. |
So what I did to test your PR without change
---
title: "Julia test"
format: html
engine: julia
---
```{julia}
1 + 1
```
So we could say So I tried a simple fix for this doing this patch diff --git a/src/execute/julia.ts b/src/execute/julia.ts
index 280925851..0e3b6893e 100644
--- a/src/execute/julia.ts
+++ b/src/execute/julia.ts
@@ -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";
@@ -318,7 +318,7 @@ async function startOrReuseJuliaServer(
"-ArgumentList",
powershell_argument_list_to_string(
"--startup-file=no",
- `--project=${juliaProject}`,
+ `--project=${pathWithForwardSlashes(juliaProject)}`,
resourcePath("julia/quartonotebookrunner.jl"),
transportFile,
),and now both Maybe this diff --git a/src/execute/julia.ts b/src/execute/julia.ts
index 280925851..ff18b2e7f 100644
--- a/src/execute/julia.ts
+++ b/src/execute/julia.ts
@@ -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";
@@ -271,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.`,With this change I think the PR is ok (unless we can fix the
I probably got lost in this problem where Deno.Command() is doing something different from using shell directly. Thanks for getting me out of this ! 😄 |
|
Ah yes, backslashes, that's not good :) And I never tried with those because the runner terminal didn't autocomplete to them. Great that we found where the disparity was.
The daemon in the native julia engine refers to the worker process for a given file. The server process always exists after the first start and only automatically times out a couple minutes after no more workers exist. So that behavior is as intended (this is different from how jupyter kernels work). #11803 (comment) adds a |
|
Oh I see. We get a new concept with this server process that no current flag covers... 🤔 I let you do the change for insuring forward slash and I'll create manual test procedure for now that we can run if necessary. We'll add automated test when we have a way to kill the server. |
|
Ok I've added the backslash replacement, although I could not reproduce the transport file failure on the CI runner with a path with backslashes (Julia should in principle handle those ok on Windows). Well, it can't hurt to do this, and if it fixes things for you locally 🤷♂️ |
|
All works now ok with this PR while changing THanks |
|
Thanks, I've added a changelog entry in the same style as the 1.6 log had |
|
The errors seem unrelated, I had the same ones on a different PR earlier and they went away again without intervention |
Should fix #10111 with the technique reported to work in #10489 (comment) which seems to match one of the syntax options mentioned in the official docs https://learn.microsoft.com/en-us/powershell/module/microsoft.powershell.management/start-process?view=powershell-7.4#example-7-specifying-arguments-to-the-process
I don't have a Windows machine so I could not separately test this. For a test of the bug, one of the passed paths would need a space inside it which I can't control easily on CI. The only path amenable to that would be the project path which can be controlled with the ENV variable QUARTO_JULIA_PROJECT (but only if no server process is running yet, and the order of tests seems to be automatically decided by the CI script, so I'm not sure how to control that).