Fix #48: Missing npm on native Windows #3020
Conversation
nodtem66
changed the title
|
Hey @nodtem66, thanks for this contribution! Since the PR is still a draft, please let us know on the #wasp-dev channel on Discord once it's ready for a full review. To help us review this effectively, could you please update the PR description to outline the scope of your changes? It's very helpful for us to know:
After that, please run the test suite with |
|
Hi @cprecioso, I've added the description that you suggested. The test suit did not pass and produced a long output, so I put it here for reviewing. Summary
I guess it is the problem of Windows EOF, right? Maybe we need to rewrite the test for handling Update: I have fixed this issue in #3027 |
- change `System.Process proc` to `System.Process. shell` for running npm.cmd and npm.ps1 commands - add detail to changeLog.md
|
Hey @nodtem66, if you don't mind we need to wait this PR as we'd like @Martinsos to review it when he comes back from holidays. So it might be a couple of weeks or so until this gets reviewed. |
nodtem66
changed the title
|
This PR is going to patch many POSIX logics that are not compatible to Windows. I would like to PR everything in one go to save time. Currently, I'm working on the prerequisite PR #3027. When it's done, I'll update this PR. |
There was a problem hiding this comment.
Awesome @nodtem66 , thanks a lot for figuring this out and creating this PR!
So this makes sense to me, and it is great to hear a solution so simple could be the best solution.
As you said, that older PR used a different approach, where it specified process differently when platform was Win. That also works, but this seems simpler + more robust (using shell), as we are letting the system (shell) figure out how to resolve where npm or npx or whatever is, instead of us hardcoding that knowledge in Wasp.
The only thing I am worried about is potential issues that using shell vs proc could cause. As you said, potentially more can be executed than via proc. Wasp is a piece of software that user executes on their own on their own machine and these calls are very controlled, so it is probably fine, but still, I woudl also like to make sure we are not making any kind of regression here. I mostly asked about escaping the arguments, you will see in the comments.
| NodeVersion.VersionCheckSuccess -> do | ||
| envVars <- getAllEnvVars | ||
| let nodeCommandProcess = (P.proc command args) {P.env = Just envVars, P.cwd = Just $ SP.fromAbsDir fromDir} | ||
| let nodeCommandProcess = (P.shell fullCommand) {P.env = Just envVars, P.cwd = Just $ SP.fromAbsDir fromDir} |
There was a problem hiding this comment.
I would add some comment here, explaining why it is important that we use shell and not proc, so future readers don't wonder why shell was used.
| J._jobType = jobType | ||
| } | ||
| return exitCode | ||
| fullCommand = unwords $ command : args |
There was a problem hiding this comment.
There is one thing I am not sure about: are we missing out on any args escaping here? So before we were giving command and args to P.proc, and I guess it was potentially doing some manipulation with those args. Now it is just one big string (command + args) -> could that be an issue? Should we be wrapping args into ' or something? What could happen if we don't? I am always a bit confused with escaping of args, so we should understand this properly.
There was a problem hiding this comment.
Hi @Martinsos, Thanks for reviewing. I've checked the source code of both P.proc and P.shell. Indeed, P.proc is the modern and safe version of P.shell (see https://github.com/haskell/process/blame/98101f82543b3a28e9f5192d758a508881f8b464/System/Process/Windows.hsc#L423).
I think that we are going to reinvent the wheel by trying to escape args for P.shell. After reviewing these things, I've changed my mind to go for P.proc. (cont. below)
| commandResult <- | ||
| catchIOError | ||
| (Right <$> readProcessWithExitCode commandName commandArgs "") | ||
| (Right <$> readCreateProcessWithExitCode (shell fullCommand) "") |
There was a problem hiding this comment.
Hm same question here as above, do we need to somehow do something with arguments? fullCommand was used only for printing so that was fine, but now we are passing it to shell.
|
Btw GPT is proposing to use It also advised not to switch to Yeah this is kind of where I ended up chatting with GPT on this, and it makes sense to me:
So on Linux/Mac we do it as we were doing so far, with |
|
@Martinsos
All arguments in these solution will be escaped automatically by The reason why Test programimport System.IO
import System.Process
import Control.Exception
-- A minimal program to play with System.Process.proc
main :: IO ()
main = do
putStrLn "--- Test Case 1: Simple command (ls) ---"
testSimpleCommand
putStrLn "\n--- Test Case 2: NPM command ---"
testNPMCommandViaProc
putStrLn "\n--- Test Case 3: NPM command (shell) ---"
testNPMCommandViaShell
putStrLn "\n--- Test Case 4: NPM command (cmd suggested by GPT) ---"
testNPMCommandViaProcAndCmdSuggestedByGPT
putStrLn "\n--- Test Case 5: NPM command (cmd with separate argument) ---"
testNPMCommandViaProcAndCmdWithSeparatedArgument
putStrLn "\n--- Test Case 6: NPM command (cmd) ---"
testNPMCommandViaProcAndCmd
putStrLn "\n--- Test Case 7: NPM command (cmd with escaped argument) ---"
testNPMCommandViaProcAndCmdWithEscapedArgument
putStrLn "\n--- Test Case 8: NPM command (PowerShell) ---"
testNPMCommandViaProcAndPowershell
-- Running a simple command that succeeds.
testSimpleCommand :: IO ()
testSimpleCommand = do
let p = proc "ls" ["-l"]
tryCreateProcess p
-- Failing command there is no npm.exe in the path
testNPMCommandViaProc :: IO ()
testNPMCommandViaProc = do
let p = proc "npm" ["--version"]
tryCreateProcess p
-- Unsafe way to run a command
testNPMCommandViaShell :: IO ()
testNPMCommandViaShell = do
let p = shell "npm --version"
tryCreateProcess p
-- Failed
testNPMCommandViaProcAndCmdSuggestedByGPT :: IO ()
testNPMCommandViaProcAndCmdSuggestedByGPT = do
-- Run the command using cmd.exe
-- /c carries out the command specified by string and then terminates
-- /d disables the execution of AutoRun commands
-- /s strip the first and last quotes (") around each arguments and leave the rest of the command unchanged
-- ref: https://learn.microsoft.com/en-us/windows-server/administration/windows-commands/cmd
let p = proc "cmd" ["/d", "/s", "/c", "npm --version"]
tryCreateProcess p
-- Failed
testNPMCommandViaProcAndCmdWithSeparatedArgument :: IO ()
testNPMCommandViaProcAndCmdWithSeparatedArgument = do
let p = proc "cmd" ["/d", "/s", "/c", "npm", "--version"]
tryCreateProcess p
-- Solution 1: combine everything
testNPMCommandViaProcAndCmd :: IO ()
testNPMCommandViaProcAndCmd = do
let p = proc "cmd" ["/d /s /c npm --version"]
tryCreateProcess p
-- Solution 2: only /c command and arguments
testNPMCommandViaProcAndCmdWithEscapedArgument :: IO ()
testNPMCommandViaProcAndCmdWithEscapedArgument = do
let p = proc "cmd" ["/d", "/s", "/c npm --version"]
tryCreateProcess p
-- Solution 3: use Powershell
testNPMCommandViaProcAndPowershell :: IO ()
testNPMCommandViaProcAndPowershell = do
let p = proc "pwsh" ["-CommandWithArgs", "npm --version"]
tryCreateProcess p
tryCreateProcess :: CreateProcess -> IO ()
tryCreateProcess p = do
result <- try (createProcess p) :: IO (Either SomeException (Maybe Handle, Maybe Handle, Maybe Handle, ProcessHandle))
case result of
Left err -> putStrLn $ "Error: " ++ show err
Right (_, _, _, ph) -> do
exitCode <- waitForProcess ph
putStrLn $ "Exit Code: " ++ show exitCodeOutput--- Test Case 1: Simple command (ls) ---
total 17
...
...
Exit Code: ExitSuccess
--- Test Case 2: NPM command ---
Error: npm: createProcess: does not exist (No such file or directory)
--- Test Case 3: NPM command (shell) ---
11.4.2
Exit Code: ExitSuccess
--- Test Case 4: NPM command (cmd suggested by GPT) ---
'"npm --version' is not recognized as an internal or external command,
operable program or batch file.
Exit Code: ExitFailure 1
--- Test Case 5: NPM command (cmd with separate argument) ---
node:internal/modules/cjs/loader:1228
throw err;
^
Error: Cannot find module '<path>\node_modules\npm\bin\npm-prefix.js'
at Function._resolveFilename (node:internal/modules/cjs/loader:1225:15)
at Function._load (node:internal/modules/cjs/loader:1055:27)
at TracingChannel.traceSync (node:diagnostics_channel:322:14)
at wrapModuleLoad (node:internal/modules/cjs/loader:220:24)
at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:170:5)
at node:internal/main/run_main_module:36:49 {
code: 'MODULE_NOT_FOUND',
requireStack: []
}
Node.js v22.14.0
Exit Code: ExitFailure 1
--- Test Case 6: NPM command (cmd) ---
11.4.2
Exit Code: ExitSuccess
--- Test Case 7: NPM command (cmd with escaped argument) ---
11.4.2
Exit Code: ExitSuccess
--- Test Case 8: NPM command (PowerShell) ---
11.4.2
Exit Code: ExitSuccess |
|
Some very detailed work @nodtem66 ! Thanks a lot for putting time into this -> I will have to take a bit of time to properly check what you wrote, and the tricky thing is I just started a bit longer (2 weeks) vacation, so forgive me if it will take me a bit to get fully back to you. I will try to get back to you during it, but if not, you can count on me to get on it after the vacation for sure. |
|
@nodtem66 just to let you know I am working on this, I dedicated some time and my understanding is now at a much better level I think, but I still need a bit more time to check all you did here and to make some final recommendations -> coming soon. |
Motivation
Currently. Wasp doesn't run natively on Windows OS, which is a pain point for many developers (#2484, #2890). While the program can be used through WSL, the experience can be unpredictable and has known performance issues (#2717). This PR aims to address a specific issue that prevents Wasp from running alias commands on Windows (like npm.ps1, npm.cmd, npx.ps1, npx.cmd), bringing us one step closer to native Windows support.
Description
wasp startfailing on windows #48 by changingSystem.Process proctoSystem.Process shell.System.Processto correctly identify and execute alias commands on Windows, particularlynpmandnpxwhich is often a PowerShell script (npm.ps1/npx.ps1) rather than a simple executable.Problem
While running Wasp on Windows, the
wasp startcommand fails because it cannot findnpmdespitenpmandnodebeing present in the user's path.This is because Haskell's
System.Process procstruggles to execute PowerShell script (npm.ps1) or Cmd script (npm.cmd), depending on current shell.Previous PR
The previous PR (#658) solved this problem by separate the logic for executing
npmon Windows. It works well, but I have no idea why it has never been merged into the main code.Solution
The solution is to switch from
System.Process proctoSystem.Process shellref. This change allows the Haskell process to utilize the system's shell (no matter which shell is), which is better way to handle the alias commands on Windows' PowerShell and Cmd.Security Considerations
However, the downside of this solution is
System.Process shellmight be use for executing malicious codes if user-supplied input is passed to the shell. we must ensure that theshellfunction is only used with a limited and predefined set of commands (like node, npm, npx), and any user provided arguments are properly filtered.Steps to compile in Windows 11
waspcwasp-cliTest suits
run testSee commend below.
Unrelated issues on native Windows
Select what type of change this PR introduces:
[x] Bug fix (non-breaking change which fixes an issue).
Update Waspc ChangeLog and version if needed
[x] I updated
ChangeLog.mdwith description of the change this PR introduces.