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

Optional spawn sync #18

Closed

Conversation

johanneswuerbach
Copy link
Contributor

Compiling native extensions requires a working compiler toolchain

Related: testem/testem#588

Compiling native extensions requires a working compiler toolchain
@satazor
Copy link
Contributor

satazor commented Jul 1, 2015

//cc @BigstickCarpet

@JamesMessinger
Copy link
Contributor

@johanneswuerbach - I think this is actually an issue with Spawn Sync, not Cross Spawn. Perhaps send them a pull request instead?

If we make spawn-sync an optional dependency, then we have no graceful fallback for Node < 0.12. Our only option is to throw a runtime error, which is not ideal. On the other hand, if Spawn Sync makes thread-sleep an optional dependency, then it has a graceful fallback (it falls-back to a simple loop)

@JamesMessinger
Copy link
Contributor

It looks like Spawn Sync is doing some strange stuff in their package.json file. They don't depend on thread-sleep directly, they depend on try-thread-sleep, which in-turn depends on thread-sleep. What's odd is that try-thread-sleep isn't listed as a dependency or optionalDependency, but rather as a devDependency. They then use a post-install script to install it as a dependency. I'm not sure why they're doing it that way.

The other weird thing is that try-thread-sleep has thread-sleep as an optional dependency, which means this whole issue should be moot, since nowhere in the entire dependency chain is thread-sleep actually a dependency of anything. So, as far as I can tell, thread-sleep (which is the native C module that's causing the whole issue) shouldn't ever actually get installed unless an app explicitly declares it as a dependency.

Or am I missing something?

@johanneswuerbach
Copy link
Contributor Author

Maybe @ForbesLindesay knows more?

Also //cc @stefanpenner

@stefanpenner
Copy link

you are correct, it is in spawn-sync.

@johanneswuerbach the issue in testem is, one cannot use sync spawn of anykind in node < 0.12 without a native extension. So it should be confirmed testem itself doesn't rely on that.

If thats the case, it might be interesting to split node-cross-spawn into another more specific module, node-cross-spawn-async which would never require native extensions.

@johanneswuerbach
Copy link
Contributor Author

Yep, testem is only using the async version. I'll lock to 0.2.9 until this is resolved.

@stefanpenner
Copy link

Yep, testem is only using the async version. I'll lock to 0.2.9 until this is resolved.

👍

@johanneswuerbach
Copy link
Contributor Author

Issue created. Closing.

@johanneswuerbach johanneswuerbach deleted the optional-sync branch July 1, 2015 14:13
@ForbesLindesay
Copy link
Contributor

Spawn-sync only has an optional native dependency (if that fails to build it falls back to busy waiting which is bad for performance but works fine). You will get an error message during installation, which you can safely ignore.

@stefanpenner
Copy link

but works fine)

unfortunately, there is no way to reasonable silence the failure. Users become confused and assume the install failed entirely :(

@feross feross mentioned this pull request Feb 4, 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.

None yet

5 participants