Initialize .purs-repl file.

[chexxor]

Would be nice to initialize a .purs-repl file having "import Prelude". This would allow newcomers to PS to do pulp init followed by 1+1 and not get a "Unknown operator (+)" error.

This file would only be read by psci/purs repl when purescript/purescript#2735 is merged, and likely included in the PS v0.11 release.

I haven't build/tested this change, yet, as my npm run build is failing, and I can't find further instructions in this repo for building.

[hdgarrood]

What's the error?

[chexxor]

The psvm isn't working properly. I tried to resolve by npm install psvm, which successfully installs, but npm install still fails. I do bower install and pulp build (with my global installation of pulp), and that is a success, but npm install and npm run build don't work.

$ npm install
...
Error: shell command exited with: null
    at spawnSync (/home/team/workspace/scripts.js:115:11)
    at /home/team/workspace/scripts.js:90:5
    at Array.forEach (native)
    at ensurePscVersionsInstalled (/home/team/workspace/scripts.js:89:12)
    at prepublish (/home/team/workspace/scripts.js:58:3)

[hdgarrood]

Might be a psvm bug, then. Hm. Seems to be failing during psvm install v0.10.7; if you run that manually what happens?

[chexxor]

Yeah, that works fine.

$ ./node_modules/.bin/psvm install v0.10.7
Version v0.10.7 is already installed

I can't figure out why that scripts.js file is executed when I do npm install.

{ error: 
   { [Error: spawnSync psvm use v0.10.7 ENOENT]
     code: 'ENOENT',
     errno: 'ENOENT',
     syscall: 'spawnSync psvm use v0.10.7',
     path: 'psvm use v0.10.7',
     spawnargs: [] },

I added psvm to PATH, but it's still failing...

[hdgarrood]

Running scripts.js is supposed to happen, it's part of the prepublish script in package.json. It should use a locally installed psvm too, and also npm scripts set up your PATH for you so you shouldn't need to modify it.

[chexxor]

I managed to fix the issues I had with scripts.js. I'm not sure the exact problem, but I changed the call to "spawnSync" to pass the command's args to the second parameter, "args List of string arguments".

Note that I also removed the extra quoting around "bower_components/purescript-/src/**/.purs". I'm guessing that the globbing is a little goofy when passing into a subshell. I'm not sure where this is expanded now.

With this fixed, I was able to test the pulp init and observe a ".purs-repl" file was created.

[hdgarrood]

I'd rather not make changes like this to scripts.js unless we know exactly what's causing it, especially since this error only seems to occur on your machine - it's always worked on mine and on Travis, but with this change Travis has started failing.

[hdgarrood]

Oh also, purs expands globs itself, and in fact I think it's best to pass globs to purs whenever possible instead of using the shell because it makes things more likely to work across different operating systems.

[chexxor]

Right, I agree. I'm on Fedora 25, btw. I'll reboot and retry the standard build later.

[hdgarrood]

What version of Node.js are you using, by the way?

[chexxor]

The package.json specifies node > 4 and mine is node 4.6, IIRC. I'll update later when I'm on that OS again.

[chexxor]

Yeah, issue persists after reboot and trying again, on both Node v4.4.6 and v4.7.0.
bash version: "version 4.3.46(1)-release (x86_64-pc-linux-gnu)".

[hdgarrood]

No thank you; this version range applies to people trying to run pulp, so it shouldn't be influenced by requirements for development (if it is true that they are tighter). 4.x.x is very old now too.

[hdgarrood]

What happens if you update to node 6?

[chexxor]

Yeah, I wanted to commit this to see if that would fix the CI run - I saw the CI was using v6 before, while I was using v4. I'm using v4 in my own projects because I had troubles with latest node versions because of its flat dependency structuring. I'll try building in a Docker image using Node 6.

[hdgarrood]

I think you'll have to modify .travis.yml and/or appveyor.yml if you want the CI to use a different version of Node.

[chexxor]

Ah, yeah, you're right - my change to the "engine" property didn't control the version of Node used by Travis. That property does control the node version used by buildpack apps, it's worth noting, which is used in Heroku and its clones.

I was able to successfully build using Node v6 by using this command:

[chexxor@fedora] $ docker run -it --rm -v `pwd`:/usr/src/app -w /usr/src/app node:6.10.0 bash
root@7dfde777ca59 $ cd usr/src/app
root@7dfde777ca59 $ npm run build
# ... all runs ok ...

Do you want me to create a CONTRIBUTING file? Or a section in README?

[hdgarrood]

It would be better to check the node version at the beginning of scripts.js and fail with an error if it's too old, I think.

[hdgarrood]

I think at this point we can fairly safely say that this was due to an API change; sometime between 4.x and 6.x spawnSync must have gained the ability to use a shell command. Now that we know what's causing it, I think I prefer your original approach, come to think of it.

[hdgarrood]

The only issue I can see with your original fix is that it doesn't quite preserve all the information faithfully when printing a command. I think it would be preferable to use util.inspect on the args array than to join them with spaces. Then there's less risk of it being misinterpreted, I think.

[chexxor]

@hdgarrood I don't quite follow - you would like to support Node v4 users? If so, we would split the command's executable from it's args, as I first tried.

However, I agree with your original suggestion - it's ok to expect people to use the latest Node version for building pulp.

[hdgarrood]

I would like to use the variant that doesn't involve shells, because quoting things is a pain and because shells are a great way of accidentally breaking things on OSes you aren't developing on. One byproduct of this is that it enables people to develop on node v4 which I guess is nice but I'm not really fussed about that.

[chexxor]

Looks like Node v4 doesn't expand the globs while Node v6 does. See how the arg is expanded when using "echo" instead of "psa":

  "compile": {
     cmd: "echo",                                                                                                                        
     args: ["bower_components/purescript-*/src/**/*.purs"]
     },
// in node v4
>> echo [ 'bower_components/purescript-*/src/**/*.purs' ]
bower_components/purescript-*/src/**/*.purs
// Seems to pass it to subshell *quoted*, like `echo "bower_components/purescript-*/src/**/*.purs"`
// in Node v6
>> echo [ 'bower_components/purescript-*/src/**/*.purs' ]
bower_components/purescript-ansi/src/Ansi/Codes.purs bower_components/purescript-ansi/src/Ansi/Output.purs
// Seems to pass it to subshell *unquoted* like `echo bower_components/purescript-*/src/**/*.purs`

This means that configuring args in one way produces two different behaviors in Node v4 and v6.

  "compile": {
      cmd: "psc",                                                                                                                           
      args: [
        "-c",
        "\"bower_components/purescript-*/src/**/*.purs",
        "src/**/*.purs",
      ]   
    }, 
// is equivalent to this (note the quoted quotes) in Node v4 (which fails)
// $ psc -c ' "bower_components/purescript-*/src/**/*.purs" '  ' "src/**/*.purs" '
// and this in Node v6 (which works)
// $ psc -c   "bower_components/purescript-*/src/**/*.purs"     "src/**/*.purs"

I think the Node v4 doesn't support the "shell" option to "spawnSync". Looks like it was added in Jan 2016 by nodejs/node#4598. Looks like it was also backported to Node v4 at some point, but only >v4.8 (moxystudio/node-cross-spawn#69). I was using v4.6. :( I tried in v4.8 and confirm it works.

[hdgarrood]

My impression was that using the calling style with a single string for the command and then an array of strings for the arguments would mean that it doesn't use a shell. Is that not correct?

[chexxor]

Here's what that "shell" flag does: https://github.com/nodejs/node/blob/98e54b0bd4854bdb3e2949d1b6b20d6777fb7cde/lib/child_process.js#L410

If true, it turns the command and args into a single string and puts it into a shell: /bin/sh '-c' 'psc "bower_components/purescript-*/src/**/*.purs" "src/**/*.purs"'.

If false, it just executes it as-is, I believe: 'psc "bower_components/purescript-*/src/**/*.purs" "src/**/*.purs" ', which fails with Error: spawnSync psc "bower_components/purescript-*/src/**/*.purs" "src/**/*.purs" ENOENT, because this string isn't a valid command.

[chexxor]

I wasn't able to get the commands to work with shell: false. I tried with both single string and with array of args and with all versions of node.

[hdgarrood]

Are you sure that's what it happens if you provide an args array and don't specify shell:true? That seems strange to me; how would you pass arguments to a child process without a shell?

[hdgarrood]

The docs for node 6 say shell: false is the default though: https://nodejs.org/dist/latest-v6.x/docs/api/child_process.html#child_process_child_process_spawnsync_command_args_options

[hdgarrood]

Anyway this is not really related to the original issue - can you please add a note to the commit message that this fixes whichever issue it is, and then I'll merge?

[hdgarrood]

If you'd like I'm very happy to keep discussing this in a new PR.

[hdgarrood]

Keep discussing the shell issue, I mean.

[chexxor]

Updated commit to include #259.

Very kind of you, but I am so burned out on that shell issue. Better to just use latest versions of Node.

[hdgarrood]

Understandable. Thank you!