Add support for Windows NPM #658

MetaMmodern · 2022-06-29T18:41:28Z

Description

These changes add support for Windows OS. Was tested on Windows 10 (x64), server and web-client start now.

⚠️ I am not a haskell developer!: My code may be ugly and even disguisting, but that's what I've achived by googling)

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 the npm-cli. The reason and solution is explained here: https://stackoverflow.com/a/44820337 . The implementation is made in compileOsSpecificNodeCommand Common function and in src/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.

Bug fix (non-breaking change which fixes an issue)
This change requires a documentation update

Martinsos

@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.

Martinsos · 2022-06-30T08:46:45Z

waspc/src/Wasp/Generator/Common.hs

  )
 where

 import qualified Wasp.SemanticVersion as SV
+import System.Info (os)
+import Data.List


We try to import stuff qualified or named, so we don't pollute the namespace.
Therefore, best to do import Data.List (intercalate)!

Martinsos · 2022-06-30T08:48:32Z

waspc/src/Wasp/Generator/Common.hs

@@ -38,3 +42,12 @@ npmVersionRange =

 prismaVersion :: SV.Version
 prismaVersion = SV.Version 3 15 2
+
+oSSpecificNpm :: [Char]


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.

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.

@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?

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.

Martinsos · 2022-06-30T08:52:49Z

waspc/src/Wasp/Generator/Common.hs

+oSSpecificNpm :: [Char]
+oSSpecificNpm = "npm" ++ if os /= "mingw32" then "" else ".cmd"
+
+compileOsSpecificNodeCommand :: [Char] -> [[Char]] -> ([Char], [[Char]])


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.

Martinsos · 2022-06-30T08:57:50Z

waspc/src/Wasp/Generator/Job/Process.hs

@@ -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


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?

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?

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?

Martinsos · 2022-06-30T09:06:23Z

Ah I realize now you explained in the description why we are doing the cmd.exe thing! Ok I will check it out sorry about that -> I have to run now but I should be able to get back to it in in an hour or two, I will read it then and write more educated comment!

Martinsos · 2022-06-30T10:51:47Z

Alright, read about cmd.exe and left the comment, that is it from me for this first iteration of review!

Martinsos · 2022-06-30T10:53:00Z

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.

MetaMmodern · 2022-06-30T11:14:15Z

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)

…utf8.

Martinsos · 2022-06-30T17:47:42Z

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 main branch, so make sure to pull it before you continue.
This should enable us to move forward again :)! I believe there will be another problem, which is the fact that script command does not exist on Windows, and we currently need it for smth, but let's see if that is a problem or not first and then we can consider it more.

MetaMmodern · 2022-07-02T15:25:44Z

@Martinsos for db migration similar error:

So wasp start starts, but because db is empty it's stuck at Loading....

For the rest review:

Namings changed.
Comments left.
Files Linted.

(cabal dependencies installation is extremely slow, even npm is not so slow. shame on cabal)

Martinsos · 2022-07-02T15:53:27Z

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).
I found this library now https://hackage.haskell.org/package/encoding-0.8.7/docs/Data-Encoding.html, once we add that we should have support for CP866 and many other encodings -> I just need to find some time to do it properly since it is not super simple due to them complicating things a bit with Monads!

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

Martinsos · 2022-07-02T15:36:45Z

waspc/src/Wasp/Generator/Common.hs

+
+-- | 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])


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?

Martinsos · 2022-07-02T15:37:40Z

waspc/src/Wasp/Generator/Common.hs

+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.


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.

moved comments into implementation

Martinsos · 2022-07-02T15:39:08Z

waspc/src/Wasp/Generator/Job/Process.hs

  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


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.

applied suggested change Co-authored-by: Martin Šošić <Martinsos@users.noreply.github.com>

Martinsos

@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?

MetaMmodern · 2022-07-04T14:35:43Z

@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

Martinsos · 2022-07-04T15:11:16Z

@MetaMmodern I think I found the reason -> there was this flag for this new encoding package that sometimes causes problems. I turned it off since we don't use it -> let's see now if it helps.
What sucks is that encodings package takes really long time to build, 5-10 minutes, I am not sure why, it is reported on their GH repo as an issue but not resolved yet. But there aren't really any better alternatives out there.

TODO for myself: maybe try with the knob package -> wrap bytestrings into file handlers and let withUtf8 handle it. But does withUtf8 support wide array of encodings or not?

MetaMmodern · 2022-07-04T15:13:06Z

@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)

Martinsos · 2022-07-04T15:26:16Z

@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.

Martinsos · 2022-07-04T15:32:47Z

It builds now @MetaMmodern

MetaMmodern · 2022-07-04T15:34:43Z

It builds now @MetaMmodern

ok. checking locally

MetaMmodern · 2022-07-04T16:05:52Z

@Martinsos nnnnnope)

Martinsos · 2022-07-04T16:08:30Z

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: ***@***.***>

MetaMmodern · 2022-07-04T16:22:57Z

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.

Martinsos · 2022-07-04T19:07:24Z

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 scriptand it seems to be enough to get prisma db migrate-dev to work (at least for now):

let job = runNodeDependentCommandAsJob J.Db serverDir ("cmd.exe", pure $ unwords $ "/c" : npxPrismaMigrateCmd)

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.

MetaMmodern · 2022-07-04T19:23:47Z

@Martinsos hey, congrats on setting up Windows. For encoding: try running chcp 866 in cmd window. Should work. Got it from here: https://superuser.com/questions/1170656/windows-10-terminal-encoding

Martinsos · 2022-07-04T21:15:12Z

Thanks, I tried that @MetaMmodern ! But I can't reproduce the failure to parse, the one with \xad error.

I did a final commit, which:

Removes usage of script for Windows -> for me wasp db migrate-dev now works! But it didn't ask me any questions. I know sometimes it asks, and I wonder if in that case it will not work. But I don't know how to get it to ask. Maybe you can try running wasp db migrate-dev, see how it goes?
I reversed encoding solution to the most simple one, which works for me regardless of encoding. I wonder if this will still work for you, or will cause problems again.

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 encoding library, and go with something simple, either just decodeUtf8Lenient, or use knob.

MetaMmodern · 2022-07-05T12:27:20Z

@Martinsos What does this mean?

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?

Martinsos · 2022-07-05T18:20:00Z

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: ***@***.***>

MetaMmodern · 2022-07-05T19:04:20Z

@Martinsos okay, I'll try it now.

MetaMmodern · 2022-07-06T10:31:02Z

@Martinsos In Ukraine we call this "PEREMOGA", which means "a win"💪🏼

Migration done.

App running--done.

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.
You can get your host using this command: wsl hostname -I. Source is here: https://jkarelins.medium.com/run-postgresql-database-in-docker-container-on-wsl2-656ed3c02280

So that's it)

Martinsos · 2022-07-06T21:16:40Z

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.
Let me keep this PR open few more days so I can do some final tweaks regarding the encodings + rebase it on newest master since some changes are coming there that we will need to adapt to, but everything important is done! Yay :)

Martinsos · 2022-07-07T13:34:40Z

@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 ?, the exotic ones.

MetaMmodern · 2022-07-07T13:38:57Z

@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.

MetaMmodern · 2022-07-07T14:43:38Z

@Martinsos

looks fine to me. well, except the question marks.

Martinsos · 2022-07-08T09:43:09Z

@Martinsos looks fine to me. well, except the question marks.

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!

MetaMmodern · 2022-07-09T08:06:46Z

@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)

add support for windows NPM

6148029

MetaMmodern changed the title ~~add support for Windows NPM~~ Add support for Windows NPM Jun 29, 2022

lint: remove redundant import

8fb15a2

Martinsos requested changes Jun 30, 2022

View reviewed changes

Fixed path in generated JS file that was not always Posix.

18d5cca

Now we decode output of subprocesses with locale encoding instead of …

496d1b3

…utf8.

MetaMmodern added 5 commits July 2, 2022 16:35

fix: types to be Strings

d9b3c4a

lint: clear the namespace

53f14df

fix: bad naming

bea2d07

add function comments

14001b5

format: with ormulu

2ddeb9f

MetaMmodern requested a review from Martinsos July 2, 2022 15:28

Martinsos reviewed Jul 2, 2022

View reviewed changes

MetaMmodern and others added 4 commits July 2, 2022 19:38

Update waspc/src/Wasp/Generator/Common.hs

e9a4f24

applied suggested change Co-authored-by: Martin Šošić <Martinsos@users.noreply.github.com>

fix: change call signature to npm only

bdafb28

Merge branch 'main' of https://github.com/MetaMmodern/wasp

b7316d2

fix: comments placement

8f7d68d

MetaMmodern requested a review from Martinsos July 2, 2022 18:24

Martinsos added 3 commits July 4, 2022 12:02

Fixed compiler errors, refactored node job function a bit.

9b531fb

Renamed node-cmd-as-job function.

09a6a51

Added additional encodings support to Wasp.

9115d5e

Martinsos approved these changes Jul 4, 2022

View reviewed changes

Modified encodings flag.

061af90

Fixed more for Windows

b18bfb5

Finally fixed encodings?

f0a3771

Martinsos added 2 commits July 7, 2022 15:10

Implemented encoding function for ByteString.

ed900ae

Removed knob library and now using direct decoding.

6d8c376

Martinsos added hacktoberfest This issue is perfect for HacktoberFest contributors hacktoberfest-accepted labels Oct 4, 2022

sodic assigned Martinsos Oct 11, 2022

Martinsos force-pushed the main branch from 9cbc333 to 799ff1d Compare October 12, 2023 12:32

Add support for Windows NPM #658

Are you sure you want to change the base?

Add support for Windows NPM #658

Conversation

MetaMmodern commented Jun 29, 2022 • edited

Description

Code explanation

Things to do further

Type of change

Martinsos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Martinsos Jun 30, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Martinsos commented Jun 30, 2022

Martinsos commented Jun 30, 2022

Martinsos commented Jun 30, 2022

MetaMmodern commented Jun 30, 2022

Martinsos commented Jun 30, 2022

MetaMmodern commented Jul 2, 2022 • edited

Martinsos commented Jul 2, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Martinsos left a comment

Choose a reason for hiding this comment

MetaMmodern commented Jul 4, 2022

Martinsos commented Jul 4, 2022

MetaMmodern commented Jul 4, 2022

Martinsos commented Jul 4, 2022

Martinsos commented Jul 4, 2022

MetaMmodern commented Jul 4, 2022

MetaMmodern commented Jul 4, 2022

Martinsos commented Jul 4, 2022 via email

MetaMmodern commented Jul 4, 2022

Martinsos commented Jul 4, 2022

MetaMmodern commented Jul 4, 2022

Martinsos commented Jul 4, 2022 • edited

MetaMmodern commented Jul 5, 2022

Martinsos commented Jul 5, 2022 via email

MetaMmodern commented Jul 5, 2022

MetaMmodern commented Jul 6, 2022

Martinsos commented Jul 6, 2022

Martinsos commented Jul 7, 2022 • edited

MetaMmodern commented Jul 7, 2022

MetaMmodern commented Jul 7, 2022

Martinsos commented Jul 8, 2022

MetaMmodern commented Jul 9, 2022 • edited

MetaMmodern commented Jun 29, 2022 •

edited

Martinsos Jun 30, 2022 •

edited

MetaMmodern commented Jul 2, 2022 •

edited

Martinsos commented Jul 4, 2022 •

edited

Martinsos commented Jul 7, 2022 •

edited

MetaMmodern commented Jul 9, 2022 •

edited