Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Eliminate use Marshall/YAML in child_program #8

Closed
wants to merge 2 commits into
from

Conversation

Projects
None yet
6 participants
Collaborator

damphyr commented Aug 18, 2011

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.

Collaborator

damphyr commented Aug 24, 2011

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 :)

Collaborator

damphyr commented Sep 1, 2011

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

Collaborator

damphyr commented Sep 23, 2011

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.

Collaborator

damphyr commented Jan 9, 2012

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

Collaborator

damphyr commented Feb 3, 2012

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 Mar 2, 2012

@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.

Owner

ahoward commented Mar 2, 2012

@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....

Owner

ahoward commented Mar 2, 2012

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

purp commented Mar 2, 2012

@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!

Collaborator

damphyr commented Mar 3, 2012

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 #8 (comment) is still accurate.

Owner

ahoward commented Mar 4, 2012

@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.

Owner

ahoward commented Mar 5, 2012

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 Mar 5, 2012

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

Owner

ahoward commented Mar 5, 2012

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.

Owner

ahoward commented Mar 5, 2012

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

gshutler commented May 2, 2012

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.

Collaborator

damphyr commented Sep 26, 2012

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

Collaborator

damphyr commented Sep 27, 2012

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.

Collaborator

damphyr commented Oct 1, 2012

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 Dec 10, 2012

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 Dec 10, 2012

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 Feb 27, 2013

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 Feb 27, 2013

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

Owner

ahoward commented Feb 27, 2013

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 Feb 27, 2013

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. 😄

Owner

ahoward commented Feb 28, 2013

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

Collaborator

damphyr commented Feb 28, 2013

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.

Owner

ahoward commented Feb 28, 2013

@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

  • marshals an object
  • opens and file and writes it to disk
  • read this blob back in
  • tries to unmarshal the object

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 Mar 1, 2013

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

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

L2G commented Mar 1, 2013

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

Collaborator

damphyr commented Mar 4, 2013

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 Mar 4, 2013

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.

Collaborator

damphyr commented Mar 13, 2015

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 😄

Collaborator

damphyr commented Mar 13, 2015

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

Collaborator

damphyr commented Mar 20, 2015

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

@damphyr damphyr closed this Mar 20, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment