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 logger as dependency for ruby 3.5 support #204

Merged
merged 1 commit into from
Jan 27, 2025

Conversation

noraj
Copy link
Contributor

@noraj noraj commented Jan 27, 2025

fix #203

## v1.72.4

* Add logger as a dependency for Ruby 3.5.0+ support - thanks Alexandre ZANNI!

Copy link
Owner

Choose a reason for hiding this comment

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

❤️

@alexdalitz alexdalitz merged commit 0f038bc into alexdalitz:master Jan 27, 2025
4 checks passed
@noraj noraj deleted the patch-logger branch January 27, 2025 22:59
@@ -48,5 +48,6 @@ DNSSEC NSEC3 support.'
end

s.add_runtime_dependency 'base64', '~> 0.2.0'
s.add_runtime_dependency 'logger', '~> 1.6.5'
Copy link
Contributor

Choose a reason for hiding this comment

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

I did some research and got mixed results whether or not adding this dependency unconditionally was a good idea. Some said it should only be added where needed, with a guard to permit adding it only in certain versions. Would it be better to add such a guard ,e.g. the code below?:

  if Gem::Version.new(RUBY_VERSION) >= Gem::Version.new('3.4.0') # or other version number
    s.add_runtime_dependency 'logger', '~> 1.6.5' 
  end

(The Gem::Version approach was recommended at https://greena13.github.io/blog/2020/12/19/writing-ruby-gems-for-different-versions-of-ruby-and-rails/.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Adding the dependency unconditionally could restrain the range of Ruby version supported. However, now requiring it before 3.4.0, means you'd have a different version of logger depending on the major version of ruby used (3.1, 3.2, 3.3) so it could be challenging for consistency.

Also, isn't it the kind of stuff you should put in your Gemfile rather than in the gemspec? If I remember correctly, having conditional requirement is challenging because it results in different lockfile depending on the Ruby version used to generated it (also there isn't a Gemfile.lock published for this project, or if there had one the CI could remove it before bundle install).

Idk. I remember I tried once to play with conditional dependency requirements in Gemfile/gempsec, and spent hours without finding a configuration that wasn't and headache to manage.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Isn't there something like You can’t use conditionals on gemspec because gemspec is serialized into YAML so you have to put it in Gemfile?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To be state of the art in term of dependency requirements, I would:

  • Commit the Gemfile.lock

This is why the Gemfile.lock should be committed with version control, and shared between development and production deployments of the Ruby application. cf. source

  • Not put any add_development_dependency in the Gemspec

Because else there are listed on Rubygems when you publish you gem.

image

And bundle install installs all dependencies (runtime and dev) by default so anyone end-up install dev dependencies that are useless for runtime if they don't provide the flag or env. var. to not install them.

  • Specify the same runtime dependencies in the Gemfile as in the Gemspec.
  • Add the dev dependencies in the Gemfile and tag them with proper grouping so they can be excluded for example on CI etc.

For example

Gemspec with only runtime dependencies
https://github.com/noraj/unisec/blob/master/unisec.gemspec

Gemfile with runtime and dev dependencies that also specify the Gemspec and precise grouping:
https://github.com/noraj/unisec/blob/master/Gemfile

Gemfile.lock published…:
https://github.com/noraj/unisec/blob/master/Gemfile.lock

…that was generated using the same version of Ruby as mentioned in .tool-versions which is the latest version supported by s.required_ruby_version = ['>= 3.1.0', '< 4.0'] in the Gemspec.
https://github.com/noraj/unisec/blob/master/.tool-versions

Proper grouping in the Gemfile allowing bundle to exclude useless groups like documentation in the CI:
https://github.com/noraj/unisec/blob/4f3637ddc92c4582318e1cbc78df67eb816a6789/.github/workflows/ruby.yml#L24

No dev dependencies in the Gemspec so they are no listed on Rubygems so people doing gem install unisec won't install them by default. Only people git cloning the github repo that performs bundle install that will actually dev on the project.

image

I think that would be the best and without conditional requirement of dependencies.

@keithrbennett
Copy link
Contributor

keithrbennett commented Jan 30, 2025 via email

@noraj
Copy link
Contributor Author

noraj commented Jan 30, 2025

We could find articles defending any position.

There does seem to be some controversy surrounding checking the
Gemfile.lock file of a gem into source control

I think that's important to have reproducible builds. Or when the project is unmaintained for a moment compatibility with a newer version could break, the lock file ensures we at least have a configuration where it works.

my feeling is that a dependency should not dictate to an app which version to use,

I'm open about that, I don't really have arguments against or for it.

when the dependency is something as commonly used across other dependencies and the app itself as logger is.

The version requirements on logger could may be loosened. If someone is willing to test different versions of logger to move to something like s.add_runtime_dependency 'logger', '>= 1.5.0' accepting older versions.

It's difficult to know what the oldest version of Ruby should be targeted.

  • s.required_ruby_version is not provided in dnsruby.gemspec
  • .github/workflows/ci.yml tests only on Ruby 3.1-3.3 (3.4 is missing by the way)
  • no .rubocop.yml including a TargetRubyVersion is provided is provided in git

The easiest move would be to support only Ruby supported versions (https://www.ruby-lang.org/en/downloads/branches/) so 3.1-3.4. And so supporting:

@noraj
Copy link
Contributor Author

noraj commented Jan 30, 2025

Here is my conversation with Perplexity AI

I don't trust AI answers as there is no intelligence behind it. That's just language parsing and statistics on bit datasets. So the IA must summarize what it "learned" from StackExchange or some blogs.

However, Shopify is an enterprise greatly proficient in Ruby (https://railsatscale.com/) and contributing a lot to the community, then often innovate and contribute to Ruby core. So it should be interesting to see what convention they apply on their projects: https://github.com/orgs/Shopify/repositories?q=mirror%3Afalse+fork%3Afalse+archived%3Afalse+language%3ARuby.

We can see they:

@keithrbennett
Copy link
Contributor

keithrbennett commented Jan 31, 2025 via email

@noraj
Copy link
Contributor Author

noraj commented Jan 31, 2025

I appreciate that you are investing so much effort and expertise in this.

I ❤️ Ruby so much I'm happy to help others learn more about it and its ecosystem.

And thank you for your contributions to dnsruby!

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.

Ruby 3.4 support - logger
3 participants