Fix YAML Regression in 1.9 #16

Closed
wants to merge 2 commits into
from

Projects

None yet

3 participants

@5long
Contributor
5long commented Mar 26, 2013

After I could build a sup.gem locally, the first program I run was sup-config. Syck and Psych behave differently and current codebase just assumes Syck is used everywhere. Problem is, sup-config loads yaml(in 1.9 that's Psych) before lib/sup.rb, and the whole ruby process just uses Psych all the way down, producing this bug.

This patch fixed it.

5long added some commits Mar 26, 2013
@5long 5long A regression test for YAMLing in 1.9 5d31e5e
@5long 5long Ensure Syck is used.
In the long run we'd better drop Syck in favor of Psych
a3d313f
@5long
Contributor
5long commented Mar 30, 2013

I've rebased this branch against upstream/develop so it builds on Travis. 15.1 builds successfully and 15.2 failed due to date format of ncursesw gem.

@gauteh
Member
gauteh commented Apr 3, 2013

The fail against the date format of ncursesw: is this a real issue with all ruby 1.8 installs?

@5long
Contributor
5long commented Apr 4, 2013

I believe it's an issue of Ruby 1.8 and a recent version of rubygem. I'm thinking about switching to ncurses-ruby which does support wide char display on my Arch box. But I'm not sure if it works on Mac and Ubuntu (and other linux distros).

@gauteh
Member
gauteh commented Apr 13, 2013

As mentioned in #17, Ruby 2.0 seems to have dropped syck altogether. I think now is the time to go Psych.. since ensuring 'syck' won't work in Ruby 2.0.

@5long
Contributor
5long commented Apr 15, 2013

At first I'm working toward a Ruby 1.8-compatible release instead of 2.0-compatible one. But dropping 1.8 support would make it a lot easier to release a usable version. I'm glad to see this pull request closed if that matches the roadmap.

@gauteh
Member
gauteh commented Apr 15, 2013

Yes, as much as I would like a usable version right away I would prefer to do it right. A possibility is to just branch of a quick-and-dirty 1.8/1.9 bugfix release (which doesn't need any migration)? I think this patch would go against finally migrating to Psych.

A 1.8/1.9 bugfix release could also go into the roadmap for preliminary step. What do you think?

@gauteh
Member
gauteh commented Apr 15, 2013

By the way; I don't care much for 1.8 or 1.9 - I am all for only 2.0.0 if that is an option.

@5long
Contributor
5long commented Apr 15, 2013

Personally I'm using 1.9 for all my day-to-day Ruby apps(Jekyll, Vagrant) so I'm okay with a 1.9/2.0 only release.

I'm more toward a quick-and-dirty approach so I can just start using it without too much hassle. Besides, I just hesitate to do large-scale migration with enough familiarity with the existing code base. Especially when I'm not certain how much time I might put into sup's development.

So my advice is: find out who's willing to put the most effort into sup development, and let him/her decide :)

@gauteh
Member
gauteh commented Apr 15, 2013

Well said. I neither have much time to spare.

Cheers, Gaute

@gauteh
Member
gauteh commented Apr 20, 2013

Did some testing on this branch, and it seems to work fine on 1.9.3. I think getting ruby 2, psych and all that working requires some 1.9.3/syck breakage anyway. If noone objects I'd merge this one.

@ericweikl
Contributor

+1 Just gave it a try and didn't notice any problems. Kudos for adding a test :-)

@gauteh gauteh added a commit that referenced this pull request Apr 20, 2013
@5long @gauteh 5long + gauteh Merge #16: Fix YAML regression in 1.9
Squashed commit of the following:

commit a26b99c64e2da4115ea47606ae8b66f0b008b002
Author: Whyme Lyu <callme5long@gmail.com>
Date:   Tue Mar 26 23:03:50 2013 +0800

    Ensure Syck is used.

    In the long run we'd better drop Syck in favor of Psych

commit a99a2b244063c2088d91688f667ee68f05f24df0
Author: Whyme Lyu <callme5long@gmail.com>
Date:   Tue Mar 26 23:03:25 2013 +0800

    A regression test for YAMLing in 1.9
ac82b28
@gauteh gauteh added a commit to gauteh/sup that referenced this pull request Apr 20, 2013
@gauteh gauteh Revert "Merge #16: Fix YAML regression in 1.9"
This reverts commit ac82b28.
b93d1e3
@gauteh gauteh added a commit to gauteh/sup that referenced this pull request Apr 20, 2013
@gauteh gauteh Get the relevant stuff from issue #16.
From ac82b28.
d380c03
@gauteh gauteh closed this Apr 20, 2013
@foobacca foobacca added a commit to foobacca/sup that referenced this pull request Apr 28, 2013
@foobacca foobacca Merge remote-tracking branch 'origin/develop' into develop
* origin/develop: (21 commits)
  set email on gem to sup-talk
  Explicitly require 'rbconfig' before using it
  s/Config/RbConfig for less warnings
  Allow builds on 2.0 fail for now
  Merge and close #24: Remove old and abandoned Ditz bug system and bugs
  sup-config: call to_s() on user input
  Merge #16: Fix YAML regression in 1.9
  Added simple auto-completion to search prompt
  Fix problem with account selector
  upd webpage to supmua.org
  Update name and adresses to our fork
  Fix updating contact aliases. Fixes an UndefinedMethodError. I lost one too many emails.
  add ruby 2.0.0 to rvm
  Merge #15: Integrate Travis
  Fix #12: Require 'iconv' before patching it
  Make the tests pass again
  Fix #10: Use a maintained fork of xapian-full
  Fix #9: Convert use of Rake::GemPackageTask to Gem::PackageTask
  Fix #5: Require relative that works on both ruby 1.8 and 1.9
  Added new hook 'gpg-expand-keys'
  ...
5a3b194
@5long 5long referenced this pull request Apr 29, 2013
Merged

Don't set yamler for 1.8 #35

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