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

Declare explicit dependency on benchmark #141

Closed
wants to merge 1 commit into from

Conversation

Earlopain
Copy link

Ruby plans to bundle it: ruby/ruby#11492

That means, in the next ruby version requiring it without it appearing in the gemspec will warn, and the release after that will error.

Ruby plans to bundle it: ruby/ruby#11492

That means, in the next ruby version requiring it without it appearing in the
gemspec will warn, and the release after that will error.
Earlopain added a commit to Earlopain/rails that referenced this pull request Aug 29, 2024
Ruby plans to make `benchmark` a bundled gem: ruby/ruby#11492

It doesn't make much sense to continue this core extension since rails would
need to add it to its gemspec to continue offering it for what basically amounts to `x * 1000`.

Since requiring it could emit a warning, add it to the gemspec until it can later be removed
again with Rails 8.1

The benchmark generator will continue to work as usual:
It adds `benchmark-ips` to the users Gemfile, which in turn should depend
on `benchmark`: evanphx/benchmark-ips#141
@eregon
Copy link
Contributor

eregon commented Aug 29, 2024

I think it would be good to wait that that PR is merged at least to merge this one.
It doesn't seem nice that gem install benchmark-ips will now install two gems, when benchmark-ips so far was dependency-free.

@Earlopain
Copy link
Author

Of course, there is no rush

@evanphx
Copy link
Owner

evanphx commented Aug 29, 2024

Will this break current users though?

@@ -41,4 +41,5 @@ Gem::Specification.new do |s|
s.add_dependency(%q<minitest>, ["~> 5.4"])
s.add_dependency(%q<rdoc>, ["~> 4.0"])
end
s.add_dependency(%q<benchmark>, ["~> 0.1"])
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
s.add_dependency(%q<benchmark>, ["~> 0.1"])
s.add_dependency(%q<benchmark>, ["> 0.1"])

Copy link
Owner

Choose a reason for hiding this comment

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

No reason to stay on a 0. release forever.

@Earlopain
Copy link
Author

Will this break current users though?

If they don't update benchmark-ips for about two years but keep up with ruby, yes. I don't imagine any other breakage as a result of this change. Both ips and benchmark make no restrictions about their supported ruby version and adding past bundled gems to the gemspec of other gems has mostly been painless.

I'll come back to this later and address your PR feedback when/if the ruby PR ends up merged. Eregon has made a comment there which may have an impact.

@eregon
Copy link
Contributor

eregon commented Aug 29, 2024

benchmark doesn't test older than 2.5: https://github.com/ruby/benchmark/blob/master/.github/workflows/test.yml
benchmark-ips tests from 2.3: https://github.com/evanphx/benchmark-ips/blob/master/.github/workflows/ci.yml

FWIW Process.clock_gettime exists since Ruby 2.1 (current benchmark relies on it).

@Earlopain
Copy link
Author

Seems likenit's missing a required_ruby_version of at least 2.1. Testing against such old versions is hard and I wouldn't be surprised if there are other such instances.

For what's its worth, I agree with your points. There are barely any changes to it in the last decade and I made the same point about javascript somewhere else when the same thing happened with base64.

It's a bit sad that what basically amounts to syntactical sugar requires a dependeny. But when it comes down to it, I prefer my 50% of my downstream dependencies not having these. Same can be said for forwardable, singleton, etc. I'm sure if they were proper dependencies from the start they wouldn't have seen much adoption, and from PRs removing mutex_m, base64, observable which users mostly use for convenience expecting them to just be there, maintainers largely agree.

This all doesn't apply to gems with what I would call actual functionality, like rdoc or webrick. You can debate about some of these but overall I would 100% say it's going too far.

@eregon
Copy link
Contributor

eregon commented Aug 30, 2024

@Earlopain Maybe you could post your opinion on https://bugs.ruby-lang.org/issues/20309 too?
I think it'd be good to show I'm not the only one with concerns about this change.

@Earlopain
Copy link
Author

Interacting in the bugtracker is daunting for me. I wrote a thing there, hopefully that's adequate.

@Earlopain
Copy link
Author

Seems like they will go on with bundling benchmark. I openend ruby/benchmark#24 for the missing required_ruby_version but by now its already to late anyways.

I looked through the code here, and I'm wondering if there even is any dependency on benchmark. For example,

# Use a monotonic clock if available, otherwise use Time
handles the lack/existance of Process.clock_gettime without relying on benchmark.

This gem advertises itself as an extension to benchmark, but it seems the only thing it actually does is occupy in the same namespace. There is no require "benchmark" I can find anywhere. My bad for not checking previously.

I guess, if that is the case, I can just close this?

@eregon
Copy link
Contributor

eregon commented Sep 8, 2024

Right, seems there is no dependency so it shouldn't warn and this can be closed then.

@nateberkopec
Copy link
Collaborator

I played around with this last night and tried undefining benchmark at various points, etc. There's no actual dependency AFAICT 😆

@Earlopain
Copy link
Author

Yes, I appologise. It didn't compute that there may be no dependency here and I didn't check. Thanks for the conversation anyways

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.

4 participants