kriswallsmith / assetic

Asset Management for PHP
MIT License
3.75k stars 556 forks source link

node.js + less under windows #69

Open benjamindulau opened 13 years ago

benjamindulau commented 13 years ago

Under windows, node.js requires cygwin to be used properly. But this introduces an issue when using assetic + node/less under windows.

Assetic tries to execute a command like :

node C:\cygwin\tmp\ass37F5.tmp

But under windows, node needs to be run by using unix style paths (and so through cygwin drives). The previous command fails, but the following works well :

node /cygdrive/c/tmp/ass37F5.tmp

I'm not sure how to properly fix this but we can't use Assetic & Less because of that.

michelsalib commented 13 years ago

This is because you are referencing the wrong binary in your assetic configuration. If I remember well, referencing runnode.cmd instead of node.exe will automatically convert the filename into the cygwin compatible one (/cygdrive/c/...)

stof commented 13 years ago

The issue is that the PHP code does not know about cygwin.

benjamindulau commented 13 years ago

@michelsalib: you gave me hope ! Unfortunatly, this is not resolving the issue.

Now it works well when manually executing the command

runnode.cmd C:\cygwin\tmp\ass37F5.tmp

But assetic fails, which is weird because i have logged the command it tries to execute and it is exactly the same.

michelsalib commented 13 years ago

Yes, I came to the same issue. The command is good but assetic fails to extract the result. I unsuccessfully tried to understand and resolve it, so I did not posted about it. What I did is using lessphp, which is easier to integrate into assetic : https://github.com/leafo/lessphp

stof commented 13 years ago

@michelsalib this works for less but the other filter using node.js don't have a PHP implementation

michelsalib commented 13 years ago

@stof you are right. This is just a temporary solution just for less. Nevertheless I narrowed down the issue to be located where the stream are extracted : https://github.com/kriswallsmith/assetic/blob/master/src/Assetic/Util/Process.php#L131

kriswallsmith commented 13 years ago

@pborreli has found another issue in the Process class that may be related...

pborreli commented 13 years ago

well dunno if it's related but i had problem with Process when i was running google compiler jar so i investigated and found out that replacing fgets by stream_get_contents inside vendor\assetic\src\Assetic\Util\Process.php fixed my issue. I will publish a PR when it's fully tested against assetic and Symfony2 testcases Hope it helps you.

schmittjoh commented 13 years ago

There's another trick that fixes some things on Windows, see https://github.com/schmittjoh/GoogleClosureBundle/blob/master/Command/BaseCommand.php#L137

pborreli commented 13 years ago

had no issue on windows excepted $env which needed to be setted, i will take a look later today to relaunch my testcases on windows.

kriswallsmith commented 13 years ago

@schmittjoh should that be added to the Process component?

pborreli commented 13 years ago

@kriswallsmith imho, i don't think so, it just looks like an old hack which can be fixed easily with a proper implementation of proc_open.

stof commented 13 years ago

@pborreli The point is that the Process component is meant to be usable with PHP. so if PHP has a weird bug on windows, adding it is betetr than having broken code.

benjamindulau commented 13 years ago

@pborreli: maybe the issue exists only with less because it needs node.js and the runnode.cmd command under windows tries to convert DOS style paths to Unix style paths before executing the script. That's in fact a series of command executed one after the other.

pborreli commented 13 years ago

@stof my only point was : let's try to fix it properly instead of using "hacks"

stof commented 13 years ago

but proc_open is a core PHP function so if it is fixed, it would only be in a further release. And your code would still be broken with all older PHP 5.3 version.

pborreli commented 13 years ago

@stof i didn't investigate already so i'm not sure there is a bug inside proc_open, the only thing i see in the related link http://coding.derkeiler.com/Archive/PHP/comp.lang.php/2005-01/1724.html is the date of the post (2005) and a bug about system(), had you reproduce the bug yourself ?

pborreli commented 13 years ago

FYI, rebuilding of the CI server is in progress and will be out with a windows slave, it will help a lot with that kind of issue

benjamindulau commented 13 years ago

Any news about that ? :)

hellomedia commented 12 years ago

This issue may no longer be necessary since Cygwin is not supported anymore for node.js on windows since 2011-10-19.

https://github.com/joyent/node/wiki/Building-node.js-on-Cygwin-(Windows)

ghost commented 11 years ago

@kriswallsmith : what do you think about this now? anything that can be done anymore?