Skip to content

Conversation

@stkb
Copy link
Contributor

@stkb stkb commented Feb 3, 2021

In RunEnv.hs:getPurs we were just using the standard Directory.findExecutable to look for psa, forgetting to check for psa.cmd too. In windows this was looking for psa.exe, which doesn't exist.

To help prevent this happening again, a self-written findExecutable function in Prelude is added, instead of just exporting
Directory.findExecutable. This new version always first checks for a .cmd version of the given executable name on Windows.

Fix #693

@stkb
Copy link
Contributor Author

stkb commented Feb 3, 2021

Waiting for CI tests. It's working for me in standard Windows/Cmd.

> spago -v build --purs-args "--stash"
[debug] Running `getGlobalCacheDir`
[debug] Transformed config is the same as the read one, not overwriting it
[debug] Ensuring that the package set is frozen
[debug] Running `spago build`
[debug] Getting transitive deps
[debug] Running `fetchPackages`
[debug] Checking if `purs` is up to date
[debug] Compiling with "psa"
[debug] Running command: `psa compile --stash ".spago/console/v4.4.0/src/**/*.purs" ".spago/effect/v2.0.1/src/**/*.purs" ".spago/prelude/v4.1.1/src/**/*.purs" ".spago/psci-support/v4.0.0/src/**/*.purs" "src/**/*.purs" "test/**/*.purs"`
           Src   Lib   All
Warnings   0     0     0
Errors     0     0     0
[info] Build succeeded.

Copy link
Member

@f-f f-f left a comment

Choose a reason for hiding this comment

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

Thank you @stkb!!

I left a small comment, but otherwise I think we're good to merge 🚀

Also CI seems to be failing because of some leftover import (we have --pedantic set when building PRs on GitHub Actions)

@f-f
Copy link
Member

f-f commented Feb 3, 2021

@ntwilson would you be able to verify that this works for you? (to try out this version of Spago it should be enough to checkout the branch and running stack install)

In `getPurs` we were just using the standard `Directory.findExecutable`
to look for `psa`, forgetting to check for `psa.cmd` too. In windows
this was looking for `psa.exe`, which doesn't exist.

To help prevent this happening again, a self-written `findExecutable`
function in `Prelude` is added, instead of just exporting
`Directory.findExecutable`. This new version will always first check
for a `.cmd` version of the executable name on Windows.
@ntwilson
Copy link
Contributor

ntwilson commented Feb 3, 2021

@ntwilson would you be able to verify that this works for you? (to try out this version of Spago it should be enough to checkout the branch and running stack install)

Yes, it worked!! Thanks, @stkb!
I only tested the global version. Do we have any reason to expect it would work differently through npm/npx? Is there a way to test that from a local build?

@stkb
Copy link
Contributor Author

stkb commented Feb 3, 2021

@f-f Oops yeah missed that 🤦‍♂️. Also I wasn't seeing the redundant import error on my machine because I was still using an old Stack version. Anyway it's all looking good now 👍

@ntwilson Good to hear and I don't see any reason why it shouldn't work if installed through npm.

@f-f f-f merged commit d5c0e5d into purescript:master Feb 3, 2021
@f-f
Copy link
Member

f-f commented Feb 3, 2021

Thank you both! 🙂

@stkb stkb deleted the psa branch February 4, 2021 13:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

PSA integration changed?

3 participants