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

Fix File#write_buffer to always return the given io #575

Merged
merged 1 commit into from Apr 9, 2024

Conversation

casperisfine
Copy link

Ref: ef89a62

This fixes a regression in 2.4.rc1.

cc @hainesr

@hainesr hainesr self-assigned this Apr 9, 2024
@hainesr hainesr merged commit 57cff33 into rubyzip:2.4 Apr 9, 2024
20 checks passed
@hainesr
Copy link
Member

hainesr commented Apr 9, 2024

Thanks for this.

I've cherry-picked it into master too 🚀

@casperisfine
Copy link
Author

Thanks for the quick merge. I have a few more failures in my app, I'll try to figure if they are regressions or us using undocumented behavior.

@hainesr
Copy link
Member

hainesr commented Apr 9, 2024

If you can stand it, it's probably worth seeing how things go with 3.0.0 too - there are like 3 years of bug fixes in there 😮

I'm going to try and cut an RC for version 3 later today...

@casperisfine
Copy link
Author

it's probably worth seeing how things go with 3.0.0 too

yeah, I ought to look at it soonish. I attempted an upgrade and saw hundreds of errors on CI, so I've put it on the backlog 😅

@casperisfine
Copy link
Author

Alright, I got a minmal repro for what I think is a regression:

# frozen_string_literal: true

require 'bundler/inline'

gemfile do
  source 'https://rubygems.org'
  gem 'rubyzip', ENV.fetch("VERSION", "2.3.2")
  gem 'minitest'
end


require 'zip'
require 'zip/version'

puts "RubyZip: #{Zip::VERSION}"

require 'minitest/autorun'

class ZipTest < Minitest::Test
  ZIP = "PK\x03\x04\x14\x00\x00\x00\b\x002^\x89X\x16\xE8v@c\x00\x00\x00\x01\x00\x01\x00\x14\x00\x00\x00resources_export.csv\xED\xC1\xC9\r\x00\x10\x00\x00\xB0\xBFYL%D<x8\x12\xE3\eD\xDB\x96\xE38=\x95\x19\xD7\x9Em\xD4p\x01\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\xF8QxPK\x01\x024\x03\x14\x00\x00\x00\b\x002^\x89X\x16\xE8v@c\x00\x00\x00\x01\x00\x01\x00\x14\x00\x00\x00\x00\x00\x00\x00\x01\x00\x00\x00\xA4\x81\x00\x00\x00\x00resources_export.csvPK\x05\x06\x00\x00\x00\x00\x01\x00\x01\x00B\x00\x00\x00\x95\x00\x00\x00\x00\x00"

  def test_write_into_other_io
    other_io = StringIO.new

    Zip::File.open_buffer(ZIP) do |z|
      z.write_buffer(other_io)
    end

    other_io.rewind
    assert_equal ZIP.bytesize, other_io.size
  end
end

Basically, I think write_to should always write, because it can't assume it's being asked to write back into the same IO it read from.

Now maybe I misunderstand the API?

I'll open a PR and we can discuss there.

@hainesr
Copy link
Member

hainesr commented Apr 9, 2024

yeah, I ought to look at it soonish. I attempted an upgrade and saw hundreds of errors on CI, so I've put it on the backlog 😅

It would be really useful if you could use https://github.com/rubyzip/rubyzip/wiki/Updating-to-version-3.x and see if anything is missing!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants