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

Consolidate gem dependencies and metadata #9306

Closed
joshcooper opened this issue Apr 1, 2024 · 5 comments
Closed

Consolidate gem dependencies and metadata #9306

joshcooper opened this issue Apr 1, 2024 · 5 comments
Labels
enhancement New feature or request triaged Jira issue has been created for this

Comments

@joshcooper
Copy link
Contributor

joshcooper commented Apr 1, 2024

TL;DR

We should remove the gem metadata contained in ext/project_data.yaml for the
puppet gem, instead use the gemspec as the source of truth
when building it.

Background

Puppet (and Facter's) gemspec files are only used when running under bundler,
but not when building the gems!! Since there are multiple sources of truth, we
have frequently updated one but not the other. Most recently, the puppet 8.0.0
gem listed the wrong minimum required ruby version, forcing us to release 8.0.1

One complexity is we publish platform-specific gems for puppet:

https://rubygems.org/downloads/puppet-8.5.1.gem
https://rubygems.org/downloads/puppet-8.5.1-x86-mingw32.gem
https://rubygems.org/downloads/puppet-8.5.1-x64-mingw32.gem
https://rubygems.org/downloads/puppet-8.5.1-universal-darwin.gem

The reason is because puppet depends on CFPropertyList on macOS and ffi on
Windows. By publishing platform specific gems, we can add runtime dependencies
that only affect that platform.

You might be asking why not just add CFPropertyList and ffi for all platforms?
The reason is those gems contain native extensions, and so would require a
compiler and ruby-dev packages to be installed in order to gem install puppet.

Technically, CFPropertyList itself doesn't contain native extensions, but it has
at various times had a runtime dependency on nokogiri, which does.

Proposal

  1. Move and reconcile all of the common metadata from ext/project_data.yaml to
    the gemspec, such as author, description, required ruby version, etc.

  2. Move .gemspec back to puppet.gemspec, since it should be used to build the
    gem, see commit 8f81b522a98472

  3. For the platform-specific gems, create corresponding
    puppet-<platform>.gemspec files. Each gemspec can append platform specific
    dependencies like:

    $ cat puppet-x64-mingw32.gemspec
    spec = Gem::Specification.load('puppet.gemspec')
    spec.platform = 'x64-mingw32'
    spec.add_runtime_dependency('ffi', '1.15.5')
    spec
  4. Modify our build process to build the generic gem:

    gem build <project>.gemspec
  5. Optionally, build platform specific gems in a data driven way. In other
    words, the build automation should not have a list of platforms to build. It
    should be based on the names of the gemspec files. For example, we could
    conceivably have different platform-specific gemspec in 7.x vs main branches.

    for file in <project>-*.gemspec
    do
        gem build $file
    done

One downside of this approach is Gemfile will need to contain conditional logic to include the right gemspec file, taking into account the bundler and rubygems use different platform names:

source ENV['GEM_SOURCE'] || "https://rubygems.org"

case platform
when 'x64-mingw32'
  gemspec(name: 'puppet-x64-mingw32.gemspec')
...
else
  gemspec
end

Alternatives

It's possible to add conditional logic in the gemspec, to include additional runtime
dependencies based on the platform we're packaging for:

Gem::Specification.new do |spec|
  ...
  # something like
  if Gem::Platform.match('x64-mingw32')
    spec.add_runtime_dependency('ffi', '1.15.5')
  end
end

And then specify the platform to build for gem build --platform x64-mingw32 puppet.gemspec. One nice thing is the Gemfile would not need to change. However, the list of the platforms would need to be stored somewhere, and could be different for 7.x vs main.

Additional Context

1342839
2707fd8
https://puppet.atlassian.net/browse/PA-1077
https://puppet.atlassian.net/browse/PA-2058
https://puppet.atlassian.net/browse/PUP-8685

@joshcooper joshcooper added enhancement New feature or request triaged Jira issue has been created for this labels Apr 1, 2024
Copy link

github-actions bot commented Apr 1, 2024

Migrated issue to PUP-12031

@bastelfreak
Copy link
Contributor

Hi,
I think this is a good path forward that was also requested multiple times in the past, specifically from people that package Puppet for some distributions (like I do for Arch Linux). I don't have a strong opinion on the two different approaches. A single gemspec feels a bit more natural to me. I would put all data in a single gemspec file and track the desired platforms in another file next to the gemspec, so you can just iterate on it in the pipeline.

@joshcooper
Copy link
Contributor Author

A single gemspec feels a bit more natural to me.

Agreed, we're investigating this approach. It also means the Gemfile doesn't need to change, since gemspec will include the one and only gemspec.

@joshcooper
Copy link
Contributor Author

Fixed in #9325

The gem build puppet.gemspec command now works as expected. It's also possible to build platform specific gems, for example gem build --platform x64-mingw32, even when running on a different platform.

And there is a rake task (rake pl_ci:gem_build) to build "all the gems", so that if/when we add new platforms like those based on ucrt.

@AriaXLi
Copy link
Contributor

AriaXLi commented May 9, 2024

Change was backported to 7.x in #9337

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request triaged Jira issue has been created for this
Projects
None yet
Development

No branches or pull requests

3 participants