File.open ignores :universal_newline options on Windows #3736

Closed
enebo opened this Issue Mar 15, 2016 · 6 comments

Comments

Projects
None yet
2 participants
@enebo
Member

enebo commented Mar 15, 2016

Environment

both jruby 1.7.24 and JRuby 9.0.5.0 have this issue.

Expected Behavior

A file which contains \r\n should still retain the \r\n and not convert to \n if universal_newline is false.

require 'csv'
require "tempfile"

@tempfile = Tempfile.new(%w"temp .csv")
@tempfile.close
@path = @tempfile.path

File.open(@path, "wb") do |file|
  file << "1\t2\t3\r\n"
  file << "4\t5\r\n"
end

f = File.open(@path, {:universal_newline=>false})
p f.gets

Actual Behavior

We seem to always return \n as delimiter.

@enebo enebo added this to the JRuby 1.7.25 milestone Mar 15, 2016

@enebo enebo added the windows label Mar 15, 2016

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Mar 15, 2016

Member

Weird.

Member

headius commented Mar 15, 2016

Weird.

@enebo

This comment has been minimized.

Show comment
Hide comment
@enebo

enebo Mar 16, 2016

Member

@headius I will land a fix today and I hope you can review it because it is ugly. I am hoping on 9k this is a much different fix but on 1.7 we have the CRLF wrapper and we set it in a place where we have no idea about ecflags. So my patch post-processes that wrapping and removes the CRLF wrapper based on ec settings :)

No doubt with fuller port on master we are only failing on master because windows is not hitting native paths. Hopefully, the fix will still be more harmonius...

Member

enebo commented Mar 16, 2016

@headius I will land a fix today and I hope you can review it because it is ugly. I am hoping on 9k this is a much different fix but on 1.7 we have the CRLF wrapper and we set it in a place where we have no idea about ecflags. So my patch post-processes that wrapping and removes the CRLF wrapper based on ec settings :)

No doubt with fuller port on master we are only failing on master because windows is not hitting native paths. Hopefully, the fix will still be more harmonius...

enebo added a commit that referenced this issue Mar 16, 2016

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Mar 28, 2016

Member

@enebo Your fix for 1.7 seems "fine" even if it is pretty hacky. I'm going to take a quick look to see if it's possible to propagate ecflags to the place we create CRLF Wrapper but, but I know 1.7 has very weak ecflags processing right now.

Member

headius commented Mar 28, 2016

@enebo Your fix for 1.7 seems "fine" even if it is pretty hacky. I'm going to take a quick look to see if it's possible to propagate ecflags to the place we create CRLF Wrapper but, but I know 1.7 has very weak ecflags processing right now.

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Mar 28, 2016

Member

@enebo I believe the patch below is a start at propagating ecflags into the actual channel wrapping. We process ecflags as part of the open process, and it's stored in the RubyFile that eventually calls fdopen, so propagating it should work ok here.

https://gist.github.com/headius/a02a91c3c744930f3876

For now I'm going to leave this change and look into the 9k fix.

Member

headius commented Mar 28, 2016

@enebo I believe the patch below is a start at propagating ecflags into the actual channel wrapping. We process ecflags as part of the open process, and it's stored in the RubyFile that eventually calls fdopen, so propagating it should work ok here.

https://gist.github.com/headius/a02a91c3c744930f3876

For now I'm going to leave this change and look into the 9k fix.

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Mar 28, 2016

Member

Ok, I think I see the issue in 9k.

Because we can't open a file in text mode on Windows (Java has no such concept) we currently force read/write conversion to use UNIVERSAL_NEWLINE_DECORATOR. Your example, explicitly tries to turn that mode off, but since we're on Windows doing text mode it gets turned back on again.

So we need a way to propagate that universal newlines were explicitly disabled and not turn them on for text mode in that case.

Member

headius commented Mar 28, 2016

Ok, I think I see the issue in 9k.

Because we can't open a file in text mode on Windows (Java has no such concept) we currently force read/write conversion to use UNIVERSAL_NEWLINE_DECORATOR. Your example, explicitly tries to turn that mode off, but since we're on Windows doing text mode it gets turned back on again.

So we need a way to propagate that universal newlines were explicitly disabled and not turn them on for text mode in that case.

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Mar 28, 2016

Member

Marking this one as fixed for 1.7. Additional work for 9k is in #3762.

Member

headius commented Mar 28, 2016

Marking this one as fixed for 1.7. Additional work for 9k is in #3762.

@headius headius closed this Mar 28, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment