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

[vcpkg] vcpkg_configure_make: Support macOS cross-compile #15659

Merged
merged 4 commits into from Jan 29, 2021

Conversation

past-due
Copy link
Contributor

@past-due past-due commented Jan 15, 2021

Describe the pull request

Support macOS cross-compile in vcpkg_configure_make (ex. building an arm64-osx triplet on an x86_64 Mac).

  • What does your PR fix?

Previously, ports like gettext that use vcpkg_configure_make were building successfully, but for the native architecture regardless of the osx triplet used.

So, for example, trying to build gettext:arm64-osx on a 64-bit Intel Mac would actually build an x86_64 architecture library.

This sets the proper flags to ensure the libraries are built for the desired target architecture.
(i.e. Building gettext:arm64-osx on a 64-bit Intel Mac yields an arm64 architecture library.)

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

This should impact cross-compiling osx triplet scenarios.

  • Additional notes

@Neumann-A:

I added all of this logic in vcpkg_configure_make because there are other consumers of vcpkg_internal_get_cmake_vars (like vcpkg_configure_meson) that I haven't had time to look into yet.

Theoretically, -arch ${TARGET_ARCH} -isysroot ${VCPKG_DETECTED_CMAKE_OSX_SYSROOT} could be added to CFLAGS / CXXFLAGS in get_cmake_vars, but, for example, meson appears to use some sort of "cross file" that specifies architectures, so I'm unsure whether it should get this addition to CFLAGS / CXXFLAGS?

@JackBoosY JackBoosY self-assigned this Jan 15, 2021
@JackBoosY JackBoosY added the category:tool-update The issue is with build tool or build script, which requires update or should be executed correctly label Jan 15, 2021
@JackBoosY
Copy link
Contributor

cc @Neumann-A for review this PR.

@Neumann-A
Copy link
Contributor

Theoretically, -arch ${TARGET_ARCH} -isysroot ${VCPKG_DETECTED_CMAKE_OSX_SYSROOT} could be added to CFLAGS / CXXFLAGS in get_cmake_vars, but, for example, meson appears to use some sort of "cross file" that specifies architectures, so I'm unsure whether it should get this addition to CFLAGS / CXXFLAGS?

we are running meson in --plain mode so it probably also requires those flags.

@Neumann-A
Copy link
Contributor

An what happens if you add the settings to the compiler flags in the toolchain? Does CMake deduplicate the flags?

@past-due
Copy link
Contributor Author

past-due commented Jan 15, 2021

An what happens if you add the settings to the compiler flags in the toolchain? Does CMake deduplicate the flags?

@Neumann-A:
Just tested this, and CMake does not de-duplicate the flags. Thus putting them in the toolchain isn't a satisfactory option.

we are running meson in --plain mode so it probably also requires those flags.

Testing fribidi:arm64-osx (which uses vcpkg_configure_meson) yields the same issue as gettext:arm64-osx when building on an Intel64 Mac - it builds an x86_64 arch library.

So I've moved this compiler flags logic to get_cmake_vars. Additionally, I wrote it in a way that it should (theoretically) support "universal" (multi-arch) osx libraries, if and when the rest of vcpkg adds full support for this.

EDIT: Note: There still seem to be additional issues preventing meson builds from properly building for the target arch, but I may not have time to dig into that for a while (and may be best left to a subsequent PR). As-is, this PR should now fix many ports that use vcpkg_configure_make and vcpkg_build_make (quite a few ports) when building for a non-native Mac arch.

@past-due past-due marked this pull request as ready for review January 15, 2021 15:31
@past-due
Copy link
Contributor Author

I don't believe the single CI failure is related to this PR?

REGRESSION: mlpack:x86-windows. If expected, add mlpack:x86-windows=fail to .\scripts\ci.baseline.txt

@past-due
Copy link
Contributor Author

@NancyLi1013 @JackBoosY The gettext failure on Windows appears to be also happening on the master branch: https://github.com/microsoft/vcpkg/runs/1716299865

@JackBoosY
Copy link
Contributor

gettext does have regression on master, but the reason is different from this:

-- Building arm64-windows-dbg
CMake Error at scripts/cmake/vcpkg_build_make.cmake:205 (message):
  libtool could not find a file being linked against!
Call Stack (most recent call first):
  scripts/cmake/vcpkg_install_make.cmake:26 (vcpkg_build_make)
  ports/gettext/portfile.cmake:59 (vcpkg_install_make)
  scripts/ports.cmake:137 (include)

@Neumann-A
Copy link
Contributor

The real error is:

*** Warning: linker path does not have real file for library -liconv.
*** I have the capability to make that library automatically link in when
*** you link to this library.  But I can only do this if you have a
*** shared version of the library, which you do not appear to have
*** because I did check the linker path looking for a file starting
*** with iconv and none of the candidates passed a file format test
*** using a file magic. Last file checked: /usr/bin/iconv.exe
*** The inter-library dependencies that have been dropped here will be
*** automatically added whenever a program is linked with this library
*** or is declared to -dlopen it.

*** Since this library must not contain undefined symbols,
*** because either the platform does not support them or
*** it was explicitly requested with -no-undefined,
*** libtool will only create a static version of it.

@past-due
Copy link
Contributor Author

past-due commented Jan 18, 2021

@Neumann-A @JackBoosY I can reproduce this same exact error with a fresh checkout of latest master, attempting to build gettext:arm64-windows (on x64 Windows 10 + MSVC 2019).

A git bisect reveals that:
3ddaeb4 - works
af3c99b - breaks gettext:arm64-windows

Created a new Issue: #15737

@JackBoosY JackBoosY added the depends:different-pr This PR or Issue depends on a PR which has been filed label Jan 19, 2021
@JackBoosY
Copy link
Contributor

Waiting for #15740.

@JackBoosY JackBoosY removed the depends:different-pr This PR or Issue depends on a PR which has been filed label Jan 20, 2021
@past-due
Copy link
Contributor Author

@JackBoosY Anything else needed to get this merged? It's now working well on CI (and in my tests).

(I have some follow-up PRs that depend on this one to fix specific ports.)

@JackBoosY
Copy link
Contributor

Should be good to me.

@JackBoosY JackBoosY added the info:reviewed Pull Request changes follow basic guidelines label Jan 21, 2021
@past-due
Copy link
Contributor Author

past-due commented Jan 25, 2021

@dan-shaw: Please let me know if you have any questions that I can answer before this can be merged. (With the recent conversion of the harfbuzzport to meson (cc093a8), many more ports are now broken on arm64-osx, and this PR is a pre-requisite for a future PR (edit: now opened as #15867) to fix meson builds on arm64-osx.)

Co-authored-by: Billy O'Neal <bion@microsoft.com>
@dan-shaw
Copy link
Contributor

LGTM, ready to merge once CI is passing

@dan-shaw
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@past-due
Copy link
Contributor Author

LGTM, ready to merge once CI is passing

@dan-shaw: Looks like the remaining CI failure is unrelated to this PR. (Seems a version info update was missed in 5546681.)

@JackBoosY
Copy link
Contributor

Regression will be fixed in #15933.

@dan-shaw dan-shaw merged commit 7115ef4 into microsoft:master Jan 29, 2021
@PhoebeHui PhoebeHui changed the title vcpkg_configure_make: Support macOS cross-compile [vcpkg] vcpkg_configure_make: Support macOS cross-compile Feb 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:tool-update The issue is with build tool or build script, which requires update or should be executed correctly info:reviewed Pull Request changes follow basic guidelines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants