Undefine VERSION in gemspec to avoid namespace pollution between multiple MemoWises

[JacobEvelyn]

This commit updates memo_wise.gemspec to undefine the MemoWise::VERSION constant after it is read, as first suggested in
simplecov-ruby/simplecov#557 (comment).

Before this change, accessing MemoWise::VERSION immediately after the doff_and_don call in benchmarks.rb gives the version of the gem pulled from GitHub, even though doff_and_don should mean nothing is in the MemoWise namespace. After this change, accessing MemoWise::VERSION there gives this error as we expect:

uninitialized constant MemoWise (NameError)

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

[pboling]

Nice :)

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (5f8d31d) to head (642ee28).

Additional details and impacted files

@@            Coverage Diff            @@
##              main      #370   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            2         3    +1     
  Lines          190       192    +2     
  Branches        90        90           
=========================================
+ Hits           190       192    +2     

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

[pboling]

Update...

Thinking about this more. This could introduce a bug where another require / require_relative will not have the effect of the laoding the file because it thinks the file has already been loaded, and it doesn't know that you've removed the constant... thus the VERSION constant may never get loaded.

I do think it should be load here.

[pboling]

Basically, you want to avoid the "tracking" side effect of require because you're going to remove the VERSION constant, and thus want other requires to work normally in all circumstances.

[JacobEvelyn]

This could introduce a bug where another require / require_relative will not have the effect of the laoding the file because it thinks the file has already been loaded, and it doesn't know that you've removed the constant... thus the VERSION constant may never get loaded.

@pboling that's what I expected to happen, but I wasn't able to reproduce it locally. I suspect I had a bug in my setup though—are you able to reproduce the issue you described?

[pboling]

Ooooh. Looking into this more deeply...

require_relative does not cache the loaded file like require. This means that if you use require_relative multiple times in a single program, Ruby will reload the file each time, even if it was previously loaded.

(According to AI Llama 2.5 4B 🗡️ )

[pboling]

You should be fine with require_relative. 💯

[pboling]

I do think there is a cognitive benefit to load, as it is very clearly an indication that you want the load to be not require. It is a hard break from the require paradigm, where most people, including me, only think of the difference between require and require_relative, as a performance benefit by avoiding the load path, and not about the difference in tracking, which I'd forgotten about.

[pboling]

And that's quite enough bike shedding from me. 👯

[JacobEvelyn]

require_relative does not cache the loaded file like require. This means that if you use require_relative multiple times in a single program, Ruby will reload the file each time, even if it was previously loaded.

This is false. I just tried using require_relative to load the same file twice, and the second time it returned false and did not load a new constant I added to the file in between require_relative calls. So I think the concern still is valid; I just would like to be able to reproduce it with MemoWise before switching to load. Does that make sense?

[pboling]

Wow. So AI lied to me. :). Glad to know my original thinking was correct. I now remember when I operationalized the load paradigm for my gems I wanted to use require_relative, and tried to make that work, but I couldn't, but most of that experience is lost in the fog of time. I think I can create an example to demostrate the issue...

[JacobEvelyn]

For context, I tried reproducing the issue with this Gemfile:

# frozen_string_literal: true

source "https://rubygems.org"

git_source(:github) { |repo| "https://github.com/#{repo}.git" }

ruby ">= 2.7.7"

gem "memo_wise", github: "panorama-ed/memo_wise", branch: "version-undefined"

and this test Ruby file:

require 'memo_wise'

puts MemoWise::VERSION

[pboling]

Yep. Playing with this myself, and I think there are two paths.

First, the paranoid path, which is what I've started down, but haven't committed to:

# final param `true` wraps the loaded namespace in an anonymous module,
#   making the constant collision impossible.
# downside is it uses a $LOAD_PATH search, and the inherent performance penalty.
load("lib/gem_name/version.rb", true)

Second, the hopeful path:

# works fine, so long as the gem's other lib files, and other tools,
#   always use require / require_relative to also load the version file.
# upside is it does not use the $LOAD_PATH search, so has a performance benefit.
require_relative "lib/gem_name/version"

I can't fault either choice. I've not used the wrap parameter of load in this context yet, but I think I may start trying it out.

[JacobEvelyn]

@pboling can you explain what you mean by

so long as the gem's other lib files, and other tools, always use require / require_relative to also load the version file.

I don't think I follow.

[pboling]

require / require_relative track what is loaded, so they will be careful to not reload the same file twice. If version.rb is subsequently loaded by load then you'll get the warning constant already defined noise, assuming you don't remove the constant as this PR does.

I didn't make the point very clearly that I was going for, which is this:

I think with the final wrap parameter load(path, true), we wouldn't need to remove the constant.

That's what I'm going to try out now.

[JacobEvelyn]

@pboling gotcha, so something like this?

Module.new.tap { |mod| load("lib/memo_wise/version.rb", mod) }::MemoWise::VERSION

[pboling]

Recent versions of Ruby are able to pass a module, instead of a simple true/false wrap param, ever since this PR: ruby/ruby#4986
Unfortunately most of my libraries support older versions of Ruby still, so that's not useful for me yet. Prior to that feature, the only way to get access to the anonymous module that wrapped was a complex throw catch that straddled both the loading and the loaded files.

https://users.aalto.fi/~tontti/posts/ruby-by-examples/ruby-catching-anonymous-module/

So, my load with wrap param idea isn't viable for libraries that need old Ruby support. I suppose there isn't a good reason to not use require_relative, and I have a lot of code I should update.

Thanks for this discussion!

[pboling]

@JacobEvelyn only saw your response after my previous reply, but yes. That will work for gems that target recent versions of Ruby only.

[pboling]

So the problem now is if using require_relative, and then removing the constant, the file will not be reloaded later, because we've determined that it does track / does not reload, and the constant will remain removed. I'm not sure how this could work @JacobEvelyn

[pboling]

I guess I'm full circle to: it needs to use load, and can optionally use the wrap param if on a new enough version of Ruby.

[pboling]

Thinking about this more... still. You made this PR so that you could isolate the versions of libraries in the same namespace.

Your solution is to remove the constant. This works, however, the constant remains removed, and the version.rb file will not be reloaded by the gem. This means that MemoWise::VERSION should remain undefined later.

Your example in this comment is working but only because when you load the gem with require 'memo_wise' it doesn't load the gemspec... I think.

But I expect that there are scenarios where the gemspec and the gem could both be loaded, and in those scenarios, it could break, by not having a VERSION anymore, if something tried to reference it.

[pboling]

Aha!

https://bundler.io/guides/bundler_in_a_single_file_ruby_script.html

The example for Bundler inline is exactly this type of scenario which breaks because the gemspec and the gem are loaded in the same context!

Below I've only changed it to reference MemoWise, but otherwise it is the same as the canonical example from bundler.io linked above.

require 'bundler/inline'

gemfile do
  source "https://rubygems.org"
  git_source(:github) { |repo| "https://github.com/#{repo}.git" }
  gem "memo_wise", github: "panorama-ed/memo_wise", branch: "version-undefined"
end

puts 'Gems installed and loaded!'
puts "MemoWise Version is #{MemoWise::VERSION}"

The result of running this:

❯ ruby main.rb
Gems installed and loaded!
main.rb:10:in `<main>': uninitialized constant MemoWise::VERSION (NameError)

puts "MemoWise Version is #{MemoWise::VERSION}"
                                    ^^^^^^^^^

It blows up because it is relying on the require "memo_wise" that bundler does internally for all gems that do not have require: false appended.

If you use load + remove constant in the gemspec instead, it will work 👍

[JacobEvelyn]

Hmm @pboling when I try your code above (with load) I get:

/Users/jevelyn/.gem/ruby/3.3.3/gems/bundler-2.4.6/lib/bundler.rb:635:in `rescue in eval_gemspec':  (Bundler::GemspecError)
[!] There was an error while loading `memo_wise.gemspec`: cannot load such file -- lib/memo_wise/version. Bundler cannot continue.

 #  from /Users/jevelyn/.gem/ruby/3.3.3/bundler/gems/memo_wise-b06f1c6964e3/memo_wise.gemspec:17
 #  -------------------------------------------
 #  #   end::MemoWise::VERSION
 >  load "lib/memo_wise/version"
 #  gem_version = MemoWise.send(:remove_const, :VERSION)

[pboling]

LGTM

[pboling]

Typo. load requires the file extension .rb.

[pboling]

Suggested change

	load "lib/memo_wise/version"
	load "lib/memo_wise/version.rb"

[pboling]

LGTM

[pboling]

LGTM