samvera-deprecated / jettywrapper

Convenience tasks and classes for automated testing of hydra heads.
Apache License 2.0
7 stars 19 forks source link

jettywrapper gem causes RubyMine/Intellij line debugging to fail if rake gem is not loaded #53

Closed da70 closed 8 years ago

da70 commented 8 years ago

This is a bug report that covers a perhaps unusual spread of cases. I first encountered it when attempting to line debug an rspec test in a Blacklight project using Intellij (aka RubyMine). The version of jettywrapper being used is 1.8.3, but the 2.0.0 version has the same issue.

I had created the Run/Debug Configuration, which was simply the running of a spec script. The script failed with this error:

.../gems/jettywrapper-1.8.3/lib/tasks/jettywrapper.rake:4:in <top (required)>': undefined methodnamespace' for main:Object (NoMethodError)

I investigated, and found in jettywrapper.rb:

Dir[File.expand_path(File.join(File.dirname(__FILE__),"tasks/*.rake"))].each { |ext| load ext } if (defined?(Rake))

This code was attempting to load the jettywrapper rake tasks even though the rake gem was not loaded. This turned out to be due to JetBrains code for TeamCity integration, which monkey patches the Rake module. Example on Mac OS X:

In file ~/Library/Application Support/IntelliJIdea12016.1/ruby/rb/testing/patch/common/teamcity/utils/logger_util.rb:

...
module Rake
  module TeamCity
    module Utils
      class FileLogger
...

...and there are many other files that apply their own patches as well.

The problem is that if the rake gem is not actually loaded, the JetBrains code will cause the Rake constant to be defined regardless, and thus jettywrapper will attempt to load its rake tasks, which will of course error out because the core rake functionality is absent.

RubyMine/Intellij is a fairly popular development environment, and its line debugging is one of its strongest features. Aside from this IDE-related issue, though, it would be more robust to test for defined?(Rake::Version) instead of just defined?(Rake), as other libraries/gems might also be monkey patching rake.

I will submit a pull request right after creating this ticket. I haven't included a test because for my case it's an Intellij line debugging issue and that would be a difficult situation to simulate. There are probably ways to package a spec though, merely by adding fixture that includes a monkey patch of rake and then verifying that the jettywrapper rake tasks were not loaded. If this is considered absolutely necessary I can attempt it.

However, I think this issue and the related fix is straightforward enough that no test is needed, and I believe the switch to defined?(Rake::Version) is a reasonably stable and more conceptually sound duck-typing approach to dependency testing.

da70 commented 8 years ago

Just wondering if you know when you'll be updating the rubygems.org gem version, so as to get this fix?

mjgiarlo commented 8 years ago

I'll ping the Hydra slack channel and see if anyone has cycles to cut a release.

da70 commented 8 years ago

Actually, I've discovered that our Hydra version keeps us from upgrading jettywrapper beyond ~> 1.8.2:

$ bundle update jettywrapper
Fetching gem metadata from https://rubygems.org/............
Fetching version metadata from https://rubygems.org/...
Fetching dependency metadata from https://rubygems.org/..
Resolving dependencies.......
Bundler could not find compatible versions for gem "jettywrapper":
  In Gemfile:
    jettywrapper (~> 2.0.3)

    hydra (~> 7.1.0) was resolved to 7.1.1, which depends on
      jettywrapper (~> 1.8.2)

    hydra (~> 7.1.0) was resolved to 7.1.1, which depends on
      hydra-head (~> 7.2.0) was resolved to 7.2.2, which depends on
        hydra-core (= 7.2.2) was resolved to 7.2.2, which depends on
          jettywrapper (>= 1.4.1)

So even if a new release is made, we wouldn't be able to get it into our project without upgrading our hydra. Is there any way the fix could be backported to 1.8.3 or a newer 1.8.x version to get it through ~> 1.8.2?

jcoyne commented 8 years ago

@da70 you can remove the hydra gem from your gemfile and instead pin to the version of hydra-head that you want to use.

da70 commented 8 years ago

@jcoyne, unfortunately we can't upgrade Hydra right now but we definitely plan to at some point.

If jettywrapper 1.8.3 is no longer under active development or support we can fork for the time being. I just checked the LICENSE and it seems that forking is fine once we fulfill the requisite conditions?

jcoyne commented 8 years ago

@da70 I'm not advocating for you to upgrade hydra (although you should), just saying that you do not need the hydra gem. It just imposes a strict set of dependencies that is currently hobbling you. You don't need it.