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

Conversation

joshcooper
Copy link
Contributor

@joshcooper joshcooper commented Aug 13, 2024

Update puppet to support minitar 1.0 which contains breaking changes.

Note this important warning from https://github.com/halostatue/minitar

Minitar does not perform validation of path names provided to the convenience
classes Minitar::Output and Minitar::Input, which use Kernel.open for their
underlying implementations when not given an IO-like object.

As a result we always pass a reader/writer to the unpack/pack methods respectively.

Fixes #9446

This is blocked on adding minitar 1.0.x to the runtime.

@joshcooper joshcooper added the blocked PRs blocked on work external to the PR itself label Aug 14, 2024
Update puppet to support minitar 1.0 which contains breaking changes.

Note this important warning from https://github.com/halostatue/minitar

    Minitar does not perform validation of path names provided to the convenience
    classes Minitar::Output and Minitar::Input, which use Kernel.open for their
    underlying implementations when not given an IO-like object.

As a result we always pass a reader/writer to the unpack/pack methods
respectively.
@@ -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?

@@ -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')

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocked PRs blocked on work external to the PR itself
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Minitar v1 has been released
3 participants