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

Fix various JRuby failures #422

Merged
merged 34 commits into from May 1, 2014

Conversation

Projects
None yet
3 participants
@ddfreyne
Copy link
Member

commented Apr 14, 2014

This improves compatibility with nanoc on JRuby (and, to a lesser extent, Ruby 1.8.x).

Some changes:

  • JRuby tests are run twice: once with Nokogiri enabled, and once with Nokogiri disabled. JRuby-Nokogiri has several problems (most of them being worked on), but I’d much rather split up the tests while waiting for Nokogiri to be fixed.
  • A warning message is printed when using Nokogiri on JRuby. See above for the reasoning why.
  • JRuby with 1.8 mode has been dropped. If you already have JRuby, why would you want to run it in 1.8 mode?
  • The Gemfile now more clearly defines supported platforms for dependencies. It has been modified to accomodate both JRuby and Ruby 1.8.x.
  • Temp path creation is more reliable on JRuby (JRuby seems to delete empty temp directories, which is sensible, but breaks an assumption made by nanoc).

@ddfreyne ddfreyne changed the title Fix various JRuby failures [WIP] Fix various JRuby failures Apr 14, 2014

@ddfreyne

This comment has been minimized.

Copy link
Member Author

commented Apr 14, 2014

I reported a bunch of issues for Nokogiri on JRuby, that came to light in the nanoc test cases:

@ddfreyne

This comment has been minimized.

Copy link
Member Author

commented Apr 20, 2014

Skip Nokogiri tests on JRuby
Nokogiri on JRuby is unusably buggy. For details, see #422.
@ddfreyne

This comment has been minimized.

Copy link
Member Author

commented Apr 20, 2014

I’ve decided to skip all Nokogiri tests on JRuby, because that library is insanely buggy and I just don’t want to deal with it anymore.

I’m considering printing a warning whenever Nokogiri is used on JRuby. @bobthecow Thoughts?

@ddfreyne

This comment has been minimized.

Copy link
Member Author

commented Apr 20, 2014

The following is now printed when using Nokogiri on JRuby:

--------------------------------------------------------------------------------
Caution:

Nokogiri on JRuby has severe bugs that prevent it from producing correct results
in many cases. For example, the nanoc test cases revealed the following bugs:

- http://github.com/sparklemotion/nokogiri/issues/1077
- http://github.com/sparklemotion/nokogiri/issues/1078
- http://github.com/sparklemotion/nokogiri/issues/1079
- http://github.com/sparklemotion/nokogiri/issues/1080
- http://github.com/sparklemotion/nokogiri/issues/1081
- http://github.com/sparklemotion/nokogiri/issues/1084

Because of these issues, we advise against using Nokogiri on JRuby. If you need
XML parsing functionality, consider not using JRuby for the time being.
--------------------------------------------------------------------------------

Given the bugginess of the library, I think it’s necessary to warn users so that they know what to expect.

Suggestions about the phrasing is quite welcome.

ddfreyne added some commits Apr 20, 2014

Use more reliable checksumming method
Marshal.dump is not consistent in its output.
Skip pruner test that checks behavior for output dir symlink
JRuby 1.7.11 has buggy File.find behavior. Also see
jruby/jruby#1647

@ddfreyne ddfreyne changed the title [WIP] Fix various JRuby failures Fix various JRuby failures Apr 20, 2014

@ddfreyne

This comment has been minimized.

Copy link
Member Author

commented Apr 20, 2014

All JRuby issues are now fixed (apart from the Nokogiri ones; those are just skipped).

CC @bobthecow

@ddfreyne ddfreyne added to review and removed waiting labels Apr 20, 2014

@bobthecow

This comment has been minimized.

Copy link
Member

commented Apr 20, 2014

I feel like leaving nokogiri tests in and marking jruby as an allowed failure is slightly better. That way it's very clear that jruby/nokogiri is still crappy until it's fixed.

@ddfreyne

This comment has been minimized.

Copy link
Member Author

commented Apr 20, 2014

@bobthecow What about running JRuby twice, once with Nokogiri enabled and once with Nokogiri disabled?

@bobthecow

This comment has been minimized.

Copy link
Member

commented Apr 20, 2014

@ddfreyne Yeah, good call.

@ddfreyne ddfreyne changed the title Fix various JRuby failures Fix various JRuby and Ruby 1.8.x failures Apr 21, 2014

ddfreyne added some commits Apr 21, 2014

Do not use #each_with_object
Ruby 1.8.x does not have #each_with_object; Ruby 1.9+ does.
Skip ordered hash test on Ruby 1.8.x.
Ruby 1.8.x does not have ordered hashes; Ruby 1.9+ does.
@ddfreyne

This comment has been minimized.

Copy link
Member Author

commented Apr 21, 2014

Ruby 1.8.x doesn’t have a way of figuring out the exit status of the child. That’s unfortunate, as it means popen3 can’t be used reliably.

This article suggests to use either the open4 functionality built into JRuby, or using the open4 gem.

@ddfreyne

This comment has been minimized.

Copy link
Member Author

commented Apr 21, 2014

The jruby-18mode Travis CI target is problematic now. It errors with the following message:

Gem::InstallError: celluloid requires Ruby version >= 1.9.2.

This makes sense, since the Gemfile contains (simplified) this:

gem 'listen', :platforms => [:ruby_19, :ruby_20, :ruby_21, :jruby]

Listen depends on celluloid, which is not compatible with 1.8.x (because Celluloid uses fibers, I believe, which are available on 1.9+ only).

The :jruby symbol here should really be something like “JRuby-but-not-1.8”.

That is, unless we decide to only support JRuby with 1.9 support, and only support 1.8.x on MRI. Thoughts?

@ddfreyne

This comment has been minimized.

Copy link
Member Author

commented Apr 21, 2014

Seems like JRuby (without Nokogiri) is passing the tests now. Sweet!

I believe the Tempfile trick I used to create a new path (rather than a file) is unreliable. I’m doing this:

      def temp_filename
        FileUtils.mkdir_p(TMP_TEXT_ITEMS_DIR)
        tempfile = Tempfile.new('', TMP_TEXT_ITEMS_DIR)
        new_filename = tempfile.path
        tempfile.close!

        File.expand_path(new_filename)
      end

That should really be replaced with something that returns a unique, new and unused filename in a directory that is guaranteed to exist. The Tempfile#close! method makes no guarantee that the directory of the temporary file won’t be deleted. (In fact, deleting the directory if it’s empty makes sense.)

So, it seems to me that exchanging the Tempfile trick with proper temp-path logic is a requirement for this PR to be merged.

Move temp path creation logic into new TempPathRegistry
Using `Tempfile` is unreliable as there is no guarantee that the
generated temporary file's directory will not be deleted after calling
`Tempfile#close!`.
@ddfreyne

This comment has been minimized.

Copy link
Member Author

commented Apr 21, 2014

Temporary path creation logic is now in a separate TempPathRegistry (WIP).

@ddfreyne

This comment has been minimized.

Copy link
Member Author

commented Apr 22, 2014

This PR is ready to be reviewed.

Open questions:

  • Should we disable jruby-18mode? (My vote: yes)
  • Should we move JRuby 1.7.11 (without Nokogiri) out of allowed failures? (My vote: yes)

There are still some Ruby 1.8.x and Rubinius issues, but I’ll tackle those in separate PRs.

@ddfreyne ddfreyne changed the title Fix various JRuby and Ruby 1.8.x failures Fix various JRuby failures Apr 22, 2014

@ddfreyne

This comment has been minimized.

Copy link
Member Author

commented Apr 22, 2014

JRuby 1.8 mode disabled and JRuby 1.7.11 removed from allowed failures after discussing with @bobthecow.

@ddfreyne ddfreyne added the to review label Apr 22, 2014

Reword Pure Java Nokogiri warning
The original message was too negative.
@ddfreyne

This comment has been minimized.

Copy link
Member Author

commented Apr 22, 2014

I’ve reworded the JRuby-Nokogiri message to be more constructive:

Note:

The behavior of Pure Java Nokogiri differs from the Nokogiri used on the
standard Ruby interpreter (MRI) due to differences in underlying libraries.

These sometimes problematic behavioral differences can cause nanoc filters not
to function properly, if at all. If you need reliable (X)HTML and XML handling
functionality, consider not using Nokogiri on JRuby for the time being.

These issues are being worked on both from the Nokogiri and the nanoc side. Keep
your Nokogiri and nanoc versions up to date!

For details, see https://github.com/nanoc/nanoc/pull/422.

@headius Apologies for letting my frustrations get the better of me.

@ddfreyne

This comment has been minimized.

Copy link
Member Author

commented Apr 22, 2014

Ready for review.

@bobthecow

This comment has been minimized.

Copy link
Member

commented Apr 22, 2014

haven't reviewed the code yet, but the warning is better.

@ddfreyne

This comment has been minimized.

Copy link
Member Author

commented Apr 23, 2014

Hm, had ~> 1.5.5 for Nokogiri in the Gemfile. I don’t quite remember what that was for, so I changed it to ~> 1.6. Maybe I should get rid of it entirely?

It doesn’t affect any of the reported issues, since those were tested outside of the nanoc context.

@@ -94,6 +94,10 @@ def test_run_with_exclude

def test_run_with_symlink_to_output_dir
skip_unless_have_symlink
if defined?(JRUBY_VERSION) && JRUBY_VERSION == '1.7.11'

This comment has been minimized.

Copy link
@bobthecow

bobthecow Apr 30, 2014

Member

should that be <= or something?

This comment has been minimized.

Copy link
@ddfreyne

ddfreyne Apr 30, 2014

Author Member

Theoretically, yes, but I only really care about the version that is currently the latest one. (Travis CI runs 1.7.11.)

@bobthecow

This comment has been minimized.

Copy link
Member

commented Apr 30, 2014

👍

ddfreyne added a commit that referenced this pull request May 1, 2014

@ddfreyne ddfreyne merged commit 14ceafc into release-3.6.x May 1, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details

@ddfreyne ddfreyne deleted the bug/fix-jruby-failures branch May 1, 2014

@ddfreyne ddfreyne removed the to review label May 1, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.