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

Add support for minitar 1.x #9449

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion Gemfile
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ group(:features) do
gem 'hocon', '~> 1.0', require: false
# requires native libshadow headers/libs
#gem 'ruby-shadow', '~> 2.5', require: false, platforms: [:ruby]
gem 'minitar', '~> 0.9', require: false
gem 'minitar', '~> 1.0', require: false
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it required to list it here if it's also in the gemspec? Is that something special because it's a feature?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We only add minitar as a hard runtime dependency when building windows specific puppet gems, for example, compare the runtime dependencies in https://rubygems.org/gems/puppet/versions/8.8.1 and https://rubygems.org/gems/puppet/versions/8.8.1-x64-mingw32. So it's a hard dependency on windows but soft on other platforms. And soft dependencies are managed via puppet's feature system, e.g. Puppet.features.minitar?

gem 'msgpack', '~> 1.2', require: false
gem 'rdoc', ['~> 6.0', '< 6.4.0'], require: false, platforms: [:ruby]
# requires native augeas headers/libs
Expand Down
2 changes: 1 addition & 1 deletion lib/puppet/feature/base.rb
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@
# We have Hiera
Puppet.features.add(:hiera, :libs => ["hiera"])

Puppet.features.add(:minitar, :libs => ["archive/tar/minitar"])
Puppet.features.add(:minitar, :libs => ["minitar"])

# We can manage symlinks
Puppet.features.add(:manages_symlinks) do
Expand Down
26 changes: 13 additions & 13 deletions lib/puppet/module_tool/tar/mini.rb
Original file line number Diff line number Diff line change
Expand Up @@ -3,17 +3,12 @@
class Puppet::ModuleTool::Tar::Mini
def unpack(sourcefile, destdir, _)
Zlib::GzipReader.open(sourcefile) do |reader|
# puppet doesn't have a hard dependency on minitar, so we
# can't be certain which version is installed. If it's 0.9
# or above then we can prevent minitar from fsync'ing each
# extracted file and directory, otherwise fallback to the
# old behavior
args = [reader, destdir, find_valid_files(reader)]
spec = Gem::Specification.find_by_name('minitar')
if spec && spec.version >= Gem::Version.new('0.9')
args << { :fsync => false }
end
Archive::Tar::Minitar.unpack(*args) do |action, name, stats|
files = find_valid_files(reader)

# Never pass a source file as a string to unpack, otherwise minitar will
# call Kernel.open on it, which could invoke shell commands. Always pass a
# reader that responds to `:read`
Minitar.unpack(reader, destdir, files, fsync: false) do |action, name, stats|
case action
when :dir
validate_entry(destdir, name)
Expand All @@ -33,7 +28,10 @@ def unpack(sourcefile, destdir, _)

def pack(sourcedir, destfile)
Zlib::GzipWriter.open(destfile) do |writer|
Archive::Tar::Minitar.pack(sourcedir, writer) do |step, name, stats|
# Never pass the destination file as a string to pack, otherwise minitar
# will call Kernel.open on it, which could invoke shell commands. Always
# pass a writer that responds to `:write`
Minitar.pack(sourcedir, writer) do |step, name, stats|
# TODO smcclellan 2017-10-31 Set permissions here when this yield block
# executes before the header is written. As it stands, the `stats`
# argument isn't mutable in a way that will effect the desired mode for
Expand Down Expand Up @@ -93,7 +91,9 @@ def set_default_user_and_group!(stats)
# tar format info: https://pic.dhe.ibm.com/infocenter/zos/v1r13/index.jsp?topic=%2Fcom.ibm.zos.r13.bpxa500%2Ftaf.htm
# pax format info: https://pic.dhe.ibm.com/infocenter/zos/v1r13/index.jsp?topic=%2Fcom.ibm.zos.r13.bpxa500%2Fpxarchfm.htm
def find_valid_files(tarfile)
Archive::Tar::Minitar.open(tarfile).collect do |entry|
raise ArgumentError, "Cannot list files from '#{tarfile}', because the object does not implement a 'read' method" unless tarfile.respond_to?(:read)

Minitar.open(tarfile).collect do |entry|
flag = entry.typeflag
if flag.nil? || flag =~ /[[:digit:]]/ && (0..7).cover?(flag.to_i)
entry.full_name
Expand Down
2 changes: 1 addition & 1 deletion puppet.gemspec
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,6 @@ Gem::Specification.new do |spec|
if platform == 'x64-mingw32' || platform == 'x86-mingw32'
# ffi 1.16.0 - 1.16.2 are broken on Windows
spec.add_runtime_dependency('ffi', '>= 1.15.5', '< 1.17.0', '!= 1.16.0', '!= 1.16.1', '!= 1.16.2')
spec.add_runtime_dependency('minitar', '~> 0.9')
spec.add_runtime_dependency('minitar', '~> 1.0')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

to ensure we don't pull in the broken 1.0.0. should we enforce 1.0.1 or newer?

Suggested change
spec.add_runtime_dependency('minitar', '~> 1.0')
spec.add_runtime_dependency('minitar', '~> 1.0', '>= 1.0.1')

end
end
6 changes: 3 additions & 3 deletions spec/unit/module_tool/tar/mini_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ def initialize(mode = 0100)

expect(Zlib::GzipWriter).to receive(:open).with(destfile).and_yield(writer)
stats = {:mode => 0222}
expect(Archive::Tar::Minitar).to receive(:pack).with(sourcedir, writer).and_yield(:file_start, 'abc', stats)
expect(Minitar).to receive(:pack).with(sourcedir, writer).and_yield(:file_start, 'abc', stats)

minitar.pack(sourcedir, destfile)
end
Expand All @@ -70,7 +70,7 @@ def initialize(mode = 0100)
writer = double('GzipWriter')

expect(Zlib::GzipWriter).to receive(:open).with(destfile).and_yield(writer)
expect(Archive::Tar::Minitar).to receive(:pack).with(sourcedir, writer).
expect(Minitar).to receive(:pack).with(sourcedir, writer).
and_yield(:file_start, 'abc', {:entry => MockFileStatEntry.new(nil)})

minitar.pack(sourcedir, destfile)
Expand All @@ -82,7 +82,7 @@ def unpacks_the_entry(type, name, mode = 0100)
expect(Zlib::GzipReader).to receive(:open).with(sourcefile).and_yield(reader)
expect(minitar).to receive(:find_valid_files).with(reader).and_return([name])
entry = MockFileStatEntry.new(mode)
expect(Archive::Tar::Minitar).to receive(:unpack).with(reader, destdir, [name], {:fsync => false}).
expect(Minitar).to receive(:unpack).with(reader, destdir, [name], {:fsync => false}).
and_yield(type, name, {:entry => entry})
entry
end
Expand Down