Skip to content

Conversation

@colinwahl
Copy link
Collaborator

@colinwahl colinwahl commented Dec 19, 2020

Description of the change

Fixes #683 by implementing a script command, which can be used to run a standalone PureScript file.

To see an example, you can run this command: spago script -d node-fs ~/path/to/script.purs
on this file: https://gist.github.com/colinwahl/ea8f9cc1f13b0ac9e9ba80614d8aff24
to log to the console and write a text file.

This will install the node-fs dependency (along with the default console, effect, and psci-support dependencies), compile the files, and run the main function within the provided file.

It is similar to spago repl, except that it executes the given script instead of opening a repl.

Checklist:

  • Added the change to the "Unreleased" section of the changelog
  • Added some example of the new feature to the README
  • Added a test for the contribution (if applicable)

Open Questions:

  • How much logging should we do?
    • Right now, installation, compilation, warnings, and info logs are included. Given that this may be used in a pipeline, we may want to suppress those logs
  • Are there any particular types of tests you'd like to see for this?
  • I have a few other questions particular to the implementation, I will attach them to the code snippet.

@thomashoneyman
Copy link
Member

It's also worth noting that this PR doesn't make any attempt to do the following:

  • Support installing backend-specific dependencies (ie. no installing via npm install)
  • Support caching (ie. projects are initialized from scratch every time spago script is run)
  • Support shebangs or comments (ie. you won't be able to specify dependencies in the .purs file or chmod +x it).

Those are all well worth doing in followup PRs, but we wanted to keep this small and get the feature in its smallest form implemented.

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.

@colinwahl @thomashoneyman thank you so much for doing this, looks great! There are some things to adjust and I left some comments, thank you for leaving some review pointers too

@f-f
Copy link
Member

f-f commented Dec 20, 2020

@colinwahl to answer your questions:

  1. more logging is better - we log to stderr so the logs shouldn't mix up with the program's output if people use the stdout, and people can anyways use the quiet flag if needed
  2. a good test for this could be the one you posted in your first comment, where we run a script and then check that it writes a file or something like that. Also note that current tests are failing, I suppose because of the changes to the runBackend function

@colinwahl
Copy link
Collaborator Author

I made some progress on this last night and updated some comments. I'll be back later today to continue working on it.

@colinwahl
Copy link
Collaborator Author

@f-f I've run into a bit of an issue with implementing a test to check program output:

It looks like the Compiling <module_name logs actually get logged to stdout, causing some issues in my test, and possibly issues down the line when trying to pipe this output to other commands.

Do you think that is worth prioritizing now? I'm not sure how to get those logs to go somewhere else.

@f-f
Copy link
Member

f-f commented Dec 24, 2020

@colinwahl these lines are outputted by the compiler, which uses stdout pre-0.14 release, but will use stderr as well from 0.14 on - see purescript/purescript#3672 for more info.

0.14 is about to come out, but I'd also be fine with a crappy workaround to make these tests pass as long as we have enough comments pointing out the hack to be removed as soon as we upgrade to the new release. Makes sense?

@colinwahl
Copy link
Collaborator Author

@colinwahl these lines are outputted by the compiler, which uses stdout pre-0.14 release, but will use stderr as well from 0.14 on - see purescript/purescript#3672 for more info.

0.14 is about to come out, but I'd also be fine with a crappy workaround to make these tests pass as long as we have enough comments pointing out the hack to be removed as soon as we upgrade to the new release. Makes sense?

Sounds good! My workaround for now is to not have a test that checks the actual output of a script, but instead one that checks a file created by a script matches what it should. I'll put a comment about wanting some additional tests once 0.14 is out.

@colinwahl
Copy link
Collaborator Author

colinwahl commented Dec 24, 2020

It looks like moving to using absolute paths via RunDirectories in all cases caused some issues for the windows builds, in particular, it looks like we are now trying to require a module 'C:projectsspago\testspago-test-ff9151978c8dbd41/myOutput/Test.Main' from the file C:\\projects\\spago\\test\\spago-test-ff9151978c8dbd41\\.spago\\run.js.

I'm not sure exactly why this is happening but it seems to be something to do with nodeContents and probably fromFilePath.

I'm going to keep investigating but I'm having a bit of trouble. @f-f @thomashoneyman If you've seen something like this before, any tips would be appreciated!

@f-f
Copy link
Member

f-f commented Dec 24, 2020

@colinwahl in my experience trying to work with Windows absolute paths is always extremely fiddly.
Looking at the logs I have a suspicion that we should escape the backslashes inside the require because they are in a JS string?

If that doesn't help, according to these docs together with these docs it looks like we might be able to use unix-like absolute paths with require on Windows too

@colinwahl
Copy link
Collaborator Author

@colinwahl in my experience trying to work with Windows absolute paths is always extremely fiddly.
Looking at the logs I have a suspicion that we should escape the backslashes inside the require because they are in a JS string?

If that doesn't help, according to these docs together with these docs it looks like we might be able to use unix-like absolute paths with require on Windows too

@f-f The unix-like absolute paths do work - I ended up replacing the backslashes (that were improperly escaped) with forward slashes and now things are working on windows.

@colinwahl
Copy link
Collaborator Author

@f-f / @thomashoneyman

I've fix the tests, resolved review comments & finished my checklist - I think this is ready for a final review!

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.

@colinwahl this is almost perfect now! Really great work! 👏 🎉

I left only small suggestions, so once we resolve these we can merge 🙂

Copy link
Member

@thomashoneyman thomashoneyman left a comment

Choose a reason for hiding this comment

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

Agree with @f-f's feedback, and I'm really excited this has come together. Great work, @colinwahl!

Colin Wahl and others added 5 commits December 26, 2020 23:26
Co-authored-by: Fabrizio Ferrai <[email protected]>
Co-authored-by: Fabrizio Ferrai <[email protected]>
Co-authored-by: Fabrizio Ferrai <[email protected]>
Co-authored-by: Fabrizio Ferrai <[email protected]>
Co-authored-by: Fabrizio Ferrai <[email protected]>
@colinwahl
Copy link
Collaborator Author

Thanks @f-f for the feedback & tips, it's all ready now :D

@f-f f-f merged commit 7999f53 into purescript:master Dec 27, 2020
@thomashoneyman thomashoneyman deleted the spago-script branch December 27, 2020 19:01
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.

Feature request: spago script

3 participants