Code Monkey home page Code Monkey logo

Comments (32)

ForbesLindesay avatar ForbesLindesay commented on August 14, 2024

Only applies when using the component build form, component-build gives a different error.

from component.

tj avatar tj commented on August 14, 2024

hmm! not good, is the which() call returning anything? (in bin/component)

from component.

ForbesLindesay avatar ForbesLindesay commented on August 14, 2024

If I run component build with a couple of console.logs I get:

Call which with the string "component-build" then after that call, bin is set to the string "component-build.CMD". The output of that doesn't seem to be used though?

Also, customFds appear to be deprecated I have no idea what they do.

from component.

tj avatar tj commented on August 14, 2024

hmm i dont have a windows vm so i wont be able to reproduce but i'll see what i can do

from component.

tj avatar tj commented on August 14, 2024

the customfds just let you change the stdio file descriptors, so that the component-* commands in this case use the same stdin, but I think since node abstracts away FDs they want you to just pipe() shit around

from component.

ForbesLindesay avatar ForbesLindesay commented on August 14, 2024

Yeh, so the FDs can probably be removed. Not sure what's causing the problems with spawn.

from component.

ForbesLindesay avatar ForbesLindesay commented on August 14, 2024

Out of curiosity I tried spawning path.join(__dirname, cmd) instead, but it didn't really help. I then got the error: CreateProcessW: %1 is not a valid Win32 application.

from component.

geekytime avatar geekytime commented on August 14, 2024

It looks like you can't call cmd or batch files from spawn because they're not directly executable. I've run into similar problems with build scripts in the past.

Anyway, Grunt uses CLI, which uses exec() instead of span(), so I replaced the spawn() call with an exec() call (in place in my bin folder), and that seems to work.

Not sure if there was a reason for using spawn over exec. Presumably the buffer length of the output could be a problem?

A slightly messier alternative would be to build an alternative command for spawn() that invokes the windows cmd executable. You'd have to extract it out into Windows and non-windows versions of the spawn process, of course, but in windows it works like:

var bin = "cmd";
var cmdName = 'component-' + cmd;
//New args will go to cmd.exe, then we append the args passed in to the list
newArgs = ["/c", cmdName].concat(args);  //  the /c part tells it to run the command.  Thanks, Windows...
var proc = spawn(bin, newArgs);

I don't have a non-Windows vm stood up to test, right now, but either of those will probably work okay. Let me know if a pull request would help, but I wasn't comfortable choosing between those solutions without a bit more backstory...

from component.

ForbesLindesay avatar ForbesLindesay commented on August 14, 2024

Great work @geekytime thanks. Have you tried adding node to the front of the commands so that we do spawn('node ' + command) ?

I wouldn't of thought that these process would output enough info for it to matter hugely if it's buffered, it's more that you want to be able to see the progress for long operations as it happens. If it's needed, I'd be in favour of the messier alternative (even if it is a little ugly) it's at least not much code to switch between depending on OS.

from component.

Raynos avatar Raynos commented on August 14, 2024

@visionmedia customFds is replaced by the stdio option

from component.

tj avatar tj commented on August 14, 2024

@Raynos cool I'll have to check that out

from component.

ForbesLindesay avatar ForbesLindesay commented on August 14, 2024

Actually, looks like we could just do:

child_process.exec(command, {stdio: 'inherit'}, callback)

And it would solve our problem of not working on windows and do the fds thing properly.

from component.

geekytime avatar geekytime commented on August 14, 2024

I ended up using a Windows-specific spawn function.

I couldn't get child_process.exec() to work properly for commands like create, which expect user input.

from component.

tj avatar tj commented on August 14, 2024

damn maybe we just need to add which() back then, I have zero windows knowledge, I figured you guys had a PATH alternative

from component.

ForbesLindesay avatar ForbesLindesay commented on August 14, 2024

We do have path, I'm really unclear as to why spawn isn't working tbh.

from component.

tj avatar tj commented on August 14, 2024

seems like something core should be taking care of in some way or another, seeing as the whole point of all this windows stuff is to have "transparent" compatibility

from component.

geekytime avatar geekytime commented on August 14, 2024

The problem is that when you spawn a process in Windows, the file that you give as the executable must be an executable. '.cmd' and '.bat' files in Windows are not directly executable. They're scripts that must be run within the context of an executable shell, such as "cmd.exe".

If you look at the code for child_process, Node is actually invoking 'cmd' in exec() (see line 471), but attempting to spawn the process directly if you use spawn() (see line 618). This spawn() call fails in windows because the cmd files we're trying to execute aren't directly executable.

I'm not an expert in child-processes, but I'm guessing the exec() wasn't working for me because it's designed to return the results of the whole execution, and isn't set up to wait for user input.

That's why I thought our best option is probably to do like they do in child_process.exec(), and invoke cmd.exe on Windows machines. The only question I had was whether to use process.platform, os.platform() or os.type() for the OS check. The docs for that module aren't great, but from a few discussions I found it looked like os.type() might be a slightly more consistent check across versions of OSes, so I used that one.

from component.

geekytime avatar geekytime commented on August 14, 2024

(I started that last post before I saw @visionmedia's post.)

Of course, another option would be to push this down a level, and extend child_process to support a combination of spawn() and exec(). Perhaps an option for spawn() that can force it to launch the process in the native shell?

The design of differentiating between spawning a process and executing a command is probably okay, but I agree that they way it's written now it forces anyone who wants things to work cross-platform to jump through this particular hoop when they want to spawn() an interactive process.

from component.

tj avatar tj commented on August 14, 2024

yeah it's pretty annoying, I'd rather ditch windows all together than have to deal with a bunch of junk that node is supposed to take care of

from component.

geekytime avatar geekytime commented on August 14, 2024

It turns out there's already a discussion about this: Node #2318.

There's a subtle difference between child processes that are pure executables and child processes that are really shell commands, and the cross-platform issues exacerbate it.

Something like libuv should probably be able to handle both in a much more elegant way. We shouldn't need a third-party library to run shell commands.

from component.

tj avatar tj commented on August 14, 2024

yeah our exec() is more like system()

from component.

ForbesLindesay avatar ForbesLindesay commented on August 14, 2024

Hopefully it'll get fixed in core in a future version, but in the mean time it's really important to work around this because we need Windows support to scale in terms of numbers of users. I'm biased in that I'd have to move to something else if you dropped Windows support, but I also think that the whole project would flounder and fail without support for windows.

from component.

tj avatar tj commented on August 14, 2024

I think windows is fail :D haha but yeah, I'm fine with it as long as we can minimize the amount of hacks within our source, at very least moving that sort of stuff to npm

from component.

ForbesLindesay avatar ForbesLindesay commented on August 14, 2024

True, Windows is fail :( I'll build a spawn npm module for us to use that abstracts away the problem (unless someone else wants to take on the task?)

from component.

geekytime avatar geekytime commented on August 14, 2024

Works for me. Thanks for at least trying to keep Component fully working on Windows.

Also: nice solution with the function swap. I spent way too much time working in Java... :P

from component.

tj avatar tj commented on August 14, 2024

@ForbesLindesay did you try which() after this new stdio option?

from component.

ForbesLindesay avatar ForbesLindesay commented on August 14, 2024

What do you mean by which()?

from component.

tj avatar tj commented on August 14, 2024

isaac's "which" module is like the which command, gives you a full path for an executable, he's got a bunch of windows handling stuff in there. I originally removed that because this is quite a bit faster but fails on windows I guess, I'm just wondering if we add which back to resolve the bin, and then spawn if it'll be fine

from component.

ForbesLindesay avatar ForbesLindesay commented on August 14, 2024

I hadn't looked at that. I'm not sure what it gains us though? It would tell us whether the command existed in the path, but we could do that just by checking for the existence of the file in the appropriate location?

from component.

geekytime avatar geekytime commented on August 14, 2024

which() looks like it only manages path and file extension issues. It doesn't look like it will force the launch of the cmd.exe shell. I tried all kinds of hard-coded paths in there to make sure it wasn't a path issue when I was testing, so I don't believe that'll do the trick.

For what it's worth, over in the Node discussions, it looks like @isaacs seems to be in favor of a spawnShell() function on child_process, mainly because it would make it easier to spawn shell commands in both Unix and Windows.

I might take a cut at that spawnShell() method tonight or tomorrow night and see if he'll accept it. If we can get something like that in child_process, we can simply just replace @ForbesLindeasy's fix with the built-in node function.

from component.

ForbesLindesay avatar ForbesLindesay commented on August 14, 2024

Yeh, my intention was to ensure we stuck to exactly the same API so it would be easy to swap out if node ever provided a built in way to hide the incompatibility.

from component.

tj avatar tj commented on August 14, 2024

going with your patch until node resolves this or we have some other alternative that works, thanks!

from component.

Related Issues (20)

Recommend Projects

  • React photo React

    A declarative, efficient, and flexible JavaScript library for building user interfaces.

  • Vue.js photo Vue.js

    🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.

  • Typescript photo Typescript

    TypeScript is a superset of JavaScript that compiles to clean JavaScript output.

  • TensorFlow photo TensorFlow

    An Open Source Machine Learning Framework for Everyone

  • Django photo Django

    The Web framework for perfectionists with deadlines.

  • D3 photo D3

    Bring data to life with SVG, Canvas and HTML. 📊📈🎉

Recommend Topics

  • javascript

    JavaScript (JS) is a lightweight interpreted programming language with first-class functions.

  • web

    Some thing interesting about web. New door for the world.

  • server

    A server is a program made to process requests and deliver data to clients.

  • Machine learning

    Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.

  • Game

    Some thing interesting about game, make everyone happy.

Recommend Org

  • Facebook photo Facebook

    We are working to build community through open source technology. NB: members must have two-factor auth.

  • Microsoft photo Microsoft

    Open source projects and samples from Microsoft.

  • Google photo Google

    Google ❤️ Open Source for everyone.

  • D3 photo D3

    Data-Driven Documents codes.