rtomayko / posix-spawn

Ruby process spawning library
Other
520 stars 52 forks source link

Support Windows when not using backtick execution #57

Closed parkr closed 10 years ago

parkr commented 10 years ago

Fixes https://github.com/jekyll/jekyll/issues/2556

/cc @piki

parkr commented 10 years ago

@madhur Can you see if this patch will fix the problem you're having?

parkr commented 10 years ago

holy crap @piki when was your postdoc at cornell?! i just graduated from here. holy moly guacamole that is a fun coincidence.

madhur commented 10 years ago

I just tried and observed few things. With latest gem version 0.3.8, it is only a warning and everything works fine:

D:\github\madhur.github.com>jekyll serve -w
Configuration file: D:/github/madhur.github.com/_config.yml
            Source: .
       Destination: ./_site
      Generating...
d:/portablejekyll/x86/ruby/lib/ruby/gems/2.0.0/gems/posix-spawn-0.3.8/lib/posix/spawn.rb:162: warning: cannot close fd before spawn
which: no python2 in (.;d:/portablejekyll/x86/ruby/lib/ruby/gems/2.0.0/gems/nokogiri-1.6.0-x86-mingw32/ext/nokogiri;F:\Portableapps\PortableApps\ConEmu\App\ConEmu\ConEmu;F:\Portableapps\PortableApps\ConEmu\App\ConEmu;C:\Program Files (x86)\NVIDIA Corporation\PhysX\Common;C:\Program Files (x86)\Intel\iCLS Client\;C:\Program Files\Intel\iCLS Client\;C:\Windows\system32;C:\Windows;C:\Windows\System32\Wbem;C:\Windows\System32\WindowsPowerShell\v1.0\;d:\portablejekyll\x86\ruby\bin;d:\portablejekyll\x86\devkit\bin;d:\portablejekyll\x86\git\bin;d:\portablejekyll\x86\Python\App;d:\portablejekyll\x86\devkit\mingw\bin;d:\portablejekyll\x86\curl\bin;C:\adt-bundle-windows-x86_64-20130522\sdk\platform-tools;C:\adt-bundle-windows-x86_64-20130522\sdk\tools;C:\Program Files\Intel\Intel(R) Management Engine Components\DAL;C:\Program Files\Intel\Intel(R) Management Engine Components\IPT;C:\Program Files (x86)\Intel\Intel(R) Management Engine Components\DAL;C:\Program Files (x86)\Intel\Intel(R) Management Engine Components\IPT;D:\apache-maven-3.1.1\bin;D:\gradle-1.8\bin;C:\adt-bundle-windows-x86_64-20130522\eclipse\jdk1.7.0_40\bin;D:\nodejs\;D:\PortableJekyll\x86\Git\cmd;C:\Program Files\Microsoft\Web Platform Installer\;C:\Program Files (x86)\Microsoft ASP.NET\ASP.NET Web Pages\v1.0\;C:\Program Files (x86)\Windows Kits\8.0\Windows Performance Toolkit\;C:\Program Files\Microsoft SQL Server\110\Tools\Binn\;D:\adt\sdk\platform-tools;D:\adt\sdk\tools;D:\adt\sdk\build-tools\18.0.1;D:\adt\eclipse\jdk1.7.0_40\bin;d:\PortableJekyll\x86\ruby\bin;d:\PortableJekyll\x86\git\bin;d:\PortableJekyll\x86\python\bin;d:\PortableJekyll\x86\devkit\bin;d:\PortableJekyll\x86\devkit\mingw\bin;F:\Common Tools\gnuwin32\bin;D:\adt\sdk\platform-tools;D:\Google\google_appengine;d:\maven\bin;d:\mongodb\bin;F:\Common Tools\Sublime Text 2.0.2 x64)

I tried this patch on top of 0.3.8 but had no effect.

piki commented 10 years ago

Is there a unit test that fails with the old version of the code? (i.e., when spawn calls /bin/sh on Windows?) If not, can you add one? Actually, do the tests work at all on Windows? They look pretty Unix-y.

My postdoc was 2006-2008. Looks like I left a couple of years before you started.

parkr commented 10 years ago

Is python2 in any of

?

parkr commented 10 years ago

My postdoc was 2006-2008. Looks like I left a couple of years before you started.

Indeed. Hope you liked being on the Hill. It's pretty grand during this time of the year.

Actually, do the tests work at all on Windows? They look pretty Unix-y.

I can confirm... that the tests aren't running for me at all. Mac OS X 10.9.3.

madhur commented 10 years ago

I do not have python2. See below output:

D:\PortableJekyll\x86>python2
'python2' is not recognized as an internal or external command,
operable program or batch file.

D:\PortableJekyll\x86>python --version
Python 2.7.5

D:\PortableJekyll\x86>which python
d:\portablejekyll\x86\Python\App\python.EXE
parkr commented 10 years ago

@madhur What is the value of status here? https://github.com/tmm1/pygments.rb/blob/v0.6.0/lib/pygments/popen.rb#L39

madhur commented 10 years ago

I did not find any status variable when I try to puts

D:\github\madhur.github.com>jekyll build
Configuration file: D:/github/madhur.github.com/_config.yml
            Source: .
       Destination: ./_site
      Generating...
  Liquid Exception: undefined local variable or method `status' for Pygments:Module in _posts/2011-06-03-reverseengineerchrome.markdown
jekyll 2.1.0 | Error:  undefined local variable or method `status' for Pygments:Module

I assume you meant script ?. This is the value of script variable at that position.

python d:/portablejekyll/x86/ruby/lib/ruby/gems/2.0.0/gems/pygments.rb-0.6.0/lib/pygments/mentos.py

parkr commented 10 years ago

I assume you meant script ?

Yep. Ok, so pygments is using python properly. Your original issue seemed to be that posix-spawn was attempting to use /bin/sh (which doesn't exist on Windows machines, of course). Can you trace where the error happens and where /bin/sh is being chosen? I propose this change somewhat blindly without your assistance.

madhur commented 10 years ago

Yes, I am working on that. Will post an update.

piki commented 10 years ago

Indeed. Hope you liked being on the Hill. It's pretty grand during this time of the year.

Ithaca is a pretty amazing town. Gorgeous, even. I do miss summers there.

I can confirm... that the tests aren't running for me at all. Mac OS X 10.9.3.

Yeah, me neither. Unrelated to this PR. If you're missing rake/extensiontask, just gem install rake-compiler. But even after that, you may well hang on any test that needs SIGPIPE. I'm looking into it.

parkr commented 10 years ago

Yep, it hangs on BacktickTest#test_backtick_huge, which uses a pipe. Poo!

Thanks for responding so quickly today :sparkling_heart:

piki commented 10 years ago

I got the hangs (x2) sorted out. Ignored signals get inherited, which can keep SIGPIPE from working in child processes. There are four other failing tests I'm working on, too. I'll start another PR all that.

Let me know when you're sure this fixes the Windows pygments issue and we'll get it merged.

piki commented 10 years ago

58 fixes all the broken unit tests (and three actual bugs) on OS X and Linux. They're far too unixy to do anything useful on Windows, but it's nice to know they still pass with this PR applied.

parkr commented 10 years ago

Rebased on @piki's changes. All tests run for me locally. @tmm1, what say you? :smiley:

parkr commented 10 years ago

Thank you!!!!! :heart: