Use IO.copy_stream when reading, writing#6958
Conversation
martinemde
force-pushed
the
martinemde/io-copy-stream
branch
from
77ae281 to
846dd72
Compare
segiddins
left a comment
segiddins
left a comment
There was a problem hiding this comment.
I think that's fundamentally what IO.copy_stream is doing under the hood, agree it's what we should be doing
martinemde
changed the title
|
Strangely truffleruby fails every time but only on this branch. IO.copy_stream discrepancy? It fails, If I'm not mistaken, because a binary file from a gem (a file that would pass through this change) is producing a different checksum. It is presumably checking this same checksum on the other rubies. |
martinemde
force-pushed
the
martinemde/io-copy-stream
branch
from
c047809 to
2a7d462
Compare
segiddins
force-pushed
the
martinemde/io-copy-stream
branch
from
2a7d462 to
f94a88a
Compare
martinemde
force-pushed
the
martinemde/io-copy-stream
branch
from
f94a88a to
5ae7af7
Compare
| raise Gem::Package::FormatError.new(e.message, entry.full_name) | ||
| end | ||
|
|
||
| if RUBY_ENGINE == "truffleruby" |
There was a problem hiding this comment.
add a ruby engine version check as well?
There was a problem hiding this comment.
Is there a fixed version? I haven't heard anything. A ticket was mentioned but it had to do with the return value of copy_stream. I don't think this is cause by the return value.
There was a problem hiding this comment.
truffleruby/truffleruby#3280 (comment) -- 23.1.2, to be released next month
There was a problem hiding this comment.
Oh, that makes sense. If it leaves our tar reader pos at the beginning, we can't possibly read the files right.
lib/rubygems/package.rb
Outdated
|
|
||
| if RUBY_ENGINE == "truffleruby" | ||
| def copy_stream(src, dst) # :nodoc: | ||
| dst.write src.read 16_384 until src.eof? |
There was a problem hiding this comment.
if tests keep failing, maybe change this to dst.write src.read ?
There was a problem hiding this comment.
We have to assume it's the second change that's failing, reading from the tar.gz. This is the original code for the first change.
martinemde
left a comment
martinemde
left a comment
There was a problem hiding this comment.
Maybe full read then write is required when reading from tgz in truffleruby.
|
If this doesn't work I'll just make one change at a time. The befuddlement to code ratio of this change is surprisingly high. |
|
@martinemde I think this is ready to merge? |
|
I'll merge now, then when truffleruby is released we can follow up with an engine version when we know it works. |
Use IO.copy_stream when reading, writing (cherry picked from commit 558f516)
4 participants
What was the end-user or developer problem that led to this PR?
Looking at increasing read/write efficiency. I'm not sure if this is actually better, but I believe that's the goal of IO.copy_stream.
What is your fix for the problem, implemented in this PR?
Use IO.copy_stream when writing gems to files from the gem archive.
Make sure the following tasks are checked