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

⬆️ gem_bench v2.0.5 #351

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

⬆️ gem_bench v2.0.5 #351

wants to merge 2 commits into from

Conversation

pboling
Copy link
Contributor

@pboling pboling commented Sep 20, 2024

Fixes the require_relative > require issue discussed in #349 for the gem_bench gem itself. Does not merit a mention in the changelog, IMO.

Before merging:

  • Copy the table printed at the end of the latest benchmark results into the README.md and update this PR
  • If this change merits an update to CHANGELOG.md, add an entry following Keep a Changelog guidelines with semantic versioning

Copy link

codecov bot commented Sep 20, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (ef52a1a) to head (0a4d95f).

Additional details and impacted files
@@            Coverage Diff            @@
##              main      #351   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            2         2           
  Lines          190       190           
  Branches        90        90           
=========================================
  Hits           190       190           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@pboling pboling changed the title ⬆️ gem_bench v2.0.4 ⬆️ gem_bench v2.0.5 Sep 22, 2024
@pboling
Copy link
Contributor Author

pboling commented Sep 22, 2024

It failed to run the benchmarks because I forgot to enter my MFA code to actually push the new gem version to RubyGems before I made this change. 😬 🤦 It will pass if run again, no other changes needed. The gem is released now.

@JacobEvelyn
Copy link
Member

Thanks! I'll wait to re-run this until after #350 is merged.

@JacobEvelyn
Copy link
Member

@pboling based on #349 (comment) I expected that with this code, if I add puts MemoWise::VERSION directly after the doff_and_don call I'd see an error, but I do not. What am I misunderstanding?

@pboling
Copy link
Contributor Author

pboling commented Sep 26, 2024

@JacobEvelyn as it happens, I have another gem, require_bench, which is for mencharking and debugging require and load statements... I'll see if I can figure out how Memowise::VERSION is available at that point.

@pboling
Copy link
Contributor Author

pboling commented Sep 26, 2024

@JacobEvelyn OK, I figured out exactly what is happening.

I'd like to demostrate the solution with yet another gem of mine, version_gem, which I built to avoid this exact problem, but it will take me a bit to get around to a PR for that.

To briefly summarize - gem_bench uses bundler to figure out what a gem's path is so it can copy it. In order to do this bundler has to load the available "gemspecs". Most gems use a terrible practice (IMO) of requiring their version.rb file into their gemspec, and this makes it impossible for that namespace to not be loaded if bundler has accessed the gem. When require paths don't match up this also results in the nefariously common:

warning: already initialized constant MemoWise::VERSION
warning: previous definition of VERSION was here

I use this version_gem parttern in every gem I've published in the last few years to get around that ^ annoying issue. See: https://gitlab.com/oauth-xx/version_gem/#usage for how to set it up.

I've just reviewed the README over there, and found that I left out the secret sauce that actually solves this issue.

So here it is from gembench's gemspec:
https://github.com/pboling/gem_bench/blob/main/gem_bench.gemspec#L1-L9

# Get the GEMFILE_VERSION without *require* "my_gem/version", for code coverage accuracy
# See: https://github.com/simplecov-ruby/simplecov/issues/557#issuecomment-825171399
load "lib/gem_bench/version.rb"
gem_version = GemBench::Version::VERSION
GemBench::Version.send(:remove_const, :VERSION)

Gem::Specification.new do |spec|
  spec.name = "gem_bench"
  spec.version = gem_version
  # ...
end

You'll note that it only removes the VERSION constant after grabbing a local variable pointer to the value it holds. This means that in the way I've been using it, it does still leave behind namespace pollution. That could be avoidable, perhaps by removing the namespace as well, but perhaps namespace pollution is actually good? This is the first I've thought of it.

In any case, using that pattern you could clean up all of the constants that get defined there, and have an entirely clean gemspec. Also, to follow the pattern, you actually don't need version_gem at all. It includes a few other nice-to-haves that I just happen to want everywhere.

Without fixing this, the MemoWise::VERSION constant will always be loaded by each different version of memo_wise you test, and they will initially all conflict with each other, because initially, in the gemspecs loaded by bundler, they all occupy the same space.

The version_gem library was extracted from the very popular oauth2 library, which I also maintain, a few years ago.

The original reason the version_gem pattern was created was for code coverage accuracy. Because of requiring version.rb in the gemspec it made code coverage tools see that the file had already been loaded, and they were unable to track coverage of the file, which prevented my repos from getting to 100% line coverage.

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.

2 participants