The placeholder %OUTDIR% on Windows

[tamuratak]

@jlelong I ask you a question on the placeholder, %OUTDIR%, on Windows. In the current master branch, backward slashes are used in %OUTDIR% on Windows since path.normalize replace forward slashes with backward slashes on Windows. Is this an intended behavior?

LaTeX-Workshop/src/components/manager.ts

Line 82 in bce11da

return path.normalize(out.split(path.sep).join('/'))

We can see the behavior of path.normalize on Windows by calling path.win32.normalize on other platforms.

path.win32.normalize('C:////temp\\\\/\\/\\/foo/bar');
// Returns: 'C:\\temp\\foo\\bar'

Related to #1897, #1904, and #1947.

[jlelong]

I guess there is something wrong here, the line should instead read

return path.normalize(out).split(path.sep).join('/')

We do want output paths to always use / even on Windows.

[tamuratak]

With the commit, 9ca0c06, %OUTDIR% cannot be used with cmd.exe in recipes on Windows as far as I tried. For example, the following recipe does not work. I do not have a specific opinion that it should work.

{
    "latex-workshop.latex.recipes": [
        {
          "name": "latexmk_copy",
          "tools": ["latexmk", "copyPDF"]
        }
    ],
    "latex-workshop.latex.tools": [
        {
          "name": "latexmk",
          "command": "latexmk",
          "args": [
            "-synctex=1",
            "-interaction=nonstopmode",
            "-file-line-error",
            "-pdf",
            "-outdir=%OUTDIR%",
            "%DOC%"
          ],
          "env": {}
        },
        {
          "name": "copyPDF",
          "command": "cmd.exe",
          "args": ["/C", "copy", "\"%OUTDIR%/%DOCFILE%.pdf\"",  "\"%OUTDIR%/b.pdf\""],
          "env": {}
        }
    ]
}

[tamuratak]

We can try the recipe on GitHub Actions. See

• master...tamuratak:d0569769c1a771a11e84f933eef39d00ba8b3605
• https://github.com/James-Yu/LaTeX-Workshop/tree/d0569769c1a771a11e84f933eef39d00ba8b3605

[jlelong]

I missed that point. This is indeed an issue. Short summary

• some LaTeX compilers such as lualatex require the use of /. This can be achieved by setting $MSWin_back_slash=0 in latexmk. Since latexmk-4.69 (only 4.67 is on CTAN), there is even a command line option for that -MSWinBackSlash-.
• When %OUTDIR% dir is ./, we cannot use back slash as .\ causes errors.
• The use of \ breaks cmd.exe

It seems to me that %DOC%always uses `/``

LaTeX-Workshop/src/components/builder.ts

Lines 514 to 529 in 933f095

	replaceArgumentPlaceholders(rootFile: string, tmpDir: string): (arg: string) => string {
	return (arg: string) => {
	const docker = vscode.workspace.getConfiguration('latex-workshop').get('docker.enabled')
	const doc = rootFile.replace(/\.tex$/, '').split(path.sep).join('/')
	const docfile = path.basename(rootFile, '.tex').split(path.sep).join('/')
	const outDir = this.extension.manager.getOutDir(rootFile)
	return arg.replace(/%DOC%/g, docker ? docfile : doc)
	.replace(/%DOCFILE%/g, docfile)
	.replace(/%DIR%/g, path.dirname(rootFile).split(path.sep).join('/'))
	.replace(/%TMPDIR%/g, tmpDir)
	.replace(/%OUTDIR%/g, outDir)
	}
	}
	}

So I would be tempted to do the same for %OUTDIR%.

[tamuratak]

As far as I tried, on the command prompt of Windows, the following works well with forward slashes (double quotes are necessary):

cmd.exe /C copy "C:/path/a.pdf" "C:/path/b.pdf"

I do not know why the recipe does not work.

[tamuratak]

An easy solution is providing placeholders with backward slashes, %OUTDIR_W32%, %DOC_W32%, ... and placeholders with forward slashes, %OUTDIR%, %DOC%, ... at the same time on Windows.

[jlelong]

That's for sure. It might be related to nodejs/node-v0.x-archive#25895 (comment)

[jlelong]

I think the explanation is nodejs/node#5060 (comment)
We need to pass {windowsVerbatimArguments: true} to prevent node from escaping all quotes.
See the corresponding test https://github.com/jlelong/LaTeX-Workshop/runs/545593932?check_suite_focus=true

All this is related to the way arguments to cmd.exe should be quoted. Using cross-spawn, we can make everything work by simply dropping cmd.exe and using copy as the command. If the cmd does not exist, cross-spawn automatically wraps up the command and its argument and pass the whole to cmd.exe with the appropriate escaping. See https://github.com/jlelong/LaTeX-Workshop/runs/545688242?check_suite_focus=true

https://github.com/jlelong/LaTeX-Workshop/blob/c8a4d3ff79bc4e5ffb2db3630db58f10991742ad/test/fixtures/build/fixture100/.vscode/settings.json#L22-L27

https://github.com/jlelong/LaTeX-Workshop/blob/49f68155c4b658d9af2ac4ddb2451d9398529e88/src/components/builder.ts#L270

where cs is cross-spawn.

As a conclusion of all this, I would be tempted to use cross-spawn everywhere. We already use cross-spawn to run latexindent following #1873 because escaping issues with spaces.

[jlelong]

@tamuratak What do you think of this?

[tamuratak]

My main concern is that there might be a case cmd.exe does not work well with forward slashes. See https://stackoverflow.com/questions/10523708/why-does-the-cmd-exe-shell-on-windows-fail-with-paths-using-a-forward-slash. However, we can add %OUTDIR_W32%, %DOC_W32%, ..., later if users request them.

So, if cross-spawn works well with typical recipes other than default ones, I think it is definitely ok. We might need some more tests.

[tamuratak]

One example I am considering is a recipe using a quote,q/.../, with latexmk. See https://texwiki.texjp.org/?Visual%20Studio%20Code%2FLaTeX#c465cb18. The usage of / might be problematic.

[jlelong]

OK to later add _W32 placeholders with backward slashes.

cross-spawn is just a wrapper around child_process so it should not hurt. I will make a new branch to test it intensively. Btw, many thanks for your test framework. It is awesome!

[tamuratak]

I have a question. Do users who are already using recipes with cmd.exe have to rewrite their recipes by dropping cmd.exe? Is there any other case that a user has to rewrite the recipe?

[jlelong]

The use of q/.../ might be an issue. I will have a look at it using cross-spawn.

Do users who are already using recipes with cmd.exe have to rewrite their recipes by dropping cmd.exe? Is there any other case that a user has to rewrite the recipe?

No I do not think so. My understanding is that everything that already works with child_process will still work with cross-spawn. It is designed as a drop-in replacement. It only avoids resorting to tricky options or escaping.

[jlelong]

It seems that everything works well with cross-spawn and q/.../.
I considered

{
    "latex-workshop.latex.recipes": [
        {
          "name": "latexmk_copy",
          "tools": ["latexmk", "copyPDF"]
        }
    ],
    "latex-workshop.latex.tools": [
        {
          "name": "latexmk",
          "command": "latexmk",
          "args": [
            "-e",
            "$latex=q/pdflatex %O -synctex=1 -interaction=nonstopmode -file-line-error %S/",
            "-e",
            "$pdflatex=q/pdflatex %O -synctex=1 -interaction=nonstopmode -file-line-error %S/",
            "-outdir=%OUTDIR%",
            "-pdf",
            "%DOC%"
          ],
          "env": {}
        },
        {
          "name": "copyPDF",
          "command": "copy",
          "args": ["%OUTDIR%/t.pdf", "%OUTDIR%/b.pdf"],
          "env": {}
        }
    ]
}

https://github.com/jlelong/LaTeX-Workshop/actions/runs/67671244 passed

If you have a look at master...jlelong:test-cmd-exe, you will see that the only meaningful changes to make all this work are

• the direct use of copy as a real command instead of going through `cmd.exe``
• to spawn the recipes using cross-spawn instead of child_process.

[jlelong]

See https://github.com/James-Yu/LaTeX-Workshop/wiki/Compile#placeholders for the definitions of the new placeholders.

[tamuratak]

Should we use cross-spawn also in viewer.ts? If there is not a specific reason, we had better leave it unchanged since we do not test external SyncTeX and those recipes are tricky.

LaTeX-Workshop/src/components/viewer.ts

Line 5 in a32dfdf

import * as cs from 'cross-spawn'

[Edit] I have found that viewer.ts has nothing to do with external SyncTeX. So it will not cause problems.

[jlelong]

Correct, this is only used to launch the external viewer. I do not know if this feature is much used but cross-spawn should help in some cases.

Indeed, we still use child_process for external synctex

LaTeX-Workshop/src/components/locator.ts

Lines 427 to 446 in 0d64561

	syncTeXExternal(line: number, pdfFile: string, rootFile: string) {
	if (!vscode.window.activeTextEditor) {
	return
	}
	const texFile = vscode.window.activeTextEditor.document.uri.fsPath
	const configuration = vscode.workspace.getConfiguration('latex-workshop')
	const command = configuration.get('view.pdf.external.synctex.command') as string
	let args = configuration.get('view.pdf.external.synctex.args') as string[]
	if (args) {
	args = args.map(arg => {
	return replaceArgumentPlaceholders(rootFile, this.extension.builder.tmpDir)(arg)
	.replace(/%PDF%/g, pdfFile)
	.replace(/%LINE%/g, line.toString())
	.replace(/%TEX%/g, texFile)
	})
	}
	this.extension.manager.setEnvVar()
	cp.spawn(command, args)
	this.extension.logger.addLogMessage(`Open external viewer for syncTeX from ${pdfFile}`)
	}