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.

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

Use cross-spawn instead of child_process and introduce _W32 placeholders #2015

Merged
merged 16 commits into from
Apr 5, 2020

Conversation

jlelong
Copy link
Collaborator

@jlelong jlelong commented Mar 31, 2020

Related to the tests and discussions in #2010.
It must be extensively tested before merging (paths with /, \\, spaces and also custom recipes using system commands).

Close #2010

Edit: Summary of the PR

  • All calls to child_process.spawn are replaced by cross-spawn.spawn, which takes care of escaping issues in a cross platform way.
  • We introduce new placeholders %DOC_W32%, %DOC_EXT_W32%, %DIR_W32%, %OUTDIR_W32%, which are normalized so that they use \\ as the path separator on Windows. Placeholders without the _W32 suffix always use / as the path separator. On Unix platforms, placeholders with and without the _W32 suffix have the same value.

@jlelong
Copy link
Collaborator Author

jlelong commented Apr 4, 2020

@tamuratak while I was adding more tests, I realised that the issue on /vs \ on Windows is deeper than I thought. See https://stackoverflow.com/a/10526678 for a series of (non-) working examples.

I finally figured out that the tool

      {
          "name": "copyPDF",
          "command": "copy",
          "args": ["%OUTDIR%/t.pdf", "%OUTDIR%/b.pdf"],
          "env": {}
      }

works fine as long as %OUTDIR% = %DIR% . Remember that the cwd of the spawned command is %DIR%. The issue arises when the dirname of the file to be copied uses / and does not match the cwd.

I think that instead of introducing _W32 placeholders, we should add a normalizing flag for each tool. Clearly, LaTeX commands must use / as separators regardless of the os, but for native commands normalising the args is simpler and even required in some cases. Now, the question is how to achieve this with breaking compatibility.

EDIT: Forget about that, this is far too intrusive and it may cause a lot of issue. I will instead test an other strategy:

  • Introduce _W32 placeholders that are always normalized
  • Expand all other placeholders to always use /
  • Do not further alter the arguments of spawn commands. The user is responsible for using the correct path separator with the correct placeholders. For instance, we would not correct %OUTDIR_W32%/out.

The replacement function is moved to utils/utils.js.
It is used whereever placeholders are used.
The new placeholders %DOC_W32%, %DOC_EXT_W32%, %DIR_W32% %OUTDIR_W32%
are normalized so that they use `\\` as the path separator on Windows.
Placeholders without the _W32 suffix always use `/` as the path
separator. On Unix platforms, placeholders with and without the  _W32
suffix have the same value.
@jlelong jlelong merged commit 352d622 into master Apr 5, 2020
@jlelong jlelong deleted the cross-spawn branch April 5, 2020 18:21
@jlelong jlelong changed the title Use cross-spawn to build and view Use cross-spawn instead of child_process and introduce _W32 placeholders Apr 5, 2020
@jlelong
Copy link
Collaborator Author

jlelong commented Apr 6, 2020

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

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 4, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The placeholder %OUTDIR% on Windows
1 participant