-
Notifications
You must be signed in to change notification settings - Fork 228
Fix path handling to work with Windows and add tests for it #1361
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
base: develop
Are you sure you want to change the base?
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #1361 +/- ##
========================================
Coverage 86.72% 86.72%
========================================
Files 337 337
Lines 84145 84174 +29
Branches 3140 3145 +5
========================================
+ Hits 72971 73000 +29
Misses 11174 11174 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
I have added a commit that updates the build tools to work in Windows as well. As always, the main issue is the You also need to tell Windows to use a shell for Since our package scripts expect to be run in bash, a Windows shell user will need to specify npm config set script-shell "C:\\Program Files\\git\\bin\\bash.exe" before running our build scripts. Finally, I changed the package scripts to use a literal ESC character, so that it works in Windows as well as unix. |
I should mention that this was discussed in the MathJax User's forum, where the original poster has confirmed (after a bit of back and forth) that the changes worked for him. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I ran into two issues building under Win10 with node 25 (latest version).
- When running
pnpm build
> @mathjax/[email protected] build C:\Users\vxs\git\MathJax-src
> pnpm -s build-mjs
'log' is not recognized as an internal or external command,
operable program or batch file.
ELIFECYCLE Command failed with exit code 1.
- Running the build commands without logging:
node components/bin/link-full
pnpm tsc --project tsconfig/mjs.json
pnpm tsc --project tsconfig/worker.json
pnpm copyfiles -u 1 'ts/a11y/sre/*.html' 'ts/a11y/sre/require.*' mjs
pnpm copyfiles -u 1 'ts/input/asciimath/legacy/**/*' mjs
pnpm copyfiles -u 1 ts/input/mathml/mml3/mml3.sef.json mjs
pnpm copyfiles -u 2 components/bin/package.json mjs
pnpm rimraf -g components/mjs/**/lib
pnpm rimraf bundle
node components/bin/makeAll --mjs --terse components/mjs
echo '{\n \"type\": \"commonjs\"\n}' > bundle/package.json
the penultimate command fails during webpacking with
[DEP0190] DeprecationWarning: Passing args to a child process with shell option true can lead to security vulnerabilities, as the arguments are not escaped, only concatenated.
Although this is only a warning it still seems to kill the bundling.
I Currently I assume that it does not like the options from
line 130 but I have not yet investigated further.
Did you set the npm config set script-shell "C:\\Program Files\\git\\bin\\bash.exe" or something similar (depending on where your copy of I have run it successfully in my Windows VM, and the original poster has run it after making the needed patches, so I'm pretty sure this works. |
This PR makes it possible to run MathJax via node in Windows (without needing to use a unix subsystem). The main issue is, as usual, the use of
\
rather than/
in Windows path names, so we introduce a new function in thecontext
object that converts Windows (absolute) paths to unix-style paths when needed, and use this whenever__dirname
orimport.meta.url
or similar values are used. That way, the management of paths can use/
as it currently does.In addition, the
context
setup forcontext.os
now handles the situation within node as well (it used to always returnundefined
). This is done using theprocess.platform
value to make to the genericUnix
,Windows
, andMacOS
values. I wasn't sure how to treatcygwin
, but have decided to set it toWindows
, since it should not hurt to do the path adjustment there, and my understanding is that cygwin can use Windows paths, so it might be useful to do the translation anyway.The
context
tests are adjusted for the new values, and I've added several new files to handle testing Windows and to get full coverage in the tests.This resolves issue mathjax/MathJax#3439.