Refactor extconf.rb

[lowjoel]

Hello there.

I've made a pretty major rewrite for extconf.rb. It was generating bad makefiles for Windows (since it is not commonly tested) and while trying to fix that I felt that the flow of logic could be much better.

I've split the path detection logic into the detect_* functions, and the configuration logic to configure_* functions. These are different depending on the platform.

I've tried to be as faithful to the original implementation as possible, however I have not tested this on platforms other than Windows. Comments much appreciated.

[lowjoel]

Hmm, the breakage is on Ruby 1.9.1, 1.8.7, and Ruby EE because of require 'rake'. All are no longer supported. Should they be removed from the build matrix?

[sodabrew]

Thanks for the effort on this! Will review it shortly. There's a ton of material in the merge queue at the moment.

I am surprised that the Makefiles aren't correct on Windows - I spent a lot of time updating the Windows support for the 0.3.18 release, and added AppVeyor tests. What version of Windows and what compiler environment are you using?

We're not removing any older Rubies just yet - the error is actually in the rm_f usage:

...  `rm_f': wrong number of arguments (3 for 2) (ArgumentError)
... from extconf.rb line 300

[lowjoel]

Ah yes, Windows. extconf.rb currently should work for MingW, but I compile mine using VC++. There's a ton of conditionals in extconf that test for Unixy environments (that MingW is more than VC++), and also logic for generating libmysql.a from libmysql.dll (that VC++ doesn't need, and errors out because of missing dlltool and friends).

Also, because mysqlclient.lib is a static linkage version of MySQL, and that doesn't work too well with VC++ (the correct compiler version must be used -- the MySQL C connector comes with 1 mysqlclient.lib per VC++ version). libmysql.lib works better because it's a DLL and any version of VC++ (and MingW) should be able to link to it.

line 300 is have_func('rb_thread_blocking_region')... I think rake has overridden rm_f, I don't directly use it.

[sodabrew]

Oh, that's great. I really appreciate that you're a VC++ user and dug into the build system to make it better across all platforms / Rubies (...gotta get those 1.8.7 and 1.9.3 Rubies working though...). Very odd error with rm_f.

[lowjoel]

Hopefully with the platform code split up to various classes debugging the build system and improving it should be better in future.

I'm not sure how to start fixing the old Rubies though.

[tamird]

@lowjoel looks like this patch fixes it:

diff --git a/ext/mysql2/extconf.rb b/ext/mysql2/extconf.rb
index 8320218..d6505e4 100644
--- a/ext/mysql2/extconf.rb
+++ b/ext/mysql2/extconf.rb
@@ -1,6 +1,12 @@
 # encoding: UTF-8
 require 'mkmf'
-require 'rake'
+
+# rake fucks with rm_f
+class << self
+  alias_method :rm_f_original, :rm_f
+  require 'rake'
+  alias_method :rm_f, :rm_f_original
+end

 class Platform
   # Gets the proper platform object for the current platform.

Maybe that should be wrapped in some condition on RUBY_VERSION.

[sodabrew]

Strikes me as either a) a bug in rake, or b) newer rake doesn't work with 1.8.7 anymore. Thanks for looking at this, @tamird. Let's get that patch into this branch and it should be mergeable in the 0.4.0 queue.

[lowjoel]

@tamird Thanks for the patch, I've rebased and hope it passes! @sodabrew

[sodabrew]

Thanks for updating the PR! Logged in my (slow) review queue.

[lowjoel]

Thanks!

[sodabrew]

Would you do another rebase to master now that prepared statements have landed?

[lowjoel]

Okay, done!

I'm not sure if I'm expecting merge conflicts though? It appears that this shouldn't affect what happens in the library, unless I'm missing something...

[sodabrew]

Mostly a matter of triggering the tests to ensure that the combined code runs tests cleanly.

[lowjoel]

Okay, it seems to pass :)

[sodabrew]

On my Mac, I have to remove the second argument for this to succeed.

That said, in the master extconf.rb, the output from mysql_config --libs is used without calling have_library. This is preferred to allow the user to provide an explicit --with-mysql-config=... that points at an non-default location.

[lowjoel]

So I think the problem here is that --with-mysql-config isn't properly accepted? See the other comment you made below.

[lowjoel]

This method only handles the detection of libraries if there is no mysql-config path specified.

[lowjoel]

@sodabrew I've added the check and rebased. Have a look.

[sodabrew]

Thanks for updating! Please rebase again when you get a chance, and I think there may be one more rebase next before this PR is up for final review.

[lowjoel]

Done!

[lowjoel]

Wait, @sodabrew: this was recently added:

# This is our wishlist. We use whichever flags work on the host.
  # -Wall and -Wextra are included by default.
  # TODO: fix statement.c and remove -Wno-error=declaration-after-statement
  %w(
    -Werror
    -Weverything
    -fsanitize=address
    -fsanitize=integer
    -fsanitize=thread
    -fsanitize=memory
    -fsanitize=undefined
    -fsanitize=cfi
    -Wno-error=declaration-after-statement
  ).each do |flag|
    if try_link('int main() {return 0;}', flag)
      $CFLAGS << ' ' << flag
    end
  end

That only applies for GCC/clang? VC most definitely do not support those.

My latest rebase were to pick only my changes.

[sodabrew]

Could you rebase once more? Sorry about the ever-changing whitespace in extconf.rb this week!

[lowjoel]

@sodabrew no problem. I've added the compile flag wishlist back to the same one as current master. Also I saw that mariadb was added to the paths to check, my branch has been fixed to support that too.

[tamird]

Apologies in advance; #634 is about to drop another conflict on this

[lowjoel]

@tamird Ping me when that's merged. The only thing I'd preserve from #634 would be to add another directory to check for mysql's dev files (/usr/local/opt/mysql5*, which I think is for homebrew-like installs?)

[tamird]

You should preserve the entire diff from #634 because the current code results in an infinite loop if it can't find the mysql client headers.

[lowjoel]

@tamird OK -- I see how the loop could continue forever. Does b492763 solve the problem?

[tamird]

Why do you insist on keeping this cruft?

[lowjoel]

Well, I'd rather not break things unnecessarily. If the decision is that we don't need this, then I'd be happy to omit it.

[tamird]

0.4 is out and the rubocop PR landed earlier today. @lowjoel would you rebase this?

[lowjoel]

Rebased, have a look.

I've suppressed 2 classes of Rubocop warnings, they are:

extconf.rb:13:1: C: Metrics/ClassLength: Class has too many lines. [120/100]
class Platform
^^^^^
extconf.rb:111:5: C: Style/GuardClause: Use a guard clause instead of wrapping the code inside a conditional expression.
    if inc && lib
    ^^
extconf.rb:330:5: C: Style/GuardClause: Use a guard clause instead of wrapping the code inside a conditional expression.
    unless RbConfig::CONFIG['host_os'] =~ /mswin|mingw/
    ^^^^^^

I think following the recommendation would make the build script even harder to read.

If you're okay with this, I'll squash it to one commit.

[tamird]

Attempting to get this off my GH queue.