[vcpkg] Add -isysroot args to CPPFLAGS by m-kuhn · Pull Request #25201 · microsoft/vcpkg

m-kuhn · 2022-06-12T20:39:39Z

So preprocessor only configure tests get the right includes

Describe the pull request

What does your PR fix?
CPPFLAGS are currently lacking -isysroot when invoking autoconf tools. This leads to preprocessor only tests missing headers and producing bogus configuration results. This is an issue on macos where system headers do not live in the system root but rather a dedicated sdk folder.

For more information see https://github.com/microsoft/vcpkg/pull/21847/files#r776056939

Which triplets are supported/not supported? Have you updated the CI baseline?

all
Does your PR follow the maintainer guide?

Yes
If you have added/updated a port: Have you run ./vcpkg x-add-version --all and committed the result?

No port touched

scripts/cmake/vcpkg_configure_make.cmake

So preprocessor only configure tests get the right includes

JackBoosY · 2022-06-13T08:13:07Z

scripts/cmake/vcpkg_configure_make.cmake

+    set(C_FLAGS "${VCPKG_DETECTED_CMAKE_C_FLAGS_${flag_suffix}}")
+    separate_arguments(C_FLAGS)
+    list(FIND C_FLAGS "-isysroot" ISYSROOT_FLAG_POS)


Why not use if ("${VCPKG_DETECTED_CMAKE_C_FLAGS_${flag_suffix}}" MATCHES "-isysroot")?
Are there other similar options?

${VCPKG_DETECTED_CMAKE_C_FLAGS_${flag_suffix}} is a string with all flags, separate_arguments splits it into a list and this line finds the index in this list to extract just this argument, so we can use it. I don't see how MATCHES could replace this logic.
Am I missing the point?

I think the irritating point is that one part of flag processing is done with regex on the string, and one part of flag processing is (now) done on a list.

The regex approach will likely run into limitations if the path contains spaces

m-kuhn · 2022-06-13T13:30:50Z

scripts/cmake/vcpkg_configure_make.cmake

+        MATH(EXPR ISYSROOT_FLAG_POS "${ISYSROOT_FLAG_POS}+1")
+        list(SUBLIST C_FLAGS ${ISYSROOT_FLAG_POS} 1 ISYSROOT)
+        set(CPPFLAGS_${flag_suffix} "${CPPFLAGS_${flag_suffix}} -isysroot ${ISYSROOT}")
+        string(REGEX REPLACE "-isysroot ${ISYSROOT}" "" CFLAGS_${flag_suffix} "${VCPKG_DETECTED_CMAKE_C_FLAGS_${flag_suffix}}")


@Neumann-A removing this from CFLAGS / CXXFLAGS breaks the build

?

Why and where?

It should always be $(CPPFLAGS) $(CFLAGS) in the makefiles.

From gettext-runtime/configure in checking whether <wchar.h> uses 'inline' correctly:

if $CC -o conftest$ac_exeext $CFLAGS $LDFLAGS conftest1.$ac_objext conftest2.$ac_objext $LIBS >&5 2>&1; then

BillyONeal · 2022-06-13T17:12:58Z

This is an issue on macos where system headers do not live in the system root but rather a dedicated sdk folder.

Shouldn't the toolchain be making sure any such SDK folder is available rather than having each build system under the sun try to fix it up like this?

dg0yt · 2022-06-13T17:38:53Z

This is an issue on macos where system headers do not live in the system root but rather a dedicated sdk folder.

Shouldn't the toolchain be making sure any such SDK folder is available rather than having each build system under the sun try to fix it up like this?

In vcpkg, "each build system under the sun" should take the details from CMake. Taking from CMake is done here:

vcpkg/scripts/get_cmake_vars/CMakeLists.txt

Lines 101 to 114 in 8e1f46d

    
               if(CMAKE_SYSTEM_NAME MATCHES "Darwin") 
        
                   if("${flag_var}" IN_LIST VCPKG_LANG_FLAGS) 
        
                       # macOS - append arch and isysroot if cross-compiling 
        
                       if(NOT "${CMAKE_OSX_ARCHITECTURES}" STREQUAL "${CMAKE_HOST_SYSTEM_PROCESSOR}") 
        
                           foreach(arch IN LISTS CMAKE_OSX_ARCHITECTURES) 
        
                               string(APPEND ${flag_var} " -arch ${arch}") 
        
                           endforeach() 
        
                       endif() 
        
                       string(APPEND ${flag_var} " -isysroot ${CMAKE_OSX_SYSROOT}") 
        
                       if (CMAKE_OSX_DEPLOYMENT_TARGET) 
        
                           string(APPEND ${flag_var} " -mmacosx-version-min=${CMAKE_OSX_DEPLOYMENT_TARGET}")  
        
                       endif() 
        
                   endif() 
        
               endif()

Should it be taken differently, before transforming it for a particular build system such as autotools?

BillyONeal · 2022-06-13T17:57:37Z

In vcpkg, "each build system under the sun" should take the details from CMake. Taking from CMake is done here:

vcpkg/scripts/get_cmake_vars/CMakeLists.txt

Lines 101 to 114 in 8e1f46d

if(CMAKE_SYSTEM_NAME MATCHES "Darwin")

if("${flag_var}" IN_LIST VCPKG_LANG_FLAGS)

# macOS - append arch and isysroot if cross-compiling

if(NOT "${CMAKE_OSX_ARCHITECTURES}" STREQUAL "${CMAKE_HOST_SYSTEM_PROCESSOR}")

foreach(arch IN LISTS CMAKE_OSX_ARCHITECTURES)

string(APPEND ${flag_var} " -arch ${arch}")

endforeach()

endif()

string(APPEND ${flag_var} " -isysroot ${CMAKE_OSX_SYSROOT}")

if (CMAKE_OSX_DEPLOYMENT_TARGET)

string(APPEND ${flag_var} " -mmacosx-version-min=${CMAKE_OSX_DEPLOYMENT_TARGET}")

endif()

endif()

endif()

Should it be taken differently, before transforming it for a particular build system such as autotools?

It seems like this should mean -isysroot should already be present in VCPKG_DETECTED_Xxx?

Neumann-A · 2022-06-13T18:27:02Z

It seems like this should mean -isysroot should already be present in VCPKG_DETECTED_Xxx?

it is the problem is that a pure preprocessor run does not have the flag.

BillyONeal · 2022-06-13T18:30:34Z

it is the problem is that a pure preprocessor run does not have the flag.

Shouldn't a pure preprocessor run have all flags though? This is far from the only compiler switch that may affect preprocessor results.

dg0yt · 2022-06-13T18:55:46Z

Should it be taken differently, before transforming it for a particular build system such as autotools?

It seems like this should mean -isysroot should already be present in VCPKG_DETECTED_Xxx?

Well, there is no VCPKG_DETECTED_Xxx for the preprocessor. There is C and CXX. It somewhat questionable that we first push the sysroot flag into the C/CXX flags, and later pull it out again, instead of forwarding it separately.

Neumann-A · 2022-06-13T21:05:57Z

Shouldn't a pure preprocessor run have all flags though? This is far from the only compiler switch that may affect preprocessor results.

Nope due to differences in C and CXX flags. Some CXX flags are not allowed in C so there needs to be some filtering. I opted only for -D flags since I was sure they won't make problems but we might need to pass additional stuff like include directories.

The question here is rather why does it only do you preprocessor check and no compile check. Maybe availability of includes is only checked with the preprocessor?

m-kuhn · 2022-06-13T21:18:22Z

The question here is rather why does it only do you preprocessor check and no compile check. Maybe availability of includes is only checked with the preprocessor?

Availability of certain contents of includes in this case, FWIW.

# checking for uid_t in sys/types.h
  cat confdefs.h - <<_ACEOF >conftest.$ac_ext
/* end confdefs.h.  */
#include <sys/types.h>

_ACEOF
if (eval "$ac_cpp conftest.$ac_ext") 2>&5 |
  $EGREP "uid_t" >/dev/null 2>&1; then :
  ac_cv_type_uid_t=yes
else
  ac_cv_type_uid_t=no
fi

But I'm not sure what this knowledge changes, properly populating CPPFLAGS is probably the more portable way forward than changing such constructs in various autoconf ports?

m-kuhn · 2022-06-15T07:10:10Z

Should it be taken differently, before transforming it for a particular build system such as autotools?

It seems like this should mean -isysroot should already be present in VCPKG_DETECTED_Xxx?

Well, there is no VCPKG_DETECTED_Xxx for the preprocessor. There is C and CXX. It somewhat questionable that we first push the sysroot flag into the C/CXX flags, and later pull it out again, instead of forwarding it separately.

Adding something like VCPKG_DETECTED_CPP_FLAGS would be somewhat alien because there is no cmake equivalent. But overall it might still be cleaner.

m-kuhn · 2022-06-16T04:48:45Z

@JackBoosY is the answer good enough to remove author-response?

JackBoosY · 2022-06-17T07:03:11Z

Ping @BillyONeal @dg0yt @Neumann-A for review agian.

BillyONeal · 2022-06-30T18:55:40Z

It seems like this should mean -isysroot should already be present in VCPKG_DETECTED_Xxx?

it is the problem is that a pure preprocessor run does not have the flag.

It doesn't look like this is setting a pure preprocessor variable though, it's setting CPPFLAGS.

Adding something like VCPKG_DETECTED_CPP_FLAGS would be somewhat alien because there is no cmake equivalent. But overall it might still be cleaner.

I'm confused, why is that not VCPKG_DETECTED_CXX_FLAGS as already set here?

4b2916b#diff-b92b07d5c0c681687d71463c6a368fae98e9ffedb241e2aadbde3aac688899a9R93

m-kuhn · 2022-06-30T19:48:23Z

Sorry if I'm stating something obvious, just to avoid a potential source of confusion:

CFLAGS: C Compiler Flags
CXXFLAGS: C++ Compiler Flags
CPPFLAGS: Preprocessor Flags (a concept not used natively in cmake, where they are added to CFLAGS/CXXFLAGS)

My understanding:
For running configure we want CPPFLAGS to be set because some checks (apparently) rely on preprocessor runs.
Since cmake internally doesn't separate between pre-processor and processor flags, there's no simple cmake variable we could extract for this purpose and we need to either rely on heuristics (extracting CPPFLAGS via regex magic for defines, -isysroot, other keywords) or populate this in the toolchain file (although I'm not sure this is the only point of entry for these).

Currently this is being completely ignored and things work because on osx -isysroot is implicitly added by a wrapper executable called gcc that internally calls clang with -isysroot already populated and the sysroot on linux builds is often left to default. This is a potential source of problems, not least for cross compiling (android, ios) also.

BillyONeal · 2022-06-30T19:50:34Z

Ah, CPP as in C PreProcessor, not C PlusPlus. No, that was not obvious to me. :) My "coming from a platform where the preprocessor is not a separate program" is showing.

m-kuhn · 2022-07-10T14:54:35Z

#25670 implements the same approach as for meson, discarding this approach until the other (osx specific approach) runs into limitations

m-kuhn mentioned this pull request Jun 12, 2022

[vcpkg] Enhancement android triplet #21847

Closed

github-actions bot approved these changes Jun 12, 2022

View reviewed changes

Neumann-A reviewed Jun 12, 2022

View reviewed changes

scripts/cmake/vcpkg_configure_make.cmake Outdated Show resolved Hide resolved

Neumann-A reviewed Jun 12, 2022

View reviewed changes

scripts/cmake/vcpkg_configure_make.cmake Outdated Show resolved Hide resolved

Adela0814 assigned JackBoosY Jun 13, 2022

JackBoosY added the category:tool-update The issue is with build tool or build script, which requires update or should be executed correctly label Jun 13, 2022

Add -isysroot args to CPPFLAGS

4b2916b

So preprocessor only configure tests get the right includes

m-kuhn force-pushed the cppflags_isysroot branch from 0c0f17d to 4b2916b Compare June 13, 2022 04:37

github-actions bot approved these changes Jun 13, 2022

View reviewed changes

JackBoosY reviewed Jun 13, 2022

View reviewed changes

JackBoosY added the requires:author-response label Jun 13, 2022

m-kuhn commented Jun 13, 2022

View reviewed changes

Leave isysroot in CFLAGS and CXXFLAGS

ef76161

github-actions bot approved these changes Jun 13, 2022

View reviewed changes

JackBoosY removed the requires:author-response label Jun 17, 2022

JackBoosY requested a review from BillyONeal June 17, 2022 09:58

PhoebeHui changed the title ~~Add -isysroot args to CPPFLAGS~~ [vcpkg] Add -isysroot args to CPPFLAGS Jul 4, 2022

m-kuhn mentioned this pull request Jul 10, 2022

[vcpkg script] Ios support for meson and autotools based ports #25670

Closed

m-kuhn closed this Jul 10, 2022

Conversation

m-kuhn commented Jun 12, 2022

What does your PR fix?

Which triplets are supported/not supported? Have you updated the CI baseline?

Does your PR follow the maintainer guide?

If you have added/updated a port: Have you run ./vcpkg x-add-version --all and committed the result?

Uh oh!

Uh oh!

Uh oh!

JackBoosY Jun 13, 2022

Choose a reason for hiding this comment

Uh oh!

m-kuhn Jun 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dg0yt Jun 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

m-kuhn Jun 13, 2022

Choose a reason for hiding this comment

Uh oh!

m-kuhn Jun 13, 2022

Choose a reason for hiding this comment

Uh oh!

Neumann-A Jun 13, 2022

Choose a reason for hiding this comment

Uh oh!

m-kuhn Jun 13, 2022

Choose a reason for hiding this comment

Uh oh!

BillyONeal commented Jun 13, 2022

Uh oh!

dg0yt commented Jun 13, 2022

Uh oh!

BillyONeal commented Jun 13, 2022

Uh oh!

Neumann-A commented Jun 13, 2022

Uh oh!

BillyONeal commented Jun 13, 2022

Uh oh!

dg0yt commented Jun 13, 2022

Uh oh!

Neumann-A commented Jun 13, 2022

Uh oh!

m-kuhn commented Jun 13, 2022

Uh oh!

m-kuhn commented Jun 15, 2022

Uh oh!

m-kuhn commented Jun 16, 2022

Uh oh!

JackBoosY commented Jun 17, 2022

Uh oh!

BillyONeal commented Jun 30, 2022

Uh oh!

m-kuhn commented Jun 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

BillyONeal commented Jun 30, 2022

Uh oh!

m-kuhn commented Jul 10, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

If you have added/updated a port: Have you run `./vcpkg x-add-version --all` and committed the result?

m-kuhn Jun 13, 2022 •

edited

Loading

dg0yt Jun 13, 2022 •

edited

Loading

m-kuhn commented Jun 30, 2022 •

edited

Loading