fix(windows): Find purs.exe on Windows, if available #69

kofno · 2017-10-30T04:24:13Z

Since purs isn't executable on Windows, Turtle procs weren't finding
it to execute.

purs.cmd is executable on windows, but it must be run in a shell for
the ~%dp0 macros to expand properly. But running in a shell seems to
break on other platforms (I tested on OS X).

purs.exe seems to work just fine if it is on the path. If it is not
already on the path, we have to find it. In this solution, I'm just
looking for the exe relative to where the purs.cmd file is and
executing it.

Fixes #34

Since `purs` isn't executable on Windows, Turtle procs weren't finding it to execute. `purs.cmd` is executable on windows, but it must be run in a shell for the `~%dp0` macros to expand properly. But running in a shell seems to break on other platforms (I tested on OS X). `purs.exe` seems to work just fine if it is on the path. If it is not already on the path, we have to find it. In this solution, I'm just looking for the exe relative to where the `purs.cmd` file is and executing it.

paf31 · 2017-11-01T19:40:33Z

app/Main.hs

+      constCmd t = fmap (const t)
+      appendExeText :: Turtle.FilePath -> Text
+      appendExeText p =
+        either id id (toText (p </> "node_modules" </> "purescript" </> "vendor" </> "purs.exe"))


I'm not keen on depending on npm here like this. I would encourage users to put the executable on their PATH instead.

Yeah... I can't argue with you there.

The problem that I was working around was that when a user installs purescript with npm, the purs.cmd is what ends up on the path. That is executable, but when run as a proc, the path macros in the file don't expand properly. So purs.cmd has to be run with shell instead.

I did this because I was trying to avoid having to shell out for that one case, but this is kind of a terrible hack 🤷‍♂️

So, I could take a crack at shelling out when we find the purs.cmd but no exe on the path.

Or we could pop an error message when we detect this case that says "Windows users need to have the exe on their path". Maybe even include a link to the latest release or something.

Thoughts?

paf31 · 2017-11-01T19:41:28Z

app/Main.hs

+  purs <- which "purs"
+  cmd <- which "purs.cmd"
+  exe <- which "purs.exe"
+  return $ fromMaybe "purs" $ msum [ constCmd "purs" purs


Let's exit here instead of returning purs?

Sure thing. Perhaps a message like "purs needs to be on your path"?

Yes please!

Maybe "The "purs" executable could not be found. Please make sure your PATH variable is set correctly"?

Windows users who install purescript through npm can't use psc-package because the `purs` script isn't executable on windows. To make this work, we need to find the `purs.cmd` that npm installs on the path and run it in a shell (rather than a process). This change favors `purs` and `purs.exe` (if available on the path) over using the cmd. This also adds a more helpful error message if we can't find a purs executable.

kofno · 2017-11-04T13:39:19Z

Updated:

There's a nicer error message now.

I'm also not fishing for the exe. Instead I use the pur.cmd as a last resort. If I find it on the path I run it in a shell instead of a proc so all the windows path macros expand correctly. Every other platform and circumstance still works as before. This is really just targeting a developer on Windows who has only installed purescript through npm.

I tested this on Windows and OS X.

paf31 · 2017-11-04T22:49:23Z

app/Main.hs

+    Just c -> return c
+    where
+      constCmd :: Text -> Maybe Turtle.FilePath -> Maybe Text
+      constCmd t = fmap (const t)


You can use <$ for this.

Ahhh, cool. Thanks! Haskell is still just a hobby for me.

I will try that out.

paf31 · 2017-11-04T22:50:58Z

app/Main.hs

+pursCmd :: IO Text
+pursCmd = do
+  purs <- which "purs"
+  cmd <- which "purs.cmd"


Where is purs.cmd from again?

When you install an npm package globally using npm (or yarn, I suppose), a cmd file is created for each bin script the package has. The cmd file is executable and passes the bin script and any arguments to node.

So, for example, purescript has bin/purs.js. So npm creates purs.cmd on the path. And purs.cmd runs purs.js; like this --> node "%~dp0\node_modules\purescript\bin\purs.js" %*... well, there's a couple other steps, but that's basically it.

So windows users who use cmd.exe or powershell, instead of an msys terminal or git shell or something, need to be able find and execute the purs.cmd script if they are getting purescript from npm.

I think I would rather leave purs.cmd out here, and encourage Windows users to either a) use the binaries directly, or b) temporarily modify their path to include wherever NPM puts them. I think it would reduce some complexity here, and I really don't think we should build in support for NPM. What do you think?

I don't think that's a good idea - you're basically saying "we don't support the most common way of installing purescript" on Windows if purs.cmd is ignored.

Honestly, I don't think psc-package should have support for building anyway now that Pulp has support for using psc-package. I'd rather delete the code than have custom code for each way PureScript might be installed.

I suppose we do need it for the version check though.

I know it seems overly minimalist, or something, but it bothers me that there is a sensible default on all decent OSes (just put it on your PATH), but not on Windows, and we have to make special cases there, which then have to be documented.

Oh if we're removing build altogether that works too 👍

I feel pretty alienated when project's tooling doesn't support windows native. In this case, the work around isn't that big of a deal, but I'm sure it will still drive people away.

We could remove build, but then we'd still have verify which uses purs.

Yeah we need it to check the compiler version too, actually, so removing it isn't really a solution.

Of course there was an operator for that...

kofno · 2017-11-05T20:26:10Z

Not supporting puts.cmd also breaks init and verify

paf31 · 2017-11-18T21:53:57Z

app/Main.hs

+  run purs srcGlobs
+  where
+    run :: MonadIO io => Text -> [Text] -> io ()
+    run "purs.cmd" globs =


I think this special case should be fused into the function which finds the executable.

i.e. one function which finds purs and runs it with some arguments.

So, a runPurs command, looks up the executable and does the right thing with the globs to execute it?

Yes, I think so.

ok... Let me try it and if it's not what you had in mind, I'll be glad to change it.

Move logic for managing args and discovering executable name into a single function.

kofno · 2017-11-19T01:15:46Z

app/Main.hs

-  procs "purs" ("compile" : srcGlobs) empty
+  runPurs srcGlobs
+  where
+    runPurs :: [Text] -> IO ()


I made this change, @paf31. Not sure if that's what you had in mind.

Thanks, this is closer, but what I meant was to move this function to the top level, and to combine it with pursCmd. Could you please try that?

I’ll give it a shot

paf31 · 2017-11-19T19:17:35Z

app/Main.hs

-          Nothing -- use existing stdin
-          Nothing -- use existing stdout
-          Nothing -- use existing stderr
+    =<< run purs paths srcParts


This should use runPurs too.

paf31 · 2017-11-19T19:17:43Z

app/Main.hs

 getPureScriptVersion :: IO Version
 getPureScriptVersion = do
-  let pursProc = inproc "purs" [ "--version" ] empty
+  purs <- pursCmd
+  let pursProc = cmdProc purs


And this one.

stkb · 2018-05-25T17:34:14Z

I know very little about Haskell but would it be possible to just run the command in a shell on Windows and not on other platforms? The new add-from-bower command has the same problem.

…urescript#69)

justinwoo · 2018-11-27T08:02:15Z

Can we consider this fixed by #141?

justinwoo · 2019-02-03T16:54:15Z

Closing since #141 should have fixed this problem and there has been no activity here.

paf31 reviewed Nov 1, 2017

View reviewed changes

paf31 reviewed Nov 4, 2017

View reviewed changes

refactor(pursCmd): Replace constCmd with <$

266c0ae

Of course there was an operator for that...

paf31 reviewed Nov 18, 2017

View reviewed changes

refactor(windows): Combine detecting purs executable and managing args.

fd3e98f

Move logic for managing args and discovering executable name into a single function.

kofno commented Nov 19, 2017

View reviewed changes

paf31 reviewed Nov 19, 2017

View reviewed changes

justinwoo added the waiting on revisions label Feb 11, 2018

Rembane pushed a commit to Rembane/psc-package that referenced this pull request May 26, 2018

Add halogen, halogen-vdom, dom-indexed, fork, freeap and update dom (p…

b07adaf

…urescript#69)

This was referenced Nov 21, 2018

Fix Windows only problem of wrapper command built by npm #140

Closed

Fix purs command problem for Windows #141

Merged

justinwoo closed this Feb 3, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(windows): Find purs.exe on Windows, if available #69

fix(windows): Find purs.exe on Windows, if available #69

kofno commented Oct 30, 2017

paf31 Nov 1, 2017

kofno Nov 2, 2017

paf31 Nov 1, 2017

kofno Nov 2, 2017

paf31 Nov 2, 2017

paf31 Nov 2, 2017

kofno commented Nov 4, 2017

paf31 Nov 4, 2017

kofno Nov 4, 2017

paf31 Nov 4, 2017

kofno Nov 4, 2017

paf31 Nov 5, 2017

garyb Nov 5, 2017

paf31 Nov 5, 2017

paf31 Nov 5, 2017 •

edited

Loading

hdgarrood Nov 5, 2017

kofno Nov 5, 2017

gabejohnson Nov 29, 2017

paf31 Nov 29, 2017

kofno commented Nov 5, 2017 via email

paf31 Nov 18, 2017

kofno Nov 18, 2017

paf31 Nov 18, 2017

kofno Nov 18, 2017

kofno Nov 19, 2017

paf31 Nov 19, 2017

kofno Nov 19, 2017

paf31 Nov 19, 2017

paf31 Nov 19, 2017

stkb commented May 25, 2018

justinwoo commented Nov 27, 2018

justinwoo commented Feb 3, 2019

fix(windows): Find purs.exe on Windows, if available #69

fix(windows): Find purs.exe on Windows, if available #69

Conversation

kofno commented Oct 30, 2017

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

kofno commented Nov 4, 2017

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

Choose a reason for hiding this comment

paf31 Nov 5, 2017 • edited Loading

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

kofno commented Nov 5, 2017 via email

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stkb commented May 25, 2018

justinwoo commented Nov 27, 2018

justinwoo commented Feb 3, 2019

paf31 Nov 5, 2017 •

edited

Loading