Skip to content

Fix #48: Missing npm on native Windows #3020

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Fix #48: Missing npm on native Windows #3020

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions waspc/ChangeLog.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,10 @@

## Unreleased

### 🐞 Bug fixes

- Fixes missing `npm` and `npx` on native Windows ([#3020](https://github.com/wasp-lang/wasp/pull/3020))

### ⚠️ Breaking Changes

- Wasp now requires Node.js version to be >=v22.12. [#2915](https://github.com/wasp-lang/wasp/pull/2915)
Expand Down
2 changes: 1 addition & 1 deletion waspc/packages/prisma/src/format.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ export async function formatSchema(
const process = spawn(
"npx",
["prisma", "format", "--schema", tmpPrismaSchemaFile.path],
{},
{shell: true},
);
process.stderr.on("data", (data) => {
stderr += data;
Expand Down
3 changes: 2 additions & 1 deletion waspc/src/Wasp/Job/Process.hs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,7 @@ runNodeCommandAsJobWithExtraEnv extraEnvVars fromDir command args jobType chan =
NodeVersion.VersionCheckFail errorMsg -> exitWithError (ExitFailure 1) (T.pack errorMsg)
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}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

runProcessAsJob nodeCommandProcess jobType chan
where
-- Haskell will use the first value for variable name it finds. Since env
Expand All @@ -119,3 +119,4 @@ runNodeCommandAsJobWithExtraEnv extraEnvVars fromDir command args jobType chan =
J._jobType = jobType
}
return exitCode
fullCommand = unwords $ command : args
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

4 changes: 2 additions & 2 deletions waspc/src/Wasp/Node/Version.hs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ where

import Data.Conduit.Process.Typed (ExitCode (..))
import System.IO.Error (catchIOError, isDoesNotExistError)
import System.Process (readProcessWithExitCode)
import System.Process (readCreateProcessWithExitCode, shell)
import Wasp.Node.Internal (parseVersionFromCommandOutput)
import qualified Wasp.SemanticVersion as SV
import qualified Wasp.SemanticVersion.VersionBound as SV
Expand Down Expand Up @@ -76,7 +76,7 @@ readCommandOutput :: String -> [String] -> IO (Either ErrorMessage String)
readCommandOutput commandName commandArgs = do
commandResult <-
catchIOError
(Right <$> readProcessWithExitCode commandName commandArgs "")
(Right <$> readCreateProcessWithExitCode (shell fullCommand) "")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

(return . Left . wrapCommandIOErrorMessage . makeIOErrorMessage)
return $ case commandResult of
Left procErr -> Left procErr
Expand Down
4 changes: 3 additions & 1 deletion waspc/src/Wasp/NodePackageFFI.hs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,4 +124,6 @@ packageCreateProcess ::
String ->
[String] ->
P.CreateProcess
packageCreateProcess packageDir cmd args = (P.proc cmd args) {P.cwd = Just $ fromAbsDir packageDir}
packageCreateProcess packageDir cmd args = (P.shell fullCommand) {P.cwd = Just $ fromAbsDir packageDir}
where
fullCommand = unwords $ cmd : args
Loading