-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Add support for Windows NPM #658
base: main
Are you sure you want to change the base?
Conversation
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.
@MetaMmodern Super awesome that we are making progress in the Windows direction!!
You did a great job taking into account that you don't know Haskell :). I left some comments, mostly it comes down to figuring out what exactly we want to do. My previous bad naming makes things a bit more complex but I think we can fix that now also.
waspc/src/Wasp/Generator/Common.hs
Outdated
) | ||
where | ||
|
||
import qualified Wasp.SemanticVersion as SV | ||
import System.Info (os) | ||
import Data.List |
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.
We try to import stuff qualified or named, so we don't pollute the namespace.
Therefore, best to do import Data.List (intercalate)
!
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.
Done
waspc/src/Wasp/Generator/Common.hs
Outdated
@@ -38,3 +42,12 @@ npmVersionRange = | |||
|
|||
prismaVersion :: SV.Version | |||
prismaVersion = SV.Version 3 15 2 | |||
|
|||
oSSpecificNpm :: [Char] |
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.
Nice job, you put this in the best place in the codebase I think :), finding your way around well I see!
Maybe we can just call this function npmCmd
? The fact that it is OS specific we can leave as an implementation detail of the function itself.
A bit of Haskell: instead of [Char]
, normally we use String
-> it is just type alias for [Char]
really, but it is more common.
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.
Maybe we could also provide a short comment explaining why is it important to add this .cmd on windows, since it took us some time to figure that out, and if somebody later stumbles onto this, they might wonder why we are adding this .cmd
on windows.
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.
@Martinsos for Strings--got it, did not think haskell has those) For exmplanation comment--no problem. Should I leave a text explanation or leave a link to SO?
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.
Maybe best to do both: text explanation + SO link! Your explanations here in description are already very good, smth very similar should do just fine.
waspc/src/Wasp/Generator/Common.hs
Outdated
oSSpecificNpm :: [Char] | ||
oSSpecificNpm = "npm" ++ if os /= "mingw32" then "" else ".cmd" | ||
|
||
compileOsSpecificNodeCommand :: [Char] -> [[Char]] -> ([Char], [[Char]]) |
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.
We could maybe also call this one just compileNodeCmdWithArgs
, and leave the fact about OS as internal thing.
s/[Char]/String
Here we could also benefit from a comment explaining the reasoning behind the stuff we have to do for Windows. In this case it would also explain it to me since right now I don't understand why we have to do this :D! I thought node
was working fine for us?
EDIT: reading code below, I realized this command here and args are not node
+ some args, it is instead ANY command that we somehow know requires node
to be installed on the machine + its args. I suggested we rename on of the old functions below, check it out -> I gave it poor name before and I confused me now, possibly you also. So if we rename that one, we should also rename this one to compileCmdWithArgsRequiringNode
.
But most important will be documenting what is the intention of this function, then further direction will be clearer.
@@ -102,7 +102,8 @@ runNodeCommandAsJob fromDir command args jobType chan = do | |||
Right nodeVersion -> | |||
if SV.isVersionInRange nodeVersion C.nodeVersionRange | |||
then do | |||
let process = (P.proc command args) {P.cwd = Just $ SP.fromAbsDir fromDir} | |||
let (specificCommand, specificArgs) = C.compileOsSpecificNodeCommand command args |
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.
Looking at runNodeCommandAsJob
, I am thinking that it is not clear what it actually does. This falls on me, I should have documented it better! But reading it now, I think the point is that it runs any command
with args
, while first checking if node
is available on the machine and is using correct version?
So, it should probably be renamed to runCommandThatRequiresNodeAsJob
! Maybe we can do this in this PR since we are already here?
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.
As for using compileOsSpecificNodeCommand
here -> We are basically applying it to any command really, not node
command, just any command that for some reason requires node
to be present on the machine. Is that ok, is that what we wanted? I guess I don't know because I am not sure what that cmd.exe
does and why we are doing it.
If so, should we use it for all commands we are trying to run, and not just here?
What about us calling node directly, I see that below we are calling node
, in line 129 -> what about that, is that ok to be left like that?
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.
Ok, so now I read what you wrote about cmd.exe
and read the SO answer you linked to!
So interesting, it seems that npm.cmd
script is written in such way that calling it programmatically can make it work wrong, so by adding cmd.exe /c
in front we ensure it works correctly.
The question is then: Do we want to use this trick only when calling npm
, or do we want to use it when calling any commands on Windows? I am thinking maybe it is enough to use it just for calling npm
, at least for now?
In that case, let's use it like that, only for npm
. So what we could do is rename this function to
buildNpmCmdWithArgs :: [String] -> (String, [String])
and that is it? And then we call it only in places where we need to call npm
?
Ah I realize now you explained in the description why we are doing the |
Alright, read about cmd.exe and left the comment, that is it from me for this first iteration of review! |
Oh btw we use ormolu for formatting Haskell code -> is that something you could easily set up, so that we can get the CI working? If not, don't worry, I can take care of that for you later. |
Um, I'll try but not now, later evening) |
In the meantime I made commits that should fix the two problems that you identified @MetaMmodern (wrong path and problem with \xad) -> I pushed them onto your |
@Martinsos for db migration similar error: So For the rest review:
(cabal dependencies installation is extremely slow, even npm is not so slow. shame on cabal) |
Awesome @MetaMmodern ! I left couple more comments, some are just comments/naming, one is about where to call the "buildNpmCmdWithArgs" function. As for encoding issue with prisma db migrate -> that happens now due to us not supporting the encoding that you use in your terminal, which is CP866 (russian cyrilic). Ha yes it takes long time for cabal since there is a lot to compile really, Haskell is more complex language than JS, but usually it is so slow only on the first build! |
waspc/src/Wasp/Generator/Common.hs
Outdated
|
||
-- | Changes an npm command to a cmd.exe command on Windows only. Calling npm from API causes troubles. | ||
-- The reason and solution exaplined here: https://stackoverflow.com/a/44820337 | ||
buildNpmCmdWithArgs :: String -> [String] -> (String, [String]) |
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.
We said this is to be used only for calling npm
. But in that case, we don't need command
as input, right? Since we know it is npm
?
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.
fixed
waspc/src/Wasp/Generator/Common.hs
Outdated
oSSpecificNpm :: String | ||
oSSpecificNpm = "npm" ++ if os /= "mingw32" then "" else ".cmd" | ||
|
||
-- | Changes an npm command to a cmd.exe command on Windows only. Calling npm from API causes troubles. |
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.
Similar like above, this is addressing implementation details. I would make sure this comment talks about what function does from the outside, now how it does it, and details can go inside of function. We might not even need this comment if it is clean what it does from the function name + signature.
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.
moved comments into implementation
errorOrNodeVersion <- getNodeVersion | ||
case errorOrNodeVersion of | ||
Left errorMsg -> exitWithError (ExitFailure 1) (T.pack errorMsg) | ||
Right nodeVersion -> | ||
if SV.isVersionInRange nodeVersion C.nodeVersionRange | ||
then do | ||
let process = (P.proc command args) {P.cwd = Just $ SP.fromAbsDir fromDir} | ||
let (specificCommand, specificArgs) = C.buildNpmCmdWithArgs command args |
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.
Now that we renamed the function to buildNpmCmdWithArgs
, and that it is used only for running npm
+ some args, I don't think we want to call it here anymore, since here we are dealing with any command that requires node installed, which can also be other stuff that is not npm, for example npx
.
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.
Done)
applied suggested change Co-authored-by: Martin Šošić <[email protected]>
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.
@MetaMmodern this should be it! In order to speed the merging up a bit, I fixed one tiny compiler error that somehow sneaked in, did a tiny bit of left-over refactoring, and I added more encodings, that should resolve the issue you had with CP861!
Would you like to give it a finally try, to see if the issue with CP861 is now resolved?
in an hour or so) lil bit later |
@MetaMmodern I think I found the reason -> there was this flag for this new TODO for myself: maybe try with the |
@Martinsos maybe there is a way to apply caching to library builds? Like separate those into some kind of .bin files and use cache if dependencies did not change) |
Yup there is caching already in place! It just takes long this first time. |
It builds now @MetaMmodern |
ok. checking locally |
@Martinsos nnnnnope) |
Wooo great! This was unfortunately expected, but we took care of encoding
yay! This is furthest we got right?
…On Mon, 4 Jul 2022, 18:06 Bunyamin Shabanov, ***@***.***> wrote:
@Martinsos <https://github.com/Martinsos> nnnnnope)
[image: image]
<https://user-images.githubusercontent.com/42899786/177189747-06a070ad-33b4-41e1-90c7-192101fb550b.png>
—
Reply to this email directly, view it on GitHub
<#658 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AALXFB32RHA74DFD325MC33VSMDWXANCNFSM52GXRSUQ>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
Well, as I understood--yes. |
I managed to set up my Windows to run Haskell, so now I can also test! Ok, so I did following, as a quick hack in the place where we now use
I will try to properly add this to the code. Regarding your encoding @MetaMmodern -> is there a way I can also set my encoding to be like yours, so I can test? I tried setting language to Croatian, and to Russian, but it uses CP437 in that case and I don't have any issues with encodings, even if I just do decodeUTF8 -> ok, escape characters are often not correctly shown, but it doesn't break. So how can I reproduce it to have the same situation as you? Because I am not happy with the current encoding solution I made, it works but it has its problems. |
@Martinsos hey, congrats on setting up Windows. For encoding: try running |
Thanks, I tried that @MetaMmodern ! But I can't reproduce the failure to parse, the one with I did a final commit, which:
TLDR; Can you give it one last spin? I did as much as I could on my Windows, for me it works now, but I wonder if it will work for you with your CP861 encoding (worked for me when I used chcp 861 but it might be different). TODO for me: I have to decide what to do with encoding code. Probably kick out |
@Martinsos What does this mean? |
Oho awesome you got this far!!! So it all works!
Yes it seems you need postgre database and provide its connection URL via
this variable. Which app are you running? In docs you have detailed
instructions on how to run postgre dB, includin one liner to get it going
quickly via docker.
…On Tue, 5 Jul 2022, 14:27 Bunyamin Shabanov, ***@***.***> wrote:
@Martinsos <https://github.com/Martinsos> What does this mean?
[image: image]
<https://user-images.githubusercontent.com/42899786/177327181-05b5ff18-a0b1-429f-96e1-8e4cd699b059.png>
Do I need to have Postgres Installed to run WASP? Do I need to manually
add this env variable or is it set by WASP CLI?
—
Reply to this email directly, view it on GitHub
<#658 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AALXFB22MAPAXESN5ENKIPLVSQS3HANCNFSM52GXRSUQ>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
@Martinsos okay, I'll try it now. |
@Martinsos In Ukraine we call this "PEREMOGA", which means "a win"💪🏼 Important note for you to put into documentation: If you run docker on Windows using WSL you have to specify WSL host, not localhost as your host for DB connection. So that's it) |
Awesome @MetaMmodern well peremoga it is then :D, I can't believe we have Windows support working!! Thanks again, without you pushing this, we would not have it working soon for sure. |
@MetaMmodern , when you catch a moment (no hurry), could you run one last test? I now made a change in the code we use for decoding, so it is less convoluted + doesn't need external library -> in theory it should be the same, but if you can give it a try (wasp db migrate-dev) I would feel sure it works. Unfortunately I still haven't managed to replicate your situation with encoding in order to test the situation you had. Btw is your output messy in any way? For me, on Windows, it is often not colored when it should be, plus some chars get replaced with |
@Martinsos anything for you my man))) Yes, I have artifacts on output. Yes, I have question marks (?) at the beginning of the lines, just check my previous screenshots. But colouring works. |
@Martinsos |
Mmmmm ok, those should be emojis but I guess that is not supported on Windows -> we can work on that in the future as a separate issue! |
@Martinsos are you sure? I just tested emoji output, works fine. Anyway I agree that should be a separate issue. Let's finish with this PR) |
Description
These changes add support for Windows OS. Was tested on Windows 10 (x64), server and web-client start now.
Code explanation
The reason why npm was not starting is because Windows Process API appends .exe automatically to the executable filename, however npm as program does not provide any .exe file, only .cmd and .ps1. (explained here: https://stackoverflow.com/questions/43139364/createprocess-weird-behavior-with-files-without-extension) The first thing to do is to support .cmd for windows only. That's implemented in

oSSpecificNpm
variable. (Screenshot of ghci session for proof)When we run

npm.cmd
from inside haskell it will complain that it can't find thenpm-cli
. The reason and solution is explained here: https://stackoverflow.com/a/44820337 . The implementation is made incompileOsSpecificNodeCommand
Common function and insrc/Wasp/Generator/Job/Process.hs
file. (Screenshot as demo)Things to do further
Do NOT update docs and do not mention Windows support, since there are still following issues persistent(at least in my env):
Some path's in generated js files are incorrect for windows (backslashes do not work).

Some issues with db and/or with encoding during db initialization/migration.

UI did not render for me😢.

Fixes #48
Type of change
Please select the option(s) that is more relevant.