Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
hamstergene opened this issue Oct 28, 2020 · 3 comments
Closed
Labels

Comments

@hamstergene
Copy link

@hamstergene hamstergene commented Oct 28, 2020

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 bagder added the build label Oct 28, 2020
@bagder
Copy link
Member

@bagder bagder commented Oct 28, 2020

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
Copy link
Member

@bagder bagder commented Oct 28, 2020

We could perhaps even check $IPHONEOS_DEPLOYMENT_TARGET in there?

bagder added a commit that referenced this issue Oct 29, 2020
... even if set in the CC or IPHONEOS_DEPLOYMENT_TARGET variables.

Fixes #6138
@hamstergene
Copy link
Author

@hamstergene hamstergene commented Oct 29, 2020

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.

bagder added a commit that referenced this issue Oct 29, 2020
... even if set in the CC or IPHONEOS/MACOSX_DEPLOYMENT_TARGET
variables.

Fixes #6138
Closes #6140
@bagder bagder closed this in c131148 Oct 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

2 participants
You can’t perform that action at this time.