Skip to content

Conversation

@tamird
Copy link
Contributor

@tamird tamird commented Dec 10, 2015

Slight cleanup.

@brianmario
Copy link
Owner

I'm 👎 on this if only because having a version that can be programmatically checked at runtime allows for more flexibility in how the library can be used. Suppose a 3rd party library supports multiple mysql2 versions (across API changes) and has conditions in its code as such. Or maybe an automated security audit of some code wants to ensure mysql2 is of a specific version (though there are other ways that can be done outside of runtime).

Was this causing an issue?

@tamird
Copy link
Contributor Author

tamird commented Dec 10, 2015

It wasn't causing a serious issue, just some warning spam:

$ bundle exec rake clean
/Users/tamird/src/mysql2/mysql2.gemspec:3: warning: already initialized constant Mysql2::GEMSPEC
/Users/tamird/src/mysql2/mysql2.gemspec:3: warning: previous definition of GEMSPEC was here

That can certainly be silenced in other ways.

That said, it's still possible to programmatically access the version number, as you can see in this diff using: Gem.loaded_specs['mysql2'].version.

This change is basically just removing VERSION in favour of asking Gem for the version.

@brianmario
Copy link
Owner

This change is basically just removing VERSION in favour of asking Gem for the version.

Right but then the callers codebase is dependent on rubygems being loaded right? In some cases that's not desirable - for example, we have some extremely hot-path code that needs to load and execute as quickly as possible so we skip loading rubygems as well as any other absolutely non-essential code. While we're not checking this constant, I wouldn't want to assume nobody else would.

It looks like that warning was introduced in bb44a01, maybe another solution would be to only assign/create the Mysql2::GEMSPEC constant once by adding an unless defined? Mysql2::GEMSPEC at the end of that block?

@sodabrew
Copy link
Collaborator

Or

diff --git a/tasks/compile.rake b/tasks/compile.rake
index 4ffbefc..ce610cd 100644
--- a/tasks/compile.rake
+++ b/tasks/compile.rake
@@ -1,6 +1,6 @@
 require "rake/extensiontask"

-load File.expand_path('../../mysql2.gemspec', __FILE__)
+load File.expand_path('../../mysql2.gemspec', __FILE__) unless defined? Mysql2::GEMSPEC

 Rake::ExtensionTask.new("mysql2", Mysql2::GEMSPEC) do |ext|
   # put binaries into lib/mysql2/ or lib/mysql2/x.y/

@tamird
Copy link
Contributor Author

tamird commented Dec 10, 2015

Alright, you've convinced me. Closing in favour of #713

@tamird tamird closed this Dec 10, 2015
@tamird tamird deleted the remove-version-const branch December 10, 2015 18:06
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.

3 participants