search

ahoward / systemu

univeral capture of stdout and stderr and handling of child process pid for windows, *nix, etc.
Other
126 stars 33 forks source link

Eliminate use Marshall/YAML in child_program #8

Closed damphyr closed 9 years ago

damphyr commented 13 years ago

Tested on Win7/1.9.2 for which the 2.3.0 version does not work due to encoding problems when marshaling.

This also spares us two disk writes which can only be a good thing.

damphyr commented 13 years ago

I added an implementation that allows us to choose which serialization method to use. You pass :strategy with one of [:yaml,:marshal,:inspect] and it adjusts accordingly. I left :inspect as default and made it so that if an invalid strategy is used it will still use inspect Tested it on Mac OS with 1.8.7 and 1.9.2, I'll see if I can test it on Windows as well. Oh, and I added a couple of unit tests for this functionality as well :)

damphyr commented 13 years ago

And it works on Windows as expected (meaning :inspect works, YAML less so, Marshal not at all)

damphyr commented 13 years ago

Any news/comments on this? I've been using this version in "production" for the past month without any problems with 1.9.2 on Windows.

damphyr commented 12 years ago

I've merged all changes since last time. All tests pass and Windows is functional again

damphyr commented 12 years ago

Shall I bump this once more? It's a real bummer that the default Marshal behaviour crashes on Windows, it kinda defeats the purpose for systemu.

purp commented 12 years ago

@ahoward: Please merge this for the next release of systemu. OpsCode's Ohai depends on systemu, which is now pinned to 2.2.0 for Windows because of this bug.

See issue #14 and http://tickets.opscode.com/browse/OHAI-306 if you need details.

ahoward commented 12 years ago

@purp and @damphyr - if i get a request that will automatically merge and passes tests i'll apply and release a new gem today. too slammed to consider even a trivial merge attm....

ahoward commented 12 years ago

@damphyr also, thanks a shit ton for fixing that.

purp commented 12 years ago

@ahoward, thanks for looking. I completely understand slammed; good luck at getting out from under.

@damphyr, if you've got a chance to refresh the merge and make sure it's clean, you're a hero.

Thanks!

damphyr commented 12 years ago

I'm pretty sure that the pull request is up-to-date, my remote tracking branch did not pull any changes and my last merge is a month after @ahoward 's last change. The one thing you have to do is adjust the version number. I'm already using my changes in production since 2.3.0 was out so for over 4 months now and I haven't stumbled on a problem with inspect. Since I'm using it mainly on windows I made inspect the default serialization strategy, that might be something to consider - the tests really will not catch all the weird command lines ppl use. Unfortunately in cases like this the worst behaving citizens determine policy.

So https://github.com/ahoward/systemu/pull/8#issuecomment-3410518 is still accurate.

ahoward commented 12 years ago

@damphyr the reason i mentioned another request is because the HUBZ are claiming i can' merge cleanly. i'll try. let you know if it doesn't work.

ahoward commented 12 years ago

could one of you guys, with a windows maching, see if my last two commits fix systemu on windoze? summary: parent and child both have correct utf-8 comment. this may just make marshal work...

purp commented 12 years ago

@damphyr it's on you. I don't have a Windows system.

ahoward commented 12 years ago

i've also developed a strategy for running platform independent tests: https://github.com/ahoward/systemu/blob/master/test/systemu_test.rb

i got hung up in the past because *nix has 'ls' while windows has 'dir' - this is brittle, but better that no tests.

ahoward commented 12 years ago

so - hopefully - we can commit failing tests to this suite.

gshutler commented 12 years ago

Don't know if it adds any weight to this but I got this problem and switching to @damphyr's fork sorted it out for me.

damphyr commented 12 years ago

Damn I let this sleep for a long time. Been using my patch for a while and I got around to updating some gems which brought 2.5.2 onto my systems and I can now verify that the problem is still there:

systemu: Error - process interrupted!
o:?ArgumentError:       mesg"?dump format error(0x67):bt[I"lC:/Users/C11865/AppData/Local/Temp/syste
mu_C11865DEV_2216_1612_0.2943002275354055_1/program:5:in `load'?:
encoding"?IBM437I"nC:/Users/C11865/AppData/Local/Temp/systemu_C11865DEV_2216_1612_0.2943002275354055
_1/program:5:in `<main>'?@

I'm going to bring my pull request up-to-date tonight and run a few more tests tomorrow

damphyr commented 12 years ago

OK, I managed to bungle the merge as always. I renamed my test file and it's now running with rake test. I also added a bit of code to keep the default Marshal behaviour on all platforms but Windows and use inspect there. Tests green on my Mac for 1.8.7, 1.9.3 and Rubinius. Pending verification on Windows.

damphyr commented 12 years ago

Patch is now updated and verified on windows. This latest version will keep the original 2.5.2 behaviour on every platform but windows. On windows it will switch to using inspect to avoid the Marshal.load errors that crop up.

btm commented 11 years ago

This doesn't rebase/merge cleanly against master for me:

$ git merge damphyr
Auto-merging systemu.gemspec
CONFLICT (content): Merge conflict in systemu.gemspec
Auto-merging lib/systemu.rb
CONFLICT (content): Merge conflict in lib/systemu.rb
Automatic merge failed; fix conflicts and then commit the result.
btm commented 11 years ago

I merged this into master but it doesn't resolve the encoding problems (OHAI-306) magically for me. I didn't have time to look into it further today.

https://github.com/btm/systemu/tree/windows_encoding

L2G commented 11 years ago

I haven't been able to merge any of @damphyr's patches cleanly myself.

With @btm's branch I'm able to get all 3 tests in test/strategy_test to pass, but both tests in test/systemu_test fail. This is Ruby 1.9.2p290 on Windows XP Pro.

L2G commented 11 years ago

In fact, if I take out the Windows detection and just force it to use the "inspect" strategy unconditionally, the tests still fail. :crying_cat_face:

ahoward commented 11 years ago

i'd love to have a fix but cannot test on windoze currently. inspect makes me a little nervous though. marshal works in windows because ruby works on windoze - we know this to be true. so it seems like the windoze fail can only be caused by encoding issue caused by round tripping to file.

L2G commented 11 years ago

If it will help, I give you a standing offer to test anything you like on Windows. I am presently working at a company where we have to use Windows XP and Ruby 1.9.2 (owing to stringent requirements and a software approval process that makes the U.S. government seem like a lemonade stand by comparison). I'm trying to make a case for using Chef at this company, and Chef seems to hinge on systemu as a critical component, so I have a keen interest in seeing systemu work on 1.9.2 and Windows.

So... anything you want tested, please feel free to throw it my way. :smile:

ahoward commented 11 years ago

@L2G can you first confirm that master currently exhibits the original bug?

damphyr commented 11 years ago

I can confirm that the current gem version exhibits the original bug on a 1.9.2p290 RubyInstaller on Win7 32bit. Just did a gem update and had to revert to my patched version again. What I can do is wipe my branch and redo the patch on master, that should make it easier to merge.

ahoward commented 11 years ago

@damphyr - sounds good. will you review my latest work? if so you will notice that all the reads/writes are occuring with 'rb' and 'wb'. i'd like to distill the bug to an 'a.rb' script that simply

my gut says we are simply missing a 'force_encoding("binary")' or some such in there now. marshal should work on win - or else we have a bug for matz.

L2G commented 11 years ago

Yes, it looks like the bug is still a bug with 4e2d1bc6643c72e769b11094e5d4880fb9f94344.

ruby 1.9.2p290 (2011-07-09) [i386-mingw32]

L2G commented 11 years ago

@damphyr, please let me know when you've rebased to 4e2d1bc and I can give it a try.

damphyr commented 11 years ago

I did a hard reset of my main branch and recommitted my changes. Hopefully this will merge cleanly now. Only did a very fast 'rake test' on 1.9.3 on Mac OS X, I'll get a chance to spin up the Windows boxes on Wednesday. Also, don't forget that the bug can be reproduced with https://gist.github.com/damphyr/3853110 (on 1.9.2 - 1.9.3 does not have the problem)

L2G commented 11 years ago

Vassilis, I tried your newest code and took out the accidentally-committed merge conflict markup. Unfortunately, with 1.9.2p290 and Windows XP, I still get 2 failures out of 2 tests.

damphyr commented 9 years ago

I am bumping this again as I now have a single reproducible failure on Win7 with Ruby 2.1.5 and the following error:

systemu: Error - process interrupted!\u0004\bo:\u0012ArgumentError\b:\tmesg\"\emarshal data too short:\abt[\aI\"mC:/Users/vagrant/AppData/Local/Temp/systemu_zugspitze_5764_6036_0_6230080695073744_1/program:5:in `load'\u0006:encoding\"CP850I\"oC:/Users/vagrant/AppData/Local/Temp/systemu_zugspitze_5764_6036_0_6230080695073744_1/program:5:in `<main>'\u0006;\b@\t:\u0011bt_locations@\a

That CP850 encoding after the load though makes me suspicious as this failure only happens when Jenkins is executing. Very, very weird. I'll go back and bring everything back up to current.

Since it's blocking CI I'll be on it all Monday :smile:

damphyr commented 9 years ago

I eliminated the yaml serialization, it was just complicating things. Also there is a place where the files are not opened in binary mode...in the program file. I will make a separate PR fixing just that and test both versions

damphyr commented 9 years ago

Looks like #39 solved this. Closing the issue and hoping never to have to re-open it again!