Permalink
Browse files

Actually try if CC accepts the flag instead of guessing from the comp…

…iler name.
  • Loading branch information...
knu committed Jul 22, 2014
1 parent ec43ab6 commit 3c3d34f8bf16c0b6884cab35c61032174a6eae43
Showing with 15 additions and 3 deletions.
  1. +15 −3 ext/nokogiri/extconf.rb
View
@@ -81,6 +81,19 @@ def do_clean
exit! 0
end
+def add_cflags(flags)
+ print "checking if the C compiler accepts #{flags}... "
+ with_cflags("#{$CFLAGS} #{flags}") do
+ if try_compile("int main() {return 0;}")
+ puts 'yes'
+ true
+ else
+ puts 'no'
+ false
+ end
+ end
+end
+
def preserving_globals
values = [
$arg_config,
@@ -312,9 +325,8 @@ def patch
when /solaris/
$CFLAGS << " -DUSE_INCLUDED_VASPRINTF"
when /darwin/
- if RbConfig::MAKEFILE_CONFIG['CC'] !~ /gcc/ then
- $CFLAGS << " -Wno-error=unused-command-line-argument-hard-error-in-future"
- end
+ # Let Clang ignore unknown compiler flags
+ add_cflags("-Wno-error=unused-command-line-argument-hard-error-in-future")
else
$CFLAGS << " -g -DXP_UNIX"
end

11 comments on commit 3c3d34f

@keithpitty

This comment has been minimized.

Show comment Hide comment
@keithpitty

keithpitty Jul 22, 2014

👍

👍

@flavorjones

This comment has been minimized.

Show comment Hide comment
@flavorjones

flavorjones Jul 22, 2014

Owner

This is cleverer, but makes the intent of the conditional less clear. I'd like to revert.

Owner

flavorjones replied Jul 22, 2014

This is cleverer, but makes the intent of the conditional less clear. I'd like to revert.

@knu

This comment has been minimized.

Show comment Hide comment
@knu

knu Jul 22, 2014

Owner

But the original intent read "if it is not gcc", which is not even close to correct. The compiler flag in question is only supported by relatively new versions of clang/llvm. If you make it really clear, you should actually check the clang_major/minor macros.

Owner

knu replied Jul 22, 2014

But the original intent read "if it is not gcc", which is not even close to correct. The compiler flag in question is only supported by relatively new versions of clang/llvm. If you make it really clear, you should actually check the clang_major/minor macros.

@flavorjones

This comment has been minimized.

Show comment Hide comment
@flavorjones

flavorjones Jul 22, 2014

Owner

I agree completely -- do we have that information on clang and version?

I'm still hoping someone with a Mac will own this solution, and not the Nokogiri core committers who use Linux and OpenBSD, respectively. ;)

Owner

flavorjones replied Jul 22, 2014

I agree completely -- do we have that information on clang and version?

I'm still hoping someone with a Mac will own this solution, and not the Nokogiri core committers who use Linux and OpenBSD, respectively. ;)

@Tyrael

This comment has been minimized.

Show comment Hide comment
@Tyrael

Tyrael Jul 22, 2014

the guys at fink check for the clang version number:
https://github.com/fink/fink/blob/b20d38c0c743ee8c9e16d782608c6163c4f41455/compiler_wrapper-10.9.in

# To avoid extra warning spew, don't add 
# -Wno-error=unused-command-line-argument-hard-error-in-future
# when clang doesn't support it .
if [[ `clang --version | head -n1 | cut -d- -f2 | cut -d')' -f1` < "503.0.38" ]]; then
    suppress_hard_error=""
else
    suppress_hard_error="-Wno-error=unused-command-line-argument-hard-error-in-future"
fi

googling for clang 503.0.38 brings up a bunch of similar posts, so I think it could be reasonable to check for the version number

the guys at fink check for the clang version number:
https://github.com/fink/fink/blob/b20d38c0c743ee8c9e16d782608c6163c4f41455/compiler_wrapper-10.9.in

# To avoid extra warning spew, don't add 
# -Wno-error=unused-command-line-argument-hard-error-in-future
# when clang doesn't support it .
if [[ `clang --version | head -n1 | cut -d- -f2 | cut -d')' -f1` < "503.0.38" ]]; then
    suppress_hard_error=""
else
    suppress_hard_error="-Wno-error=unused-command-line-argument-hard-error-in-future"
fi

googling for clang 503.0.38 brings up a bunch of similar posts, so I think it could be reasonable to check for the version number

@flavorjones

This comment has been minimized.

Show comment Hide comment
@flavorjones

flavorjones Jul 22, 2014

Owner

Awesome, thanks for tracking this down. I'll take a pass at integrating it into extconf.rb.

Owner

flavorjones replied Jul 22, 2014

Awesome, thanks for tracking this down. I'll take a pass at integrating it into extconf.rb.

@flavorjones

This comment has been minimized.

Show comment Hide comment
@flavorjones

flavorjones Jul 22, 2014

Owner

Can anybody who had this original problem tell me what their value of RbConfig::MAKEFILE_CONFIG['CC'] is?

Owner

flavorjones replied Jul 22, 2014

Can anybody who had this original problem tell me what their value of RbConfig::MAKEFILE_CONFIG['CC'] is?

@knu

This comment has been minimized.

Show comment Hide comment
@knu

knu Jul 22, 2014

Owner

Looks like this specific flag is an extension of Apple LLVM 5.1 which is claimed to be only available for a certain period of time (will be removed in future). So, it is necessary anyway to actually check if the compiler takes the flag without an error.
The best we could do would be like this:

diff --git a/ext/nokogiri/extconf.rb b/ext/nokogiri/extconf.rb
index 769f2a3..7483f57 100644
--- a/ext/nokogiri/extconf.rb
+++ b/ext/nokogiri/extconf.rb
@@ -81,10 +81,10 @@ def do_clean
   exit! 0
 end

-def add_cflags(flags)
+def add_cflags(flags, src = 'int main() {return 0;}')
   print "checking if the C compiler accepts #{flags}... "
   with_cflags("#{$CFLAGS} #{flags}") do
-    if try_compile("int main() {return 0;}")
+    if try_compile(src)
       puts 'yes'
       true
     else
@@ -325,8 +325,14 @@ when 'mingw32', /mswin/
 when /solaris/
   $CFLAGS << " -DUSE_INCLUDED_VASPRINTF"
 when /darwin/
-  # Let Clang ignore unknown compiler flags
-  add_cflags("-Wno-error=unused-command-line-argument-hard-error-in-future")
+  # Let Apple LLVM/clang >=5.1 ignore unknown compiler flags
+  add_cflags("-Wno-error=unused-command-line-argument-hard-error-in-future", <<-SRC)
+#if defined(__APPLE_CC__) && __clang_major__ >= 5 && (__clang_major__ >= 6 || __clang_minor__ >= 1)
+int main() { return 0; }
+#else
+#error Not Apple LLVM >=5.1
+#endif
+  SRC
 else
   $CFLAGS << " -g -DXP_UNIX"
 end
Owner

knu replied Jul 22, 2014

Looks like this specific flag is an extension of Apple LLVM 5.1 which is claimed to be only available for a certain period of time (will be removed in future). So, it is necessary anyway to actually check if the compiler takes the flag without an error.
The best we could do would be like this:

diff --git a/ext/nokogiri/extconf.rb b/ext/nokogiri/extconf.rb
index 769f2a3..7483f57 100644
--- a/ext/nokogiri/extconf.rb
+++ b/ext/nokogiri/extconf.rb
@@ -81,10 +81,10 @@ def do_clean
   exit! 0
 end

-def add_cflags(flags)
+def add_cflags(flags, src = 'int main() {return 0;}')
   print "checking if the C compiler accepts #{flags}... "
   with_cflags("#{$CFLAGS} #{flags}") do
-    if try_compile("int main() {return 0;}")
+    if try_compile(src)
       puts 'yes'
       true
     else
@@ -325,8 +325,14 @@ when 'mingw32', /mswin/
 when /solaris/
   $CFLAGS << " -DUSE_INCLUDED_VASPRINTF"
 when /darwin/
-  # Let Clang ignore unknown compiler flags
-  add_cflags("-Wno-error=unused-command-line-argument-hard-error-in-future")
+  # Let Apple LLVM/clang >=5.1 ignore unknown compiler flags
+  add_cflags("-Wno-error=unused-command-line-argument-hard-error-in-future", <<-SRC)
+#if defined(__APPLE_CC__) && __clang_major__ >= 5 && (__clang_major__ >= 6 || __clang_minor__ >= 1)
+int main() { return 0; }
+#else
+#error Not Apple LLVM >=5.1
+#endif
+  SRC
 else
   $CFLAGS << " -g -DXP_UNIX"
 end
@flavorjones

This comment has been minimized.

Show comment Hide comment
@flavorjones

flavorjones Jul 22, 2014

Owner

Fair enough, let's stick with what you committed. However, when I run add_cflags() against my compiler (gcc) this code raises an exception instead of silently failing to add the flag to $CFLAGS.

And if I wrap the try_compile call in a begin/rescue block, it looks like the flag is still added to $CFLAGS so subsequent compiles fail. Is there some mkmf magic going on that I just haven't discovered yet?

Owner

flavorjones replied Jul 22, 2014

Fair enough, let's stick with what you committed. However, when I run add_cflags() against my compiler (gcc) this code raises an exception instead of silently failing to add the flag to $CFLAGS.

And if I wrap the try_compile call in a begin/rescue block, it looks like the flag is still added to $CFLAGS so subsequent compiles fail. Is there some mkmf magic going on that I just haven't discovered yet?

@knu

This comment has been minimized.

Show comment Hide comment
@knu

knu Jul 22, 2014

Owner

@flavorjones Oh, I must have missed something then. I'll tackle it tomorrow morning!

Owner

knu replied Jul 22, 2014

@flavorjones Oh, I must have missed something then. I'll tackle it tomorrow morning!

@knu

This comment has been minimized.

Show comment Hide comment
@knu

knu Jul 23, 2014

Owner

@flavorjones Looks like it raised an error in the first call of try_compile if a bad compiler flag is given because in that case have_devel? would think the compiler would never work.

I added another call earlier, so the problem should be gone.

Owner

knu replied Jul 23, 2014

@flavorjones Looks like it raised an error in the first call of try_compile if a bad compiler flag is given because in that case have_devel? would think the compiler would never work.

I added another call earlier, so the problem should be gone.

Please sign in to comment.