Skip to content
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

Jump to bottom

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

Closed
wants to merge 4 commits into from
Closed

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

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
98c8d69
fix(windows): Find purs.exe on Windows, if available
kofno Oct 30, 2017
725f65a
fix(windows): Add compatibility with "purs.cmd"
kofno Nov 4, 2017
266c0ae
refactor(pursCmd): Replace constCmd with <$
kofno Nov 5, 2017
fd3e98f
refactor(windows): Combine detecting purs executable and managing args.
kofno Nov 19, 2017
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
38 changes: 29 additions & 9 deletions app/Main.hs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -199,9 +199,28 @@ updateImpl config@PackageConfig{ depends } = do
echoT ("Updating " <> pack (show (length trans)) <> " packages...")
forConcurrently_ trans . uncurry $ installOrUpdate (set config)

pursCmd :: IO Text
pursCmd = do
purs <- which "purs"
cmd <- which "purs.cmd"
Copy link
Contributor

Choose a reason for hiding this comment

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

Where is purs.cmd from again?

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@paf31 paf31 Nov 5, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

exe <- which "purs.exe"
return $ fromMaybe "purs" $ msum [ constCmd "purs" purs
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's exit here instead of returning purs?

Copy link
Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes please!

Copy link
Contributor

Choose a reason for hiding this comment

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

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

, findExe cmd
, constCmd "purs" exe
]
where
constCmd :: Text -> Maybe Turtle.FilePath -> Maybe Text
constCmd t = fmap (const t)
Copy link
Contributor

Choose a reason for hiding this comment

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

You can use <$ for this.

Copy link
Author

Choose a reason for hiding this comment

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

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

I will try that out.

appendExeText :: Turtle.FilePath -> Text
appendExeText p =
either id id (toText (p </> "node_modules" </> "purescript" </> "vendor" </> "purs.exe"))
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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?

findExe :: Maybe Turtle.FilePath -> Maybe Text
findExe = fmap (appendExeText . parent)

getPureScriptVersion :: IO Version
getPureScriptVersion = do
let pursProc = inproc "purs" [ "--version" ] empty
purs <- pursCmd
let pursProc = inproc purs [ "--version" ] empty
outputLines <- Turtle.fold (fmap lineToText pursProc) Foldl.list
case outputLines of
[onlyLine]
Expand Down Expand Up @@ -327,17 +346,17 @@ listSourcePaths = do
--
-- Extra args will be appended to the options
exec :: [String] -> Bool -> [String] -> IO ()
exec execNames onlyDeps passthroughOptions = do
exec cmdParts onlyDeps passthroughOptions = do
pkg <- readPackageFile
updateImpl pkg

purs <- T.unpack <$> pursCmd
paths <- getPaths
let cmdParts = tail execNames
srcParts = [ "src" </> "**" </> "*.purs" | not onlyDeps ]
let srcParts = [ "src" </> "**" </> "*.purs" | not onlyDeps ]
exit
=<< Process.waitForProcess
=<< Process.runProcess
(head execNames)
purs
(cmdParts <> passthroughOptions
<> map Path.encodeString (srcParts <> paths))
Nothing -- no special path to the working dir
Expand Down Expand Up @@ -431,7 +450,7 @@ verify inputName = case mkPackageName (pack inputName) of
pkg <- readPackageFile
db <- readPackageSet pkg
case name `Map.lookup` db of
Nothing -> echoT . pack $ "No packages found with the name " <> show (runPackageName $ name)
Nothing -> echoT . pack $ "No packages found with the name " <> show (runPackageName name)
Just _ -> do
reverseDeps <- map fst <$> getReverseDeps db name
let packages = pure name <> reverseDeps
Expand Down Expand Up @@ -461,7 +480,8 @@ verifyPackage db paths name = do
echoT ("Verifying package " <> runPackageName name)
dependencies <- map fst <$> getTransitiveDeps db [name]
let srcGlobs = map (pathToTextUnsafe . (</> ("src" </> "**" </> "*.purs")) . dirFor) dependencies
procs "purs" ("compile" : srcGlobs) empty
purs <- pursCmd
procs purs ("compile" : srcGlobs) empty

main :: IO ()
main = do
Expand Down Expand Up @@ -501,13 +521,13 @@ main = do
(Opts.info (install <$> pkg Opts.<**> Opts.helper)
(Opts.progDesc "Install the named package"))
, Opts.command "build"
(Opts.info (exec ["purs", "compile"]
(Opts.info (exec ["compile"]
<$> onlyDeps "Compile only the package's dependencies"
<*> passthroughArgs "purs compile"
Opts.<**> Opts.helper)
(Opts.progDesc "Update dependencies and compile the current package"))
, Opts.command "repl"
(Opts.info (exec ["purs", "repl"]
(Opts.info (exec ["repl"]
<$> onlyDeps "Load only the package's dependencies"
<*> passthroughArgs "purs repl"
Opts.<**> Opts.helper)
Expand Down