Skip to content
New issue

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

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

System extensions depend on now-internalized JRuby dependencies #227

Closed
headius opened this issue Dec 9, 2017 · 14 comments
Closed

System extensions depend on now-internalized JRuby dependencies #227

headius opened this issue Dec 9, 2017 · 14 comments

Comments

@headius
Copy link

headius commented Dec 9, 2017

We received a report of highline failing to load on JRuby 9.1.15.0 in jruby/jruby#4889.

The issue is that system_extensions.rb accesss parts of the jline library we ship using its normal package name, and to avoid an embedding issue in JRuby 9.1.15.0 we now have "vendored" that library into a JRuby-specific package (jruby/jruby#4740).

Because of the relocation, highline's system extensions fail to load properly.

We can proceed in a few ways:

  • If the extensions in question are intended to make JRuby's readline work more like MRI's, they could be contributed to jruby-readline and shipped. We will then take responsibility for managing the package references.
  • If these are enhancements, they could be added as JRuby-specific methods you can call, and we would manage the package references.
  • A low-impact alternative would be for us to add a method that returns the jline package.

There's possibly another path that only require a small change to highline: use both the old path and the new path, since we are unlikely to change the new path ever again. (right, @mkristian?)

@mkristian
Copy link

mkristian commented Dec 9, 2017

@headius right it is unlikely the path will change again. but it is more likely we switch to jline-3.0 one day. and current jline-3.0 has a dependency to asm which is also something jruby puts into a relocated package.

@abinoam
Copy link
Collaborator

abinoam commented Dec 10, 2017

I let this up to you (@headius @mkristian) to decide what is best approach for JRuby, I think it will be easy to make Highline compatible with it 👍

PS: I'll probably have time to work on this by monday.

@abinoam
Copy link
Collaborator

abinoam commented Dec 10, 2017

Hi @headius,

I'm seeing now that the issue is referring to 1-7-stable branch.
These "system extensions" have gone in 2.0 (unreleased).
I'll try to have a better look at it.

@abinoam
Copy link
Collaborator

abinoam commented Dec 10, 2017

@tomdz Could you help on this? I saw the JRuby/jline code was made initially by you.

Hooking some other folks that could also probably help: @BanzaiMan, @ai, @presidentbeef, @rsutphin, @mnzaki, @iconoclast.

@mkristian
Copy link

just to get the initial context is play as it could be 1-7-branch related: jruby/jruby#4889

@presidentbeef
Copy link
Contributor

FWIW I've been using the 2.0 development releases for JRuby compatibility for a long time.

@abinoam
Copy link
Collaborator

abinoam commented Dec 11, 2017

just to get the initial context is play as it could be 1-7-branch related: jruby/jruby#4889

The stack trace shows that @petrvalkoun-gooddata is on 1.7.10.

initialize_system_extensions at C:/jruby/lib/ruby/gems/shared/gems/highline-1.7.10/lib/highline/system_extensions.rb:21

@abinoam
Copy link
Collaborator

abinoam commented Dec 11, 2017

Hi all,

I've run bundle exec rake acceptance and the readline autocomplete test worked ok on 2.0.0-pre.

I've made some simple tests on questions with readline enabled and it's woking ok on 2.0.0-pre

What about just ask @petrvalkoun-gooddata and everybody willing to keep running the most update version of JRuby to migrate his code to highline 2.0? What do you think?

By the way, I think I've postponing the 2.0 release too much. I'm waiting for feedback from people that must have their software affected by the release. But, well, I understand how hard it is to deal with legacy software. This issue afffecting JRuby is the fuel that I need to officially release 2.0.
Releasing it will push everybody that haven't set version constraints at their gemspec/Gemfiles to the most up to date HighLine version. I mean, if HighLine 2.0 was already released, perhaps this JRuby/HighLine issue wouldn't even be noticed.

Houston (@JEG2)? Agree? Release 2.0? 9, 8, 7, ...

PS:
I've had some issues trying to bundle install at 2.0.0-develop with JRuby.
Gem rugged didn't compile native extensions.
bundle install --without code_quality for the rescue. As we don't need those gems to run the tests.

@mkristian
Copy link

@abinoam cool - FYI gems with native extension do not work with JRuby. Thanks for looking into all this.

@abinoam
Copy link
Collaborator

abinoam commented Dec 11, 2017

One more good new. The gem that the issue is refereeing is https://github.com/gooddata/gooddata-ruby

Having a fast look at it, it seem it is already ready for HighLine 2.0. Just because it used "best practices". It envelopes Highline use behind a local reference. Just the way HighLine 2 favors!

https://github.com/gooddata/gooddata-ruby/blob/2bd53792728329d380d96a8358cc734359c14fd5/lib/gooddata/cli/terminal.rb#L12

require 'highline'

# Define GoodData::CLI as GLI Wrapper
module GoodData
  module CLI
    DEFAULT_TERMINAL = HighLine.new unless const_defined?(:DEFAULT_TERMINAL)

    class << self
      def terminal
        DEFAULT_TERMINAL
      end
    end
  end
end

Then it uses it like this at https://github.com/gooddata/gooddata-ruby/blob/2bd53792728329d380d96a8358cc734359c14fd5/lib/gooddata/commands/auth.rb#L45

          # Read environment
          environment = GoodData::CLI.terminal.ask('Environment') do |q|
            set_default_value(q, old_credentials[:environment], GoodData::Project::DEFAULT_ENVIRONMENT)
          end

So, it seems it will probably work out of the box with HighLine 2.0 (at least for this specific gem).

@abinoam
Copy link
Collaborator

abinoam commented Dec 11, 2017

gems with native extension do not work with JRuby.

It is a pronto dependency.

I'll see if I can fine tune the gemspec|Gemfile so it autodetects this.

Thanks 👍

@petrvalkoun-gooddata
Copy link

@abinoam many thanks for your help, I have already asked our developers to migrate to highline 2.0. I will notify the result.

@abinoam
Copy link
Collaborator

abinoam commented Dec 28, 2017

I'm closing this! 👍

@abinoam abinoam closed this as completed Dec 28, 2017
@petrvalkoun-gooddata
Copy link

Upgrade to highline 2.0 resolved the issue, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants