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

Uses cross-spawn-async for Windows compatibility #53

Merged
merged 4 commits into from
Jan 10, 2016

Conversation

IrregularShed
Copy link
Contributor

Replaced child_process.spawn with cross-spawn-async for cross-platform
compatibility. (Windows command line has problems with
child_process.spawn.)

Replaced `child_process.spawn` with `cross-spawn-async` for cross-platform
compatibility. (Windows command line has problems with
`child_process.spawn`.)
@keithwhor
Copy link
Owner

Can you write tests for this? I'd like confirmation that this works on all environments, because I haven't spun up a VM with Windows myself. We can test on more environments on TravisCI.

@keithwhor
Copy link
Owner

Great work and thank you very much, btw :)

@IrregularShed
Copy link
Contributor Author

👍 I'll try and get proper tests written over the weekend (unexpectedly busy afternoon in the office, otherwise I'd have had a go already). So far the testing has been "does this work on my Windows machine" (it does) and Travis giving the thumbs up :)

@IrregularShed
Copy link
Contributor Author

(Travis testing on non-Windows boxes I mean by that)

@keithwhor
Copy link
Owner

Thanks so much! Understand the busy office situation. Same here. ;)

@schahriar
Copy link
Contributor

@keithwhor considering that you can't run Windows tests on Travis do you plan to intergrate something like appveyor for the tests? I can write the test (depending on what you would expect) as I am in dire need for this PR to be merged so I can develop on my Windows setup.

@IrregularShed
Copy link
Contributor Author

Tests are going to be hard to achieve for a while - I'm having an absolute nightmare getting the tests to start on Windows. Adjusting the child_process.execSync() commands in runner.js so that \' are replaced with " was enough to get the postgres commands to at least run, but the command for creating the database fails completely when I try and call it from within Node (either in the test script or directly from the REPL) - the process shudders to a halt and remains unresponsive.

@keithwhor
Copy link
Owner

Hey, if you guys are confident this works as intended I have no problem merging at this point. If tests are a nightmare let's just not worry about it.

@schahriar
Copy link
Contributor

That sounds awful :( I might give it a go today and see what I can do. Is this only applicable to runner.js? psql -c \'create database nodal_test;\' -U postgres I think this is the command you are referring to. @keithwhor I haven't tested the PR so don't take my word for it. I believe we should test this properly before leaving the chance for crashing the entire ecosystem (since we rely on child_process to do db stuff)

@schahriar
Copy link
Contributor

@keithwhor are you working on an db adapter system like sails' waterline or sticking with SQL for now? Because we could re-write this in the new CLI changes.

@keithwhor
Copy link
Owner

@schahriar We have a built in db adapter system, not sure what you're asking?

@schahriar
Copy link
Contributor

@keithwhor I was referring to support for other databases (MySQL, Mongo, etc.) but nvm I confused runner.js with the rest of the code forgetting that it was part of the test. cli.js seems to use ./db/commands.js which is what I was looking for.

@schahriar
Copy link
Contributor

@IrregularShed I got runner.js & tests working and from what I see this is mostly because of psql response speed also needed an -a option for some reason. Add me as a collaborator to your fork so I can push the changes to this PR.

@IrregularShed
Copy link
Contributor Author

@schahriar You're added :)

Invoking `psql` through child_process is a bad idea and as we can see
here the behavior is not only inconsistent but rather slow on windows.
This patch adds a few options to the exec command in order to keep
`psql` sane on Windows.
@schahriar
Copy link
Contributor

Seems like fixes for Windows are breaking everything else. I'll setup a bunch of test servers to test the latest commit before pushing it to the CI.

Only way to get it to work on both Windows & Linux (I haven't tested OSX
but I am assuming it would work since the fallback code is the original)
@keithwhor
Copy link
Owner

Looks good to me. Ran fine.

keithwhor added a commit that referenced this pull request Jan 10, 2016
Uses cross-spawn-async for Windows compatibility
@keithwhor keithwhor merged commit 427b886 into keithwhor:master Jan 10, 2016
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.

3 participants