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

Tempfile.open does not recognize the 3rd argument (options to pass to File.open) #5456

Closed
erikogan opened this Issue Nov 19, 2018 · 5 comments

Comments

Projects
None yet
2 participants
@erikogan
Copy link

commented Nov 19, 2018

Environment

  • JRuby version: jruby 9.2.4.0 (2.5.0) 2018-11-13 8faff06 Java HotSpot(TM) 64-Bit Server VM 25.152-b16 on 1.8.0_152-b16 +jit [darwin-x86_64]
  • Operating system and platform: Darwin inquanok 17.7.0 Darwin Kernel Version 17.7.0: Wed Oct 10 23:06:14 PDT 2018; root:xnu-4570.71.13~1/RELEASE_X86_64 x86_64

Expected Behavior

With the following script: (note, the block is largely irrelevant, since jRuby never actually gets to it, but I wanted to show that we are writing, rewinding, setting encoding and then yielding it to a caller that reads from it.):

#!/usr/bin/env ruby

require 'Tempfile'

# aws-sdk S3.get_object reads/writes in byte chunks which may or may align with UTF-8 codepoints
Tempfile.open('prefix_', '/tmp', encoding: 'ASCII-8BIT:ASCII-8BIT') do |file|
  file.puts $$
  file.rewind
  file.set_encoding('UTF-8')
  # yield file
  puts file.read
end

This works in RMI 2.5.1.

Actual Behavior

It appears that jRuby 9.2.4.0 does not honor the 3rd argument (9.1.x.0 all did):

% RBENV_VERSION=2.5.1 ruby /tmp/tempfile.rb
30919
% RBENV_VERSION=jruby-9.2.4.0 ruby /tmp/tempfile.rb
ArgumentError: wrong number of arguments (3 for 2)
  <main> at /tmp/tempfile.rb:5
% RBENV_VERSION=jruby-9.1.17.0 ruby /tmp/tempfile.rb
32394
@headius

This comment has been minimized.

Copy link
Member

commented Nov 20, 2018

Strangely enough, there doesn't seem to be any changes to tempfile in JRuby between 9.1.17 and 9.2, so I'm not sure what broke. My simple fix of just bumping the optional arity up seems to allow this code to work fine. Can you turn it into a test case that confirms the options are actually being used?

@headius

This comment has been minimized.

Copy link
Member

commented Nov 20, 2018

Diff for posterity:

diff --git a/core/src/main/java/org/jruby/ext/tempfile/Tempfile.java b/core/src/main/java/org/jruby/ext/tempfile/Tempfile.java
index eede1d2731..8961068ef3 100644
--- a/core/src/main/java/org/jruby/ext/tempfile/Tempfile.java
+++ b/core/src/main/java/org/jruby/ext/tempfile/Tempfile.java
@@ -250,7 +250,7 @@ public class Tempfile extends RubyFile implements Finalizable {
         return open(context, recv, args, block);
     }
 
-    @JRubyMethod(required = 1, optional = 1, meta = true)
+    @JRubyMethod(required = 1, optional = 2, meta = true)
     public static IRubyObject open(ThreadContext context, IRubyObject recv, IRubyObject[] args, Block block) {
         RubyClass klass = (RubyClass) recv;
         Tempfile tempfile = (Tempfile) klass.newInstance(context, args, block);

@headius headius added this to the JRuby 9.2.5.0 milestone Nov 20, 2018

headius added a commit to headius/jruby that referenced this issue Nov 20, 2018

@headius

This comment has been minimized.

Copy link
Member

commented Nov 20, 2018

@erikogan I believe I have fixed this, but you should submit a spec for this behavior to https://github.com/ruby/spec. Thanks for the report!

@erikogan

This comment has been minimized.

Copy link
Author

commented Nov 20, 2018

I’m happy to submit a spec to make sure it does not regress in the future.

(If I have some free time over the holiday weekend I may also set up a git bisect to track down the breaking change, since I’m curious what caused it.)

erikogan added a commit to erikogan/spec that referenced this issue Nov 20, 2018

erikogan added a commit to erikogan/spec that referenced this issue Nov 20, 2018

eregon added a commit to ruby/spec that referenced this issue Nov 20, 2018

@headius

This comment has been minimized.

Copy link
Member

commented Nov 20, 2018

Actually it occurs to me now that you could just submit specs to JRuby since @eregon merges both ways. Thank you for the spec in any case!

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.