JEG2 / highline

A higher level command-line oriented interface.
Other
1.29k stars 137 forks source link

JRuby 1.7 #41

Closed mnzaki closed 12 years ago

mnzaki commented 12 years ago

I'm trying to use highline on JRuby 1.7 (jruby-head with rvm) and it fails with: NameError: cannot load Java class jline.ConsoleReader

mnzaki commented 12 years ago

JRuby moved from jline1 to jline2.7 on April 17 (jruby/jruby@bb7dc317aabcbcd0d7f812f334d7f0372e63d7df) I suppose highline is expecting jline1

JEG2 commented 12 years ago

I would love to see this fixed. Anyone who know's jline and offers up a patch is my hero.

mnzaki commented 12 years ago

I was just beginning work on a patch. Quick clarification: what's up with all the disableEcho and enableEcho for JRuby and not for other platforms? A lot of the calls (gets, readline, getbyte, etc) on $stdin/$stdout work for JRuby just as well, and I don't see why all the extra logic (and echo disable/re-enable) is needed.

mnzaki commented 12 years ago

Here's an initial patch, mnzaki/highline@721bc1e6194d7a79b19 I'm going to try to refactor a few parts then issue a pull request.

mnzaki commented 12 years ago

I have pushed more changes to the jruby-1.7 branch, which clean up the terminal code a bit. This needs more testing of course. I've tested ncurses, stty and jline.

stty and jline seem fine.

ncurses breaks the console if the process gets a SIGSTOP (ctrl-z on the terminal) and SIGCONT ("fg"). Also, after ending the process the terminal does not handle scroll events properly anymore. Usually when in secondary screen mode, the scroll events simply produce up and down keypresses, and when secondary screen mode is terminated, the scroll events are captured again. It seems like we are not closing down ncurses properly or something else is wrong.

I'd recommend giving 'jline' higher priority than 'ncurses' mode.

If you wish, you can just pull the first commit which makes minimal changes (specific to JRuby) and does not touch anything else.

JEG2 commented 12 years ago

Wow, thanks for working on this.

I agree with you assessment. Let's bump line over ncurses.

mnzaki commented 12 years ago

I need the JRuby functionality, so it's only natural that I work on it :-D

The ncurses terminal breakage is actually a JRuby bug, and an important one IMO. JRuby doesn't seem to run the 'ensure' blocks if the process gets a SIGINT, so NCurses::endwin is never called if we die suddenly. I'll be reporting this. MRI handles this correctly, and ncurses works well there (sort of; I still don't like the occasional screen blank).

I've rearranged the choices to give jline (which is much more reliable) a higher priority, and I removed the comment about needing ncurses for jruby.

BTW, all these changes will probably break highline horribly for users of jruby before 1.5 (and possible 1.6)

JEG2 commented 12 years ago

Hmm, we probably need an option for older JRuby versions. Thoughts?

mnzaki commented 12 years ago

Not sure. You could keep the changes on a different branch for a couple months then merge into master later. The JLine change in JRuby has been since mid April, that's about 2 months ago. I'm guessing there'll be considerably more people using the new JRuby in a month or two.

tomdz commented 12 years ago

You could distinguish the jruby versions using JRUBY_VERSION, and keep the jline1 code around for < 1.7.

JEG2 commented 12 years ago

I think I prefer tomdz's approach. HighLine is a general purpose library and we're going to need to support it for a while yet.

mnzaki commented 12 years ago

That sounds better. What do you think of the rest of the changes in the branch though? I'm off on vacation for a week, I might work on this when I'm back if you don't.

JEG2 commented 12 years ago

It's looking good to me. I think you're on the right track.

nirvdrum commented 12 years ago

If it helps any, I've tried @mnzaki's branch on JRuby 1.7.0-preview2 in the context of Capistrano and everything has been working great.

JEG2 commented 12 years ago

Great. So we just need to sort out the backwards compatibility issues and these changes are a go.

nirvdrum commented 12 years ago

@tomdz's solution makes sense. Otherwise, you could try to require one, rescue the exception, and then try to require the other.

JEG2 commented 12 years ago

Yeah, we just need someone to get the patch ready.

markmandel commented 12 years ago

@mnzaki - Just tested your branch as well. Worked a charm (was having exactly the same issue). Thanks for putting in the effort.

mnzaki commented 12 years ago

Apologies for the delay, but my organization has moved away from JRuby for now. I have update the branch and added back support for 1.6 I have done some basic testing under JRuby 1.6 and 1.7, and MRI 1.9. More testing is probably needed to make sure I didn't break anything.

I will create a pull request now.

JEG2 commented 12 years ago

I've merged the changes. Thank you.

Anyone else willing to double-check this stuff before I release a new gem?

ryanb commented 12 years ago

@JEG2 I had the error mentioned in this issue and using the master branch fixed it. I will be mentioning this fix in a RailsCasts episode today but it would be nice to get a release soon. Thanks!

JEG2 commented 12 years ago

I have pushed 1.6.14 with this fix. Thanks all!