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.

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

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

Closed
wants to merge 4 commits into from
Closed
Changes from all commits
Commits
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
67 changes: 50 additions & 17 deletions app/Main.hs
Original file line number Diff line number Diff line change
Expand Up @@ -199,16 +199,33 @@ 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

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"
let mpurs = msum [ "purs" <$ purs
, "purs" <$ exe
, "purs.cmd" <$ cmd
]
case mpurs of
Nothing -> exitWithErr "The \"purs\" executable could not be found. Please make sure your PATH variable is set correctly"
Just c -> return c

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

Choose a reason for hiding this comment

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

And this one.

outputLines <- Turtle.fold (fmap lineToText pursProc) Foldl.list
case outputLines of
[onlyLine]
| results@(_ : _) <- Read.readP_to_S parseVersion (T.unpack onlyLine) ->
pure (fst (maximumBy (comparing (length . versionBranch . fst)) results))
| otherwise -> exitWithErr "Unable to parse output of purs --version"
_ -> exitWithErr "Unexpected output from purs --version"
where
cmdProc "purs.cmd" = inshell "purs.cmd --version" empty
cmdProc purs = inproc purs [ "--version" ] empty

initialize :: Maybe (Text, Maybe Text) -> IO ()
initialize setAndSource = do
Expand Down Expand Up @@ -327,24 +344,30 @@ 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)
(cmdParts <> passthroughOptions
<> map Path.encodeString (srcParts <> paths))
Nothing -- no special path to the working dir
Nothing -- no env vars
Nothing -- use existing stdin
Nothing -- use existing stdout
Nothing -- use existing stderr
=<< run purs paths srcParts
Copy link
Contributor

Choose a reason for hiding this comment

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

This should use runPurs too.

where
run p paths srcParts =
let args = (cmdParts <> passthroughOptions) <> map Path.encodeString (srcParts <> paths)
in case p of
"purs.cmd" -> Process.runCommand $ List.unwords (p : args)
_ ->
Process.runProcess
p
args
Nothing -- no special path to the working dir
Nothing -- no env vars
Nothing -- use existing stdin
Nothing -- use existing stdout
Nothing -- use existing stderr

checkForUpdates :: Bool -> Bool -> IO ()
checkForUpdates applyMinorUpdates applyMajorUpdates = do
Expand Down Expand Up @@ -431,7 +454,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 +484,17 @@ 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
runPurs srcGlobs
where
runPurs :: [Text] -> IO ()
Copy link
Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Author

Choose a reason for hiding this comment

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

I’ll give it a shot

runPurs globs = do
let args = "compile" : globs
purs <- pursCmd
case purs of
"purs.cmd" -> do
let s = T.intercalate " " $ purs : args
shells s empty
_ -> procs purs args empty

main :: IO ()
main = do
Expand Down Expand Up @@ -501,13 +534,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