Skip to content

[vcpkg osx] Fix building for non-x86_64 archs on macOS (OSX) and fixes escapes for some params #12657

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

Closed
Deadpikle wants to merge 7 commits into from
Closed

[vcpkg osx] Fix building for non-x86_64 archs on macOS (OSX) and fixes escapes for some params #12657

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
c8762fa
Attempt to fix arm64 for macOS
Deadpikle Jul 30, 2020
b57ad3f
fix it
strega-nil Jul 30, 2020
80fe695
fix escaping
strega-nil Jul 30, 2020
a8285f6
Remove VCPKG_OSX_ARCHITECTURES from osx triplets
Deadpikle Jul 30, 2020
07b1c75
Add VCPKG_TARGET_IS_IOS
Deadpikle Jul 30, 2020
c611b81
Add multiple arch in VCPKG_TARGET_ARCHITECTURE
Deadpikle Jul 30, 2020
3abcea7
Fix prettify output not working in some cases
Deadpikle Jul 30, 2020
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
1 change: 1 addition & 0 deletions .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -321,6 +321,7 @@ __pycache__/
!triplets/community/x86-ios.cmake
!triplets/community/wasm32-emscripten.cmake
!triplets/community/arm64-osx.cmake
!triplets/community/x64_arm64-osx.cmake
!triplets/arm-uwp.cmake
!triplets/x64-uwp.cmake
!triplets/x64-windows.cmake
Expand Down
2 changes: 2 additions & 0 deletions scripts/cmake/vcpkg_common_definitions.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,8 @@ if (NOT VCPKG_CMAKE_SYSTEM_NAME OR VCPKG_CMAKE_SYSTEM_NAME STREQUAL "WindowsStor
endif()
elseif(VCPKG_CMAKE_SYSTEM_NAME STREQUAL "Darwin")
set(VCPKG_TARGET_IS_OSX 1)
elseif(VCPKG_CMAKE_SYSTEM_NAME STREQUAL "iOS")
set(VCPKG_TARGET_IS_IOS 1)
elseif(VCPKG_CMAKE_SYSTEM_NAME STREQUAL "Linux")
set(VCPKG_TARGET_IS_LINUX 1)
elseif(VCPKG_CMAKE_SYSTEM_NAME STREQUAL "Android")
Expand Down
16 changes: 12 additions & 4 deletions scripts/cmake/vcpkg_configure_cmake.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -211,6 +211,12 @@ function(vcpkg_configure_cmake)
endif()
endif()

# make sure to escape multiple architectures in VCPKG_TARGET_ARCHITECTURE
string(REPLACE ";" "\\;" _VCPKG_TARGET_ARCHITECTURE "${VCPKG_TARGET_ARCHITECTURE}")
Copy link
Copy Markdown
Contributor

@strega-nil strega-nil Jul 30, 2020

Choose a reason for hiding this comment

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

And then here:

Suggested change
string(REPLACE ";" "\\;" _VCPKG_TARGET_ARCHITECTURE "${VCPKG_TARGET_ARCHITECTURE}")
string(REPLACE ";" "\\;" _VCPKG_TARGET_ARCHITECTURE "${VCPKG_TARGET_ARCHITECTURE}")
string(REPLACE "x64" "x86_64" _VCPKG_TARGET_ARCHITECTURE "${_VCPKG_TARGET_ARCHITECTURE}")

thus, we actually write the vcpkg names for these targets, as opposed to the cmake names. I'm not sure if this is the right way, but it makes sense to me... cc @ras0219-msft ?

Copy link
Copy Markdown
Contributor

@strega-nil strega-nil Jul 30, 2020

Choose a reason for hiding this comment

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

The reason I think this is that in x86-windows, we set VCPKG_TARGET_ARCHITECTURE to x86, not Win32.

Copy link
Copy Markdown
Contributor

@strega-nil strega-nil Jul 30, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Also, this should break x64-osx, so these suggestions will fix it; otherwise, we'll have to change x64-osx.

if(VCPKG_TARGET_IS_OSX OR VCPKG_TARGET_IS_IOS)
# VCPKG_TARGET_ARCHITECTURE -> CMAKE_OSX_ARCHITECTURES
list(APPEND _csc_OPTIONS "-DCMAKE_OSX_ARCHITECTURES=${_VCPKG_TARGET_ARCHITECTURE}")
endif()

list(APPEND _csc_OPTIONS
"-DVCPKG_CHAINLOAD_TOOLCHAIN_FILE=${VCPKG_CHAINLOAD_TOOLCHAIN_FILE}"
Expand All @@ -233,7 +239,7 @@ function(vcpkg_configure_cmake)
"-DVCPKG_C_FLAGS_DEBUG=${VCPKG_C_FLAGS_DEBUG}"
"-DVCPKG_CRT_LINKAGE=${VCPKG_CRT_LINKAGE}"
"-DVCPKG_LINKER_FLAGS=${VCPKG_LINKER_FLAGS}"
"-DVCPKG_TARGET_ARCHITECTURE=${VCPKG_TARGET_ARCHITECTURE}"
"-DVCPKG_TARGET_ARCHITECTURE=${_VCPKG_TARGET_ARCHITECTURE}"
Copy link
Copy Markdown
Contributor

@strega-nil strega-nil Jul 30, 2020

Choose a reason for hiding this comment

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

Should this be passed only if !TARGET_IS_OSX?

"-DCMAKE_INSTALL_LIBDIR:STRING=lib"
"-DCMAKE_INSTALL_BINDIR:STRING=bin"
"-D_VCPKG_ROOT_DIR=${VCPKG_ROOT_DIR}"
Expand All @@ -250,7 +256,9 @@ function(vcpkg_configure_cmake)
# Sets configuration variables for macOS builds
foreach(config_var INSTALL_NAME_DIR OSX_DEPLOYMENT_TARGET OSX_SYSROOT OSX_ARCHITECTURES)
if(DEFINED VCPKG_${config_var})
list(APPEND _csc_OPTIONS "-DCMAKE_${config_var}=${VCPKG_${config_var}}")
# support lists in these configuration vars
string(REPLACE ";" "\\;" _csc_VCPKG_CONFIG_VAR "${VCPKG_${config_var}}")
list(APPEND _csc_OPTIONS "-DCMAKE_${config_var}=${_csc_VCPKG_CONFIG_VAR}")
endif()
endforeach()

Expand Down Expand Up @@ -307,7 +315,7 @@ function(vcpkg_configure_cmake)
message(STATUS "Configuring ${TARGET_TRIPLET}-dbg")
file(MAKE_DIRECTORY ${CURRENT_BUILDTREES_DIR}/${TARGET_TRIPLET}-dbg)
vcpkg_execute_required_process(
COMMAND ${dbg_command}
COMMAND "${dbg_command}"
WORKING_DIRECTORY ${CURRENT_BUILDTREES_DIR}/${TARGET_TRIPLET}-dbg
LOGNAME config-${TARGET_TRIPLET}-dbg
)
Expand All @@ -317,7 +325,7 @@ function(vcpkg_configure_cmake)
message(STATUS "Configuring ${TARGET_TRIPLET}-rel")
file(MAKE_DIRECTORY ${CURRENT_BUILDTREES_DIR}/${TARGET_TRIPLET}-rel)
vcpkg_execute_required_process(
COMMAND ${rel_command}
COMMAND "${rel_command}"
WORKING_DIRECTORY ${CURRENT_BUILDTREES_DIR}/${TARGET_TRIPLET}-rel
LOGNAME config-${TARGET_TRIPLET}-rel
)
Expand Down
5 changes: 4 additions & 1 deletion scripts/cmake/vcpkg_execute_required_process.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,9 @@
## * [qt5](https://github.com/Microsoft/vcpkg/blob/master/ports/qt5/portfile.cmake)
include(vcpkg_prettify_command)
function(vcpkg_execute_required_process)
cmake_parse_arguments(vcpkg_execute_required_process "ALLOW_IN_DOWNLOAD_MODE" "WORKING_DIRECTORY;LOGNAME" "COMMAND" ${ARGN})
# cmake_parse_arguments ignores one level of escaping, so make that two levels of escaping
string(REPLACE "\\;" "\\\\;" _ver_ESCAPED_ARGN "${ARGN}")
cmake_parse_arguments(vcpkg_execute_required_process "ALLOW_IN_DOWNLOAD_MODE" "WORKING_DIRECTORY;LOGNAME" "COMMAND" "${_ver_ESCAPED_ARGN}")
set(LOG_OUT "${CURRENT_BUILDTREES_DIR}/${vcpkg_execute_required_process_LOGNAME}-out.log")
set(LOG_ERR "${CURRENT_BUILDTREES_DIR}/${vcpkg_execute_required_process_LOGNAME}-err.log")

Expand All @@ -47,6 +49,7 @@ Halting portfile execution.
]])
endif()


_execute_process(
COMMAND ${vcpkg_execute_required_process_COMMAND}
OUTPUT_FILE ${LOG_OUT}
Expand Down
6 changes: 3 additions & 3 deletions scripts/cmake/vcpkg_prettify_command.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,10 +16,10 @@
macro(vcpkg_prettify_command INPUT_VAR OUTPUT_VAR)
set(${OUTPUT_VAR} "")
foreach(v ${${INPUT_VAR}})
if(${v} MATCHES "( )")
list(APPEND ${OUTPUT_VAR} \"${v}\")
if(v MATCHES "( )")
list(APPEND ${OUTPUT_VAR} "\"${v}\"")
else()
list(APPEND ${OUTPUT_VAR} ${v})
list(APPEND ${OUTPUT_VAR} "${v}")
endif()
endforeach()
list(JOIN ${OUTPUT_VAR} " " ${OUTPUT_VAR})
Expand Down
5 changes: 5 additions & 0 deletions triplets/community/x64_arm64-osx.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
set(VCPKG_TARGET_ARCHITECTURE x86_64 arm64)
Copy link
Copy Markdown
Contributor

@strega-nil strega-nil Jul 30, 2020

Choose a reason for hiding this comment

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

I think this should be:

Suggested change
set(VCPKG_TARGET_ARCHITECTURE x86_64 arm64)
set(VCPKG_TARGET_ARCHITECTURE x64 arm64)

set(VCPKG_CRT_LINKAGE dynamic)
set(VCPKG_LIBRARY_LINKAGE static)

set(VCPKG_CMAKE_SYSTEM_NAME Darwin)