Skip to content

[vcpkg] Meson osx sysroot #21772

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.

Sign up for GitHub

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

Merged
vicroms merged 6 commits into from
Feb 24, 2022
Merged

[vcpkg] Meson osx sysroot #21772

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
19d1706
[get_cmake_vars] add -isysroot flag independednt of crosscompiling
daschuer Nov 28, 2021
4c298e5
[meson] add -arch and -isysroot to linker flags in case of osx crossc…
daschuer Nov 28, 2021
267e8e3
[meson] avoid DEPRECATION warning about flags in the [properties] sec…
daschuer Nov 30, 2021
d1f915c
[meson] don't set OSXROOT evironment varibale
daschuer Jan 10, 2022
7e4ecfe
[meson] don't set MACOSX_DEPLOYMENT_TARGET environment variable
daschuer Jan 15, 2022
c56c9c1
[meson/make] Add CMAKE_OSX_DEPLOYMENT_TARGET to VCPKG_DETECTED_CMAKE_…
daschuer Jan 15, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 20 additions & 16 deletions scripts/cmake/vcpkg_configure_meson.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -164,11 +164,17 @@ function(z_vcpkg_meson_generate_flags_properties_string out_var config_type)
set(libpath "${libpath_flag}${CURRENT_INSTALLED_DIR}${path_suffix}/lib")

z_vcpkg_meson_convert_compiler_flags_to_list(cflags "${VCPKG_DETECTED_CMAKE_C_FLAGS_${config_type}}")
if(VCPKG_TARGET_IS_OSX AND VCPKG_DETECTED_CMAKE_OSX_DEPLOYMENT_TARGET)
vcpkg_list(APPEND cflags "-mmacosx-version-min=${VCPKG_DETECTED_CMAKE_OSX_DEPLOYMENT_TARGET}")
endif()
vcpkg_list(APPEND cflags "-I${CURRENT_INSTALLED_DIR}/include")
z_vcpkg_meson_convert_list_to_python_array(cflags ${cflags})
string(APPEND result "c_args = ${cflags}\n")

z_vcpkg_meson_convert_compiler_flags_to_list(cxxflags "${VCPKG_DETECTED_CMAKE_CXX_FLAGS_${config_type}}")
if(VCPKG_TARGET_IS_OSX AND VCPKG_DETECTED_CMAKE_OSX_DEPLOYMENT_TARGET)
vcpkg_list(APPEND cxxflags "-mmacosx-version-min=${VCPKG_DETECTED_CMAKE_OSX_DEPLOYMENT_TARGET}")
endif()
vcpkg_list(APPEND cxxflags "-I${CURRENT_INSTALLED_DIR}/include")
z_vcpkg_meson_convert_list_to_python_array(cxxflags ${cxxflags})
string(APPEND result "cpp_args = ${cxxflags}\n")
Expand All @@ -179,6 +185,17 @@ function(z_vcpkg_meson_generate_flags_properties_string out_var config_type)
set(linker_flags "${VCPKG_DETECTED_CMAKE_STATIC_LINKER_FLAGS_${config_type}}")
endif()
z_vcpkg_meson_convert_compiler_flags_to_list(linker_flags "${linker_flags}")
if(VCPKG_TARGET_IS_OSX)
# macOS - append arch and isysroot if cross-compiling
if(NOT "${VCPKG_OSX_ARCHITECTURES}" STREQUAL "${VCPKG_DETECTED_CMAKE_HOST_SYSTEM_PROCESSOR}")
foreach(arch IN LISTS VCPKG_OSX_ARCHITECTURES)
vcpkg_list(APPEND linker_flags -arch "${arch}")
endforeach()
endif()
if(VCPKG_DETECTED_CMAKE_OSX_SYSROOT)
vcpkg_list(APPEND linker_flags -isysroot "${VCPKG_DETECTED_CMAKE_OSX_SYSROOT}")
endif()
endif()
Comment on lines 182 to 192

@Neumann-A Neumann-A Jan 11, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add it to the linker flags in get_cmake_vars/CMakeLists.txt ?

@daschuer daschuer Jan 15, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This does not work, because it would also affect autotools ports

@JackBoosY JackBoosY Jan 18, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ping @Neumann-A for reply.

@JackBoosY JackBoosY Jan 19, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This does not work, because it would also affect autotools ports

Can you declare more?

@Neumann-A Neumann-A Jan 19, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This does not work, because it would also affect autotools ports

I also don't see why autotools should break with the additional flags. In general vcpkg should avoid adding additional flags in vcpkg_configure_ scripts.

@daschuer daschuer Jan 19, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now we have:

set(VCPKG_DETECTED_CMAKE_SHARED_LINKER_FLAGS_DEBUG "-arch arm64 -isysroot /Applications/Xcode_12.2.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX11.0.sdk")

/bin/sh ../libtool  --tag=CC   --mode=link gcc -fvisibility=hidden -fPIC -mmacosx-version-min=15.0 -arch arm64 -isysroot /Applications/Xcode_12.2.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX11.0.sdk -g  -liconv -Wl,-framework -Wl,CoreFoundation  -no-undefined -export-symbols-regex '^([^g]|g[^l]|gl[^w]|glw[^t]|glwt[^h]|glwth[^r]|glwthr[^e]|glwthre[^a]|glwthrea[^d]).*' -version-info 10:0:2 -rpath /Users/runner/mixxx-vcpkg/installed/arm64-osx-min11.0/debug/lib -L/Users/runner/mixxx-vcpkg/installed/arm64-osx-min11.0/debug/lib -arch arm64 -isysroot /Applications/Xcode_12.2.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX11.0.sdk -o libintl.la  bindtextdom.lo dcgettext.lo dgettext.lo gettext.lo finddomain.lo hash-string.lo loadmsgcat.lo localealias.lo textdomain.lo l10nflist.lo explodename.lo dcigettext.lo dcngettext.lo dngettext.lo ngettext.lo plural.lo plural-exp.lo localcharset.lo threadlib.lo lock.lo relocatable.lo langprefs.lo localename.lo localename-table.lo log.lo printf.lo setlocale.lo setlocale-lock.lo setlocale_null.lo version.lo xsize.lo osdep.lo intl-compat.lo

With -arch and -isysroot applied twice.

Conclusion: the current state of this PR is correct.

@Neumann-A Neumann-A Jan 19, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't share that conclusion.
The reason that is happening is because the common makefile has something along the line $(CC) $(CPPFLAGS) $(CFLAGS) $(LDFLAGS). Which of course doubles the flags.

@daschuer daschuer Jan 19, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which common make file do you reference?

What do you propose as an alternative solution?

@JackBoosY JackBoosY Jan 20, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All the flags should be extract from cmake:

vcpkg/scripts/cmake/vcpkg_configure_meson.cmake

Line 383 in 5ef52b5

z_vcpkg_get_cmake_vars(cmake_vars_file)

Therefore, the behavor of cmake, make, qmake and meson etc. should be same.

@daschuer daschuer Jan 20, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, this is already the case.

The issue here is that cmake and autotools take these flags from the c flags and pass them to the linker. Meson does not do it and require a special handlung.

I have demonstrated that the requested solution does apply the flags twice. The current state of the PR passes the right number of flags as desired.

qmake is out of scope, because it does not use z_vcpkg_get_cmake_vars

If you have an alternative solution in mind that works as well, please propose.

vcpkg_list(APPEND linker_flags "${libpath}")
z_vcpkg_meson_convert_list_to_python_array(linker_flags ${linker_flags})
string(APPEND result "c_link_args = ${linker_flags}\n")
Expand All @@ -188,8 +205,6 @@ endfunction()

function(z_vcpkg_meson_generate_native_file_config config_type) #https://mesonbuild.com/Native-environments.html
set(native_file "[properties]\n") #https://mesonbuild.com/Builtin-options.html
z_vcpkg_meson_generate_flags_properties_string(native_properties "${config_type}")
string(APPEND native_file "${native_properties}")
#Setup CMake properties
string(APPEND native_file "cmake_toolchain_file = '${SCRIPTS}/buildsystems/vcpkg.cmake'\n")
string(APPEND native_file "[cmake]\n")
Expand Down Expand Up @@ -219,6 +234,8 @@ function(z_vcpkg_meson_generate_native_file_config config_type) #https://mesonbu
string(APPEND native_file "VCPKG_CRT_LINKAGE = '${VCPKG_CRT_LINKAGE}'\n")

string(APPEND native_file "[built-in options]\n")
z_vcpkg_meson_generate_flags_properties_string(native_properties "${config_type}")
string(APPEND native_file "${native_properties}")
Comment thread
ras0219-msft marked this conversation as resolved.
Outdated
if(VCPKG_TARGET_IS_WINDOWS)
if(VCPKG_CRT_LINKAGE STREQUAL "static")
set(crt_type mt)
Expand Down Expand Up @@ -348,9 +365,9 @@ endfunction()

function(z_vcpkg_meson_generate_cross_file_config config_type) #https://mesonbuild.com/Native-environments.html
set(cross_${config_type}_log "[properties]\n") #https://mesonbuild.com/Builtin-options.html
string(APPEND cross_${config_type}_log "[built-in options]\n")
z_vcpkg_meson_generate_flags_properties_string(cross_properties ${config_type})
string(APPEND cross_${config_type}_log "${cross_properties}")
string(APPEND cross_${config_type}_log "[built-in options]\n")
if(VCPKG_TARGET_IS_WINDOWS)
if(VCPKG_CRT_LINKAGE STREQUAL "static")
set(crt_type mt)
Expand Down Expand Up @@ -464,16 +481,6 @@ function(vcpkg_configure_meson)
set(suffix_${buildname} "rel")
endif()

if(VCPKG_TARGET_IS_OSX)
vcpkg_backup_env_variables(VARS SDKROOT MACOSX_DEPLOYMENT_TARGET)

set(ENV{SDKROOT} "${VCPKG_DETECTED_CMAKE_OSX_SYSROOT}")
set(VCPKG_DETECTED_CMAKE_OSX_SYSROOT "${VCPKG_DETECTED_CMAKE_OSX_SYSROOT}" PARENT_SCOPE)

set(ENV{MACOSX_DEPLOYMENT_TARGET} "${VCPKG_DETECTED_CMAKE_OSX_DEPLOYMENT_TARGET}")
set(VCPKG_DETECTED_CMAKE_OSX_DEPLOYMENT_TARGET "${VCPKG_DETECTED_CMAKE_OSX_DEPLOYMENT_TARGET}" PARENT_SCOPE)
endif()

vcpkg_backup_env_variables(VARS INCLUDE)
vcpkg_host_path_list(APPEND ENV{INCLUDE} "${CURRENT_INSTALLED_DIR}/include")
# configure build
Expand Down Expand Up @@ -512,8 +519,5 @@ function(vcpkg_configure_meson)
vcpkg_restore_env_variables(VARS PKG_CONFIG PKG_CONFIG_PATH)
endforeach()

if(VCPKG_TARGET_IS_OSX)
vcpkg_restore_env_variables(VARS SDKROOT MACOSX_DEPLOYMENT_TARGET)
endif()
vcpkg_restore_env_variables(VARS INCLUDE)
endfunction()
2 changes: 1 addition & 1 deletion scripts/get_cmake_vars/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,8 +101,8 @@ macro(_vcpkg_adjust_flags flag_var)
foreach(arch IN LISTS CMAKE_OSX_ARCHITECTURES)
string(APPEND ${flag_var} " -arch ${arch}")
endforeach()
string(APPEND ${flag_var} " -isysroot ${CMAKE_OSX_SYSROOT}")
endif()
string(APPEND ${flag_var} " -isysroot ${CMAKE_OSX_SYSROOT}")

@Neumann-A Neumann-A Dec 2, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this looks like cmake should already be adding the flags (answer to https://github.com/microsoft/vcpkg/pull/21772/files#r760911069 ?)

@daschuer daschuer Dec 2, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately not. It adds the falgs only to the compiler variable but not to the linker. I am afraid we cannot add it to the linker here, because it might break autotools.

@Neumann-A Neumann-A Jan 11, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

might break autotools.

That is not good enough. It either breaks or it doesn't. And if it breaks the question is: 'why?'

endif()
endif()
endmacro()
Expand Down