-
-
Notifications
You must be signed in to change notification settings - Fork 925
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
[Truffle] Add bundler workarounds files, jt test bundle command #4230
Conversation
final List<String> commandLine = new ArrayList<>(1 + args.length); | ||
if (RubyGuards.isRubyArray(command)) { | ||
final Object[] store = (Object[]) getStore((DynamicObject) command); | ||
commandLine.add(store[0].toString()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why only the first element is considered here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure how to implement the argv0
yet which is mentioned in the documentation:
second argument is used as the argv[0] value, which may show up in process listings.
exec([cmdname, argv0], arg1, ...)
https://ruby-doc.org/core-2.3.1/Kernel.html#method-i-exec
Only implementing the cmdname was enough to get bundle
working for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, please mention the exec([cmdname, argv0], arg1, ...)
there to clarify the meaning.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added some comments to exec
for argv0
gem_url = gem[:url] | ||
name = gem_url.split('/')[-1] | ||
require 'tmpdir' | ||
temp_dir = Dir.mktmpdir(name) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a block form for Dir.mktmpdir
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the block form, the temp directory is deleted at the end of the block. For testing, I want to frequently disable this by commenting out # FileUtils.remove_entry temp_dir
to inspect the results. Also, in the future, I probably want to change this to not delete the temp directory whenever the test fails.
@@ -0,0 +1,516 @@ | |||
$quiet_workaround = false | |||
|
|||
workaround_header = <<~HEREDOC |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Squiggly heredoc is a very new feature, maybe it's better to stick to <<-
here, since anyway the indentation is not needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've update these to <<-
5ede6bf
to
3604fc3
Compare
I also updated backtick input stream reading because I had issues with:
|
IO.binwrite("#{local_temp_dir}/gzipped_versions", content) | ||
|
||
content = Zlib::GzipReader.new(StringIO.new(content)).read | ||
#content = `gunzip -c #{local_temp_dir}/gzipped_versions` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it suppose to be commented?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, i've uncommented/commented this
HEREDOC | ||
puts verify_none unless $quiet_workaround | ||
module SSL | ||
VERIFY_NONE = 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you give those constants different values? Otherwise it might have unintended side effects.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've updated this to VERIFY_NONE = 0
#run({'GEM_HOME' => gem_home}, *["-rbundler-workarounds", "-S", "bundle", "exec", "rake"]) | ||
end | ||
ensure | ||
FileUtils.remove_entry temp_dir |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is misaligned.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix this alignment
3604fc3
to
2ea4d0a
Compare
end | ||
run({'GEM_HOME' => gem_home}, *["-rbundler-workarounds", "-S", "gem", "install", "bundler", "-v","1.12.5"]) | ||
run({'GEM_HOME' => gem_home}, *["-rbundler-workarounds", "-S", "bundle", "install"]) | ||
# Need to workaround ruby_install name `jruby-truffle` in the gems binstubs (command can't be found ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't it a full path based on bindir + ruby_install_name?
There is bin/jruby-truffle
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I get the following error which I think is caused by the gem bin file (rake) having the header #!/usr/bin/env jruby-truffle
rbenv: jruby-truffle: command not found
The `jruby-truffle' command exists in these Ruby versions:
jruby-local
I'm not sure how to fix this yet.
Let's get this merged! |
========================================== | ||
Workaround: Change =~ to == for resolver#search_for | ||
Error: type mismatch: String given | ||
stdlib/rubygems/resolver.rb:237:in `block in search_for' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How does it work in MRI?
MRI also gives an error for String =~ String.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't fully figure out how this works but I think it originates from Gem::Platform
in rubygems/platform
.
It looks like Gem::Platform.local
can return a String
or Gem::Platform
in some cases. If it returned a Gem::Platform
then it has it's own Gem::Platform#=~
implementation to use.
So far this works for
bundle install
andbundle exec rake
for the gem mentioned in the changes:hike
. I'm still working on getting this working with an example gem that has a native build.Please let me know if you have any feedback.