Set RUBY_ENGINE to truffleruby and fix related libs #46

eregon · 2017-02-01T13:59:26Z

No description provided.

eregon · 2017-02-01T14:01:13Z

truffleruby/src/main/java/org/truffleruby/RubyLanguage.java

@@ -51,8 +51,7 @@
    public static final String RUBY_VERSION = "2.3.1";
    public static final int    RUBY_REVISION = 0;
    public static final String COMPILE_DATE = "2017";
-    // TODO (pitr-ch 11-Jan-2017): use our value when we are recognized by rubygems


Specific workarounds are better than waiting for a new RubyGems version (but we should fix upstream as well obviously).
We can also patch our own Rubygems of course.

bjfish · 2017-02-01T14:44:45Z

There may be some failures due to updating this. We'll probably need to shim/update in some places. This was one usage that came to mind:
https://github.com/bundler/bundler/blob/b3cb9516aaf83cf865c9ed76c52ec4d0d182015f/spec/support/platforms.rb#L66

eregon · 2017-02-01T14:50:45Z

@bjfish This is in Bundler specs, right?
It seems fine to define BUNDLER_SPEC_RUBY_ENGINE_VERSION=1.0 or so for running Bundler specs (and fix upstream with Truffle::VERSION).

bjfish · 2017-02-01T14:54:29Z

I think there's similar usage in the lib somewhere. Maybe here: https://github.com/bundler/bundler/blob/121ada2a1842ebe9989f35567e2dd829ec06b7a1/lib/bundler/ruby_version.rb#L108

pitr-ch · 2017-02-01T20:52:01Z

@bjfish please have a look at my commit introducing the change form jruby, it'll help to track down what has to be fixed

eregon · 2017-02-02T12:44:22Z

@pitr-ch Which commit it that?

pitr-ch · 2017-02-02T16:37:36Z

@eregon git blame of line #46 (diff)

eregon · 2017-02-02T19:36:27Z

83c0f24 is the commit

eregon · 2017-02-07T17:53:46Z

@pitr-ch @bjfish I fixed a few failures but the remaining ones seem related to RubyGems/Bundler.
I did not find an easy solution.
Could you take a look and continue this PR as you are more knowledgeable in this area?
The last commit (4a715d7) was just an experiment, feel free to get rid of it.

pitr-ch · 2017-02-07T22:12:23Z

It looks like bundler is not even resolving the tested gem as a dependency, therefore it is not put in $LOAD_PATH. This is why I went for the "ruby" originally to avoid untangling bundler complexities ;) until it's fixed properly without hacks. I rather not spend time on this. I think we all agree that RUBY_ENGINE should be 'truffleruby' but lets wait until bundler recognises truffleruby.

eregon · 2017-02-07T22:26:42Z

until bundler recognises truffleruby

What does it mean? We will need to find out how to make it work anyway and make a PR to Bundler. Maybe we can ask some help from the Bundler team?

pitr-ch · 2017-02-08T10:39:35Z

@eregon exactly, I'll update bundler and submit a PR. I would like to avoid hacking it in the meantime, especially if RUBY_ENGINE='ruby' works fine.

pitr-ch · 2017-02-18T20:20:39Z

fix #46

eregon · 2017-02-19T13:23:08Z

truffleruby/src/main/ruby/post-boot/post-boot.rb

@@ -38,6 +38,8 @@
      end
    end

+    require 'truffle/patching'


Isn't it too late? What if we want to patch something in rubygems?

Most of the rubygem files is loaded lazily, so it works. If we need to patch any of the eagerly loaded files it can be done manually here. I rather expect we'll be removing the patches rather than adding new ones, it should not be a problem.

eregon · 2017-02-19T13:25:38Z

truffleruby/src/main/java/org/truffleruby/core/CoreLibrary.java

@@ -789,6 +789,7 @@ private void initializeConstants() {
        Layouts.MODULE.getFields(rubiniusFFIModule).setConstant(context, node, "TYPE_VARARGS", RubiniusTypes.TYPE_VARARGS);

        Layouts.MODULE.getFields(objectClass).setConstant(context, node, "RUBY_VERSION", frozenUSASCIIString(RubyLanguage.RUBY_VERSION));
+        Layouts.MODULE.getFields(objectClass).setConstant(context, node, "TRUFFLERUBY_VERSION", frozenUSASCIIString(RubyLanguage.getTruffleRubyVersion()));


Should it be TRUFFLERUBY_VERSION (like JRUBY_VERSION)
or Truffle::VERSION (like Rubinius::VERSION) ?

Both are fine to me, the advantage of TRUFFLERUBY_VERSION is that's easier to check, even if defined? is buggy (as we used to have) and it's not the "Truffle version" but the "TruffleRuby version".

Since there is already RUBY_VERSION, JRUBY_VERSION I would follow this pattern.

eregon · 2017-02-19T13:26:08Z

lib/truffleruby-tool/lib/truffle_tool.rb

@@ -687,7 +686,9 @@ def subcommand_run(rest)
                  {}).
        merge(@options[:run][:offline] ?
                  { 'GEM_HOME' => @options[:run][:offline_gem_path].to_s,
-                    'GEM_PATH' => @options[:run][:offline_gem_path].to_s } :
+                    'GEM_PATH' => @options[:run][:offline_gem_path].to_s,
+                    'PATH'     => [p(@options[:run][:offline_gem_path].join('bin').to_s), ENV['PATH']].join(':')


Debug p in here.

eregon · 2017-02-19T13:28:02Z

lib/truffle/truffle/patching.rb

@@ -0,0 +1,38 @@
+module Truffle::Patching
+  TRUFFLE_PATCHES_DIRECTORY = File.join(Truffle::Boot.ruby_home, 'lib', 'patches')
+  TRUFFLE_PATCHES           = Dir.glob(File.join(TRUFFLE_PATCHES_DIRECTORY, '**', '*.rb')).


Please avoid File.join, it just adds clutter, and in the case of Dir.glob is even potentially incorrect (if File::SEPARATOR is changed). Filesystem pathnames in Ruby are always represented with /.

eregon · 2017-02-19T13:32:39Z

lib/truffle/truffle/patching.rb

+  TRUFFLE_PATCHES           = Dir.glob(File.join(TRUFFLE_PATCHES_DIRECTORY, '**', '*.rb')).
+      select { |path| File.file? path }.
+      map { |path| path[(TRUFFLE_PATCHES_DIRECTORY.size+1)..-4] }.
+      reduce ({}) { |h, v| h.update v => true }


I find the pure functional approach here more verbose and also much less efficient.
I propose:

TRUFFLE_PATCHES = {} Dir.glob("#{TRUFFLE_PATCHES_DIRECTORY}/**/*.rb") do |path| if File.file? path feature = path[(TRUFFLE_PATCHES_DIRECTORY.size+1)...-3] TRUFFLE_PATCHES[feature] = true end end

It's evaluated only once over about a dozen of files, I don't think performance is relevant in this particular case. As for functional vs. imperative style I think this is matter of taste, I find functional clearer, more readable. Since I think it's only a matter of personal taste in this case, I'll leave it as is.

It matters because this runs during startup, isn't it?
It creates 3 extra Arrays, plus 2*n (number of patch files) Hashes, not exactly optimal.

Array#to_h might be a good compromise here, with [path[...], true] in the map block.

It does run on startup, but 3 arrays of about ten elements, that's insignificant compared to everything else. The code will also be eventually removed anyway. It's a single hash though, update does not create new instances or did you mean something else?

Ah, Hash#update is Hash#merge! (don't use update, it's an uncommon alias)
v => true creates a Hash per iteration.
Measure it if you want.
But anyway I think reduce/inject + modifying inplace the accumulator is an anti-pattern that just adds complexity with extra variables and caring of the return value, for no gain.

to_h is best option

eregon · 2017-02-19T13:40:06Z

lib/patches/bundler/ruby_version.rb

+                            else
+                              # not defined in ruby 1.8.7
+                              "ruby"
+                            end


This is very weirdly indented, I would rather keep the same indentation as the source to ease comparison.

(on a side note, I personally find no value to aligning assignments)

yeah, auto-formatted by mistake.

(I got used to them only because I prefer aligned hash's keys and values and there is only one option for both in idea and I won't go against auto-formatter.)

eregon · 2017-02-19T13:42:07Z

lib/patches/bundler/ruby_version.rb

+                              Rubinius::VERSION.dup
+                            when "jruby"
+                              JRUBY_VERSION.dup
+                            when 'truffleruby' # added case branch


It would be good to use a common prefix to mean "comment added for TruffleRuby support".
We used to have # Truffle: my comment which I think is easy to spot.
I think we should be consistent.

yeah, I'll use # TruffleRuby:, and update the comments.

They are quite a few # Truffle:, I think it's best to update them all in a different PR.

Sorry already pushed here.

eregon · 2017-02-19T13:44:08Z

lib/patches/bundler/dependency.rb

+# add record for truffleruby
+Bundler::Dependency.send :const_set,
+                         :PLATFORM_MAP,
+                         old_value.merge(truffleruby: Gem::Platform::RUBY)


I prefer just:

Bundler::Dependency::PLATFORM_MAP[:truffleruby] = Gem::Platform::RUBY

It is much easier to see what's happening.

The hash is frozen, therefore it has to be replaced.

Unfortunate, maybe something like this would simplify a little:

class Bundler::Depedency const_set PLATFORM_MAP, PLATFORM_MAP.merge(truffleruby: Gem::Platform::RUBY) end

Yeah, maybe, it's just a patch though.

Might as well make it readable.

eregon · 2017-02-19T13:44:59Z

lib/patches/bundler/dependency.rb

+  end
+
+  reverse_platform_map.each { |_, platforms| platforms.freeze }
+end.freeze)


Same here, please use a clearer imperative approach with less nesting.

This is a copy of original code, keeping it.

eregon · 2017-02-19T13:49:01Z

lib/truffle/truffle/patching.rb

@@ -0,0 +1,38 @@
+module Truffle::Patching


This approach is quite different than what I had in mind in #68 (comment). Your approach could also use that mechanism, but it is simpler and sufficient for now, so let's keep it simple 👍
Could you add a lib/patches/README.md to document briefly what it is for and what it does?

…gine version

* introduce patching system, loads a file from patches directory after a file with same is required * replace bundler-workarounds with patches * make bundler recognize truffleruby * remove few unnecessary fixes * C extensions are not built by default, TRUFFLERUBY_CEXT_ENABLED can be set to enable

* commit '0f3ffd2ecabba9de24ba8aaae5351755fcb42800': Change the imagename in the GraalVM property file.

Typo Merge-Requested-By: XrXr Merge-Queue-Emergency-Bypass: True

eregon mentioned this pull request Feb 1, 2017

RUBY_ENGINE should be "truffleruby" #45

Closed

eregon commented Feb 1, 2017

View reviewed changes

eregon force-pushed the fix_ruby_engine branch from 421e2d1 to 4a715d7 Compare February 3, 2017 20:53

pitr-ch changed the title ~~Set RUBY_ENGINE to truffleruby~~ Set RUBY_ENGINE to truffleruby and fix related libs Feb 18, 2017

pitr-ch force-pushed the fix_ruby_engine branch 3 times, most recently from 0f7c6d1 to ead57a4 Compare February 18, 2017 22:42

pitr-ch mentioned this pull request Feb 18, 2017

Automatically require a specific workaround when a lib is seen #68

Closed

eregon commented Feb 19, 2017

View reviewed changes

pitr-ch force-pushed the fix_ruby_engine branch 4 times, most recently from 1c74877 to bbbfb5b Compare February 19, 2017 19:34

pitr-ch mentioned this pull request Feb 20, 2017

Improve user experience #59

Closed

7 tasks

eregon added 4 commits February 20, 2017 17:02

Set RUBY_ENGINE to truffleruby

610150c

Fix detection of truffleruby now that its RUBY_ENGINE is "truffleruby"

6cfb0e3

Fix RUBY_ENGINE check in jt.rb

c44e04c

Workaround Bundler::RubyVersion.system trying to find out the ruby en…

57abc39

…gine version

eregon and others added 4 commits February 20, 2017 17:02

Try stubbing Gem extensions to always be empty

8efc6de

Fix sinatra-server test

e4af30b

Add log message

f3f85c2

pitr-ch force-pushed the fix_ruby_engine branch from bbbfb5b to f3f85c2 Compare February 20, 2017 16:59

pitr-ch merged commit de7772e into master Feb 20, 2017

eregon deleted the fix_ruby_engine branch February 20, 2017 18:28

chrisseaton added a commit that referenced this pull request May 30, 2018

Merge pull request #46 in G/truffleruby from topic/GR-9653 to master

8a77fe7

* commit '0f3ffd2ecabba9de24ba8aaae5351755fcb42800': Change the imagename in the GraalVM property file.

chrisseaton pushed a commit to Shopify/truffleruby that referenced this pull request Nov 13, 2019

Merge pull request oracle#46 from Shopify/aw/typo

fb56b04

Typo Merge-Requested-By: XrXr Merge-Queue-Emergency-Bypass: True

Set RUBY_ENGINE to truffleruby and fix related libs #46

Set RUBY_ENGINE to truffleruby and fix related libs #46

Conversation

eregon commented Feb 1, 2017

Choose a reason for hiding this comment

bjfish commented Feb 1, 2017

eregon commented Feb 1, 2017

bjfish commented Feb 1, 2017

pitr-ch commented Feb 1, 2017

eregon commented Feb 2, 2017

pitr-ch commented Feb 2, 2017 • edited Loading

eregon commented Feb 2, 2017

eregon commented Feb 7, 2017

pitr-ch commented Feb 7, 2017

eregon commented Feb 7, 2017

pitr-ch commented Feb 8, 2017

pitr-ch commented Feb 18, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eregon Feb 19, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eregon Feb 19, 2017 • edited Loading

Choose a reason for hiding this comment

pitr-ch commented Feb 2, 2017 •

edited

Loading

eregon Feb 19, 2017 •

edited

Loading

eregon Feb 19, 2017 •

edited

Loading