install path of gem executables; bad gem home warning #60

pitr-ch · 2017-02-11T17:09:08Z

connect #59

eregon · 2017-02-14T11:05:24Z

.gitignore

-!lib/bin/rdoc
-!lib/bin/ri
+# Bin directory
+bin/**


Just * should be enough

bjfish · 2017-02-17T02:54:19Z

truffleruby/src/main/ruby/core/rbconfig.rb

@@ -85,7 +85,7 @@ module RbConfig

  if ruby_home
    libdir = "#{ruby_home}/lib"
-    bindir = "#{libdir}/bin"
+    bindir = "#{ruby_home}/bin"


The current bindir is causing me issues when testing gems, so I hope this line change goes in soon.

eregon · 2017-02-17T10:11:53Z

README.md

@@ -84,9 +84,11 @@ which can be used to get it working.
 The best way to get started with TruffleRuby is via the GraalVM, which includes
 compatible versions of everything you need as well as TruffleRuby.

-http://www.oracle.com/technetwork/oracle-labs/program-languages/
+<http://www.oracle.com/technetwork/oracle-labs/program-languages/>


Why the < >? It seems to make link without as well.

Ha, I've confused the syntax with some other markup.

eregon · 2017-02-17T10:12:19Z

README.md


 Inside the GraalVM is a `bin/ruby` command that runs TruffleRuby.
+See [`doc/user/using-graalvm`](https://github.com/graalvm/truffleruby/tree/master/doc/user/using-graalvm.md)


See [Using GraalVM](doc/user/using-graalvm.md)

eregon · 2017-02-17T10:13:59Z

doc/user/installing-gems.md

-unset GEM_HOME GEM_PATH GEM_ROOT
-```
+> **Note:** Ensure you have ruby managers configured properly, see 
+[`doc/user/ruby-managers`](https://github.com/graalvm/truffleruby/blob/master/doc/user/ruby-managers.md).


This should not be a note, it's a critical step.

if the user is following the steps, he has visited the page already and configured it, therefore it was kept as note only. I don't mind either way, changing it.

Users should not need to navigate too much to do one thing. It's also always best to be able to point to a single page and have clear prerequisites.

eregon · 2017-02-17T10:15:43Z

doc/user/installing-gems.md


-    ruby -rbundler-workarounds -S gem install bundler -v 1.13.7
+    ruby -r bundler-workarounds -S gem install bundler -v 1.13.7


I like the one argument -rlibrary, it's more convenient to navigate between words at the command line and I believe is the most comment usage over -r library.

It's only simple to write, for reading I find it terrible, the word is joined with the option and there is no space for eyes to find the start of the value. I'll keep spaces.

But it's not how users write it, so that's confusing.

That's an assumption though, I for example write space even on command line. Since this is a documentation, the more readable option with space separation is a better option then a shortcut.

eregon · 2017-02-17T10:16:57Z

doc/user/installing-gems.md


 `bundle exec` does not need the `bundler-workarounds` loaded. 

    ruby -S bundle exec bin/rails server
+
+Next step: [Playing Optcarrot](https://github.com/graalvm/truffleruby/blob/master/doc/user/optcarrot.md),


How is this related to Installing Gems?

The next step is not meant to imply relation but rather what to read next.

These docs should be each about one thing, not reading pages ;)

eregon · 2017-02-17T10:17:38Z

doc/user/optcarrot.md

@@ -0,0 +1,13 @@
+# Playing optcarrot
+
+Clone optcarrot repository.


Clone +the+

eregon · 2017-02-17T10:17:45Z

doc/user/optcarrot.md

+    git clone https://github.com/mame/optcarrot.git
+    cd optcarrot
+
+You will need mplayer to be installed, then you can play Lan Master 


eregon · 2017-02-17T10:18:27Z

doc/user/optcarrot.md

+You will need mplayer to be installed, then you can play Lan Master 
+with following command.
+
+    path/to/graalvm-0.20/bin/ruby bin/optcarrot --video mplayer --input term examples/Lan_Master.nes


Did you test it? I never tried with the upstream mame/optcarrot.
There might be issues for input.

ah, tested but I forgot I am on a branch, I'll have a look

eregon · 2017-02-17T10:19:22Z

doc/user/ruby-managers.md

+sure that the manager does not set environment variables `GEM_HOME`, `GEM_PATH`,
+and `GEM_ROOT`. The variables are picked up by truffleruby (as any other Ruby 
+implementation would do) causing truffleruby to pickup wrong gem-home directory 
+instead of it's own.


+the+ wrong
its own

eregon · 2017-02-17T10:20:10Z

doc/user/ruby-managers.md

+
+If you are using a Ruby manager like `rvm`, `rbnev`, or `chruby` make 
+sure that the manager does not set environment variables `GEM_HOME`, `GEM_PATH`,
+and `GEM_ROOT`. The variables are picked up by truffleruby (as any other Ruby 


By RubyGems actually but it might easier to simplify as you did.

yeah, every ruby has rubygems by default ...

eregon · 2017-02-17T10:21:44Z

doc/user/ruby-managers.md

+It will be easier when TruffleRuby can be added to ruby managers, we are working 
+on it.  
+
+Next step: [Installing gems](https://github.com/graalvm/truffleruby/blob/master/doc/user/isntalling-gems.md).


All links can be relative, GitHub handles it nicely and it means it works on branches (test it with the markdown preview in the PR).
BTW, typo in the link here.

eregon · 2017-02-17T10:24:22Z

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

@@ -16,6 +16,27 @@
    require 'rubygems'
  rescue LoadError
  else
+    unless Gem.dir.include?(Truffle::Boot.ruby_home)


I think we should we check the whole Gem.path.
Otherwise RubyGems might still load gems from another Ruby.

eregon · 2017-02-17T10:33:47Z

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

+      bad_gem_home ||= Gem.dir.include?('rvm/gems') && !Gem.dir.include?('rvm/gems/truffleruby')
+
+      # chruby stores gem in ~/.gem/<ruby>/<version>
+      bad_gem_home ||= Gem.dir.include?('.gem') && !Gem.dir.include?('.gems/truffleruby')


Seeing the logic here, how about:

our_gem_home = Gem.default_dir bad_gem_home = dir != our_gem_home && !dir.include?('/truffleruby/')

Could be, but I wanted to be more precise. I would like the warning not to live for long anyway so we can revisit if it is not removed soon-ish.

The check is not precise though:

I think we should we check the whole Gem.path.
Otherwise RubyGems might still load gems from another Ruby.

i.e. I think my check is more precise and simpler, so please use it.

The condition you are suggesting will catch any GEM_HOME change, the old one catches only different GEM_HOME pointing to a gem-home of another ruby in a ruby manager. That is the missing preciseness in the new condition I am referring too. Gem path can be anything, it cannot be easily checked that it's unintentional change and that the path belongs to other ruby. Anyway we need to only detect the common case of misconfigured a ruby manager. We ignore other changes. This will be deleted when truffleruby can be added to ruby managers.

Checking Gem.path will show you the warning is always there,
as $HOME/.gem/ruby/2.3.0 is always added by RubyGems, unless GEM_PATH is set
And that's an actual problem as it looks through system Ruby gems if there is a system ruby of version 2.3.x or some other Ruby used that default gem home to install gem (for instance chruby would use that for MRI 2.3.x).
(For instance, bad extensions, conflicting gem versions, etc)
But probably it is less problematic or likely than a bad value for GEM_HOME, so let's merge for now.
The right fix for this default system Ruby path is changing RUBY_ENGINE.

Yeah agreed, I am already working on the RUBY_ENGINE fix. Looks promising it should not take too long.

eregon · 2017-02-17T17:07:41Z

doc/user/compatibility.md

@@ -112,7 +112,7 @@ provide the same interface as JRuby does for this functionality.

 Calling Ruby code from Java is supported by the `PolyglotEngine` API of Truffle.

-http://lafo.ssw.uni-linz.ac.at/javadoc/truffle/latest/com/oracle/truffle/api/vm/PolyglotEngine.html
+<http://lafo.ssw.uni-linz.ac.at/javadoc/truffle/latest/com/oracle/truffle/api/vm/PolyglotEngine.html>


eregon · 2017-02-17T17:08:10Z

doc/user/installing-gems.md

@@ -1,37 +1,29 @@
 # Installing gems

-As mentioned in the 
-[`README`](https://github.com/graalvm/truffleruby/tree/truffle-head/README.md) 
+As mentioned in the [README](README.md) 


broken link, should be ../../README.md

eregon · 2017-02-17T17:09:54Z

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

+      bad_gem_home ||= Gem.dir.include?('rvm/gems') && !Gem.dir.include?('rvm/gems/truffleruby')
+
+      # chruby stores gem in ~/.gem/<ruby>/<version>
+      bad_gem_home ||= Gem.dir.include?('.gem') && !Gem.dir.include?('.gems/truffleruby')


The check is not precise though:

I think we should we check the whole Gem.path.
Otherwise RubyGems might still load gems from another Ruby.

* dedicated ruby managers page * links to next step * rails compatibility section * add optcarrot example

…to master * commit 'b238d6939a555f31049278a70183e3edc70f2db0': Attempt to find the original file to require if a patch is loaded for a gem that hasn't been registered with the patching system.

Run minitest in CI Merge-Requested-By: chrisseaton Merge-Queue-Digest: 1dbe6365c5da7312dffc2ac8a1de063689089547368b3ef929c10b6a5cbb6926

pitr-ch mentioned this pull request Feb 11, 2017

Improve user experience #59

Closed

7 tasks

eregon reviewed Feb 14, 2017

View reviewed changes

.gitignore Outdated

!lib/bin/rdoc

!lib/bin/ri

# Bin directory

bin/**

Copy link

Member

eregon Feb 14, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just * should be enough

bjfish reviewed Feb 17, 2017

View reviewed changes

eregon reviewed Feb 17, 2017

View reviewed changes

eregon requested changes Feb 17, 2017

View reviewed changes

pitr-ch added 4 commits February 17, 2017 17:40

Install gem executables into bin instead of lib/bin

1f5d573

Detect and warn about bad Gem.dir

f2b48db

Remove unnecessary local variables

e8ff35d

Replace JRuby with TruffleRuby in a log message

87cddd3

eregon requested changes Feb 17, 2017

View reviewed changes

Documentation improvements

e4c6d71

* dedicated ruby managers page * links to next step * rails compatibility section * add optcarrot example

eregon approved these changes Feb 17, 2017

View reviewed changes

pitr-ch merged commit 4585c0e into oracle:master Feb 17, 2017

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

Merge pull request oracle#60 from Shopify/minitest

eb46d6a

Run minitest in CI Merge-Requested-By: chrisseaton Merge-Queue-Digest: 1dbe6365c5da7312dffc2ac8a1de063689089547368b3ef929c10b6a5cbb6926


		Inside the GraalVM is a `bin/ruby` command that runs TruffleRuby.
		See [`doc/user/using-graalvm`](https://github.com/graalvm/truffleruby/tree/master/doc/user/using-graalvm.md)


		ruby -rbundler-workarounds -S gem install bundler -v 1.13.7
		ruby -r bundler-workarounds -S gem install bundler -v 1.13.7

		@@ -0,0 +1,13 @@
		# Playing optcarrot

		Clone optcarrot repository.

install path of gem executables; bad gem home warning #60

install path of gem executables; bad gem home warning #60

Conversation

pitr-ch commented Feb 11, 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

eregon Feb 17, 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

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 17, 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

pitr-ch commented Feb 11, 2017 •

edited

Loading

eregon Feb 17, 2017 •

edited

Loading

eregon Feb 17, 2017 •

edited

Loading