curl / curl

A command line tool and library for transferring data with URL syntax, supporting DICT, FILE, FTP, FTPS, GOPHER, GOPHERS, HTTP, HTTPS, IMAP, IMAPS, LDAP, LDAPS, MQTT, POP3, POP3S, RTMP, RTMPS, RTSP, SCP, SFTP, SMB, SMBS, SMTP, SMTPS, TELNET, TFTP, WS and WSS. libcurl offers a myriad of powerful features
https://curl.se/
Other
35.73k stars 6.41k forks source link

./configure incorrectly adds mmacosx-version-min to iOS/watchOS/tvOS builds #6138

Closed hamstergene closed 3 years ago

hamstergene commented 3 years ago

Problem

./configure script contains this piece:

  19975     if test -z "$(echo $CFLAGS | grep m.*os.*-version-min)"; then
  19976       min="-mmacosx-version-min=10.8"                                                                                                   
  19977       CFLAGS="$CFLAGS $min"

It breaks iOS/watchOS/tvOS (any non-macOS) build if -miphoneos-version-min is given as part of CC rather than part of CFLAGS for example:

CC=/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/cc  -arch arm64 -isysroot /Applications/Xcode.app/Contents/Developer/Platforms/iPhoneOS.platform/Developer/SDKs/iPhoneOS13.2.sdk -miphoneos-version-min=9.0
CFLAGS=-g -fPIC

...or if the minimum version is provided via an environment variable instead:

export IPHONEOS_DEPLOYMENT_TARGET=9.0

What I expected

  1. curl configure script either needs better detection of *-version-min presence
  2. or maybe it needs to lose that mmacosx-version-min piece and let the user take care of that

Context

The reason I tried providing essential flags via CC instead of CFLAGS is #3189 — the true cause of that bug was that ./configure was invoking preprocessor as $CC -E without the essential -isysroot flag which caused some autoconf checks to fail to find any system headers. The advice inside #3189 to update command line tools is not correct way to do it; the correct solution is to give sysroot/target/etc to both CFLAGS, CPPFLAGS, and LDFLAGS.

bagder commented 3 years ago

Maybe we could just extend the check and also see if the user has set the option in CC? Like this:


diff --git a/acinclude.m4 b/acinclude.m4
index e7a36e4bd..f78bb547d 100644
--- a/acinclude.m4
+++ b/acinclude.m4
@@ -2524,13 +2524,13 @@ AC_DEFUN([CURL_MAC_CFLAGS], [

   AC_MSG_CHECKING([for good-to-use Mac CFLAGS])
   AC_MSG_RESULT([$tst_cflags]);

   if test "$tst_cflags" = "yes"; then
-    AC_MSG_CHECKING([for *version-min in CFLAGS])
+    AC_MSG_CHECKING([for *version-min in CFLAGS or CC])
     min=""
-    if test -z "$(echo $CFLAGS | grep m.*os.*-version-min)"; then
+    if test -z "$(echo $CFLAGS $CC | grep m.*os.*-version-min)"; then
       min="-mmacosx-version-min=10.8"
       CFLAGS="$CFLAGS $min"
     fi
     if test -z "$min"; then
       AC_MSG_RESULT([set by user])
bagder commented 3 years ago

We could perhaps even check $IPHONEOS_DEPLOYMENT_TARGET in there?

hamstergene commented 3 years ago

Grepping $CC $CFLAGS should cover most users I think. I don't know how many users use environment variables, but on my memory I can't recall seeing that in the wild often.

Checking environment may be a bit more complicated:

  % strings /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/clang | grep DEPLOYMENT_TARGET
MACOSX_DEPLOYMENT_TARGET
IPHONEOS_DEPLOYMENT_TARGET
TVOS_DEPLOYMENT_TARGET
WATCHOS_DEPLOYMENT_TARGET
BRIDGEOS_DEPLOYMENT_TARGET
DRIVERKIT_DEPLOYMENT_TARGET

EDIT: the problem with environment variables is that we don't know if they will actually be used. Maybe IPHONEOS_DEPLOYMENT_TARGET is set, but the compiler will be targeting Mac? IMO that's bad design from Apple side, there is just no way for curl to have it 100% right in all cases.