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 scripts] toolchain/osx: Fix CMAKE_HOST_SYSTEM_PROCESSOR condition with lower case arm64 #40503

Merged

Conversation

WangWeiLin-MV
Copy link
Contributor

@WangWeiLin-MV WangWeiLin-MV commented Aug 16, 2024

Fix #40474

CMake STREQUAL is case-sensitive, change ARM64 to arm64 https://cmake.org/cmake/help/latest/variable/CMAKE_APPLE_SILICON_PROCESSOR.html

@WangWeiLin-MV WangWeiLin-MV added info:internal This PR or Issue was filed by the vcpkg team. category:tool-update The issue is with build tool or build script, which requires update or should be executed correctly labels Aug 16, 2024
@WangWeiLin-MV
Copy link
Contributor Author

WangWeiLin-MV commented Aug 16, 2024

Additional

There is an issue that it seems that this optimization has not taken effect:

  • The host dependency will still build
  • In port file, VCPKG_CROSSCOMPILING is still 1 (TRUE). So the port still requires being passed tools path for running.
  • In toolchain file, CMAKE_CROSSCOMPILING is set to empty. The package will build tools, and the passed tool path will be ignored.

Currently, the additional host dependencies could be avoided by specifying --host-triplet to same with target triplet. However, this method will fail if the host dependency does not support the target architecture, for example arm64.

Possible impact: When the option --host-triplet is set same with target, CMAKE_CROSSCOMPILING is TRUE while VCPKG_CROSSCOMPILING is FALSE.


Quote from previous discussion #40474 (comment)

If the port does not depend on CMAKE_CROSSCOMPILING and depend independent tools port or provide options, then the original issue could be easily solved:

Just add both host and target tools.

${CURRENT_HOST_INSTALLED_DIR} will point to the correct host program.
The additional target-triplet tools are available to users
Just like -DPKG_CONFIG_EXECUTABLE=${PKGCONFIG}.

Some ports do not provide options, and builds many internal tools in-place, it could be better handled by upstream for that difference behavior of cross-compiling.


Example from https://web.archive.org/web/20240813003145/https://cmake.org/cmake/help/book/mastering-cmake/chapter/Cross%20Compiling%20With%20CMake.html

if(CMAKE_CROSSCOMPILING)
   find_package(MyGen)
else()
   add_executable(mygen gen.c)
   export(TARGETS mygen FILE
          "${CMAKE_BINARY_DIR}/MyGenConfig.cmake")
endif()

add_custom_command(
  OUTPUT "${CMAKE_CURRENT_BINARY_DIR}/generated.h"
  COMMAND mygen -o "${CMAKE_CURRENT_BINARY_DIR}/generated.h"
  )

Reference

@dg0yt
Copy link
Contributor

dg0yt commented Aug 17, 2024

I see that the old line didn't make sense, but the new behaviour might not be desired either. Basically, I would suggest to let a cross build be a cross build.

At least for ports, there are no CPU-based short-cuts. When the target triplet is different from the host triplet, it is "host": true, !native, in manifest, and VCPKG_CROSSCOMPILING, in portfile. So when this line is reached for the target triplet, the host tools are already built.

@WangWeiLin-MV WangWeiLin-MV marked this pull request as ready for review August 19, 2024 09:59
@WangWeiLin-MV
Copy link
Contributor Author

I see that the old line didn't make sense, but the new behaviour might not be desired either. Basically, I would suggest to let a cross build be a cross build.

In that case, it will make "CMAKE_CROSSCOMPILING" true even if "--host-triplet" is specified.

@WangWeiLin-MV WangWeiLin-MV changed the title toolchain/osx: Fix CMAKE_HOST_SYSTEM_PROCESSOR condition with lower case arm64 [vcpkg scripts] toolchain/osx: Fix CMAKE_HOST_SYSTEM_PROCESSOR condition with lower case arm64 Aug 19, 2024
@dg0yt
Copy link
Contributor

dg0yt commented Aug 19, 2024

In that case, it will make "CMAKE_CROSSCOMPILING" true even if "--host-triplet" is specified.

This particular toolchain file sets CMAKE_CROSSCOMPILING to OFF for a case which might looks like meant for target_triplet == host_triplet:

if(CMAKE_HOST_SYSTEM_NAME STREQUAL "Darwin")
if(CMAKE_SYSTEM_PROCESSOR STREQUAL CMAKE_HOST_SYSTEM_PROCESSOR)
set(CMAKE_CROSSCOMPILING OFF CACHE STRING "")

But vcpkg doesn't control CMAKE_HOST_SYSTEM_PROCESSOR... but CMake does, for macOS: https://cmake.org/cmake/help/latest/variable/CMAKE_HOST_SYSTEM_PROCESSOR.html#macos-platforms

I can't tell how far one would get with running x64 build tools on arm64 osx but this would clearly remove the crosscompiling from targeting x64 osx.

And everything else could be subeject to a specific toolchain instead of vcpkg's generic one.

Cheney-W
Cheney-W previously approved these changes Aug 21, 2024
@Cheney-W Cheney-W added the info:reviewed Pull Request changes follow basic guidelines label Aug 21, 2024
Copy link
Contributor

@dg0yt dg0yt left a comment

Choose a reason for hiding this comment

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

Given that it didn't work in the past, I would prefer to not introduce it now.

scripts/toolchains/osx.cmake Show resolved Hide resolved
@Cheney-W Cheney-W removed the info:reviewed Pull Request changes follow basic guidelines label Aug 22, 2024
@WangWeiLin-MV WangWeiLin-MV force-pushed the toolchains/macOS-arm64-lower-case branch from 5c33fff to 07a1cdf Compare August 23, 2024 08:21
@WangWeiLin-MV WangWeiLin-MV changed the title [vcpkg scripts] toolchain/osx: Fix CMAKE_HOST_SYSTEM_PROCESSOR condition with lower case arm64 [vcpkg scripts] Restore original intention of CMAKE_CROSSCOMPILING Aug 23, 2024
@WangWeiLin-MV WangWeiLin-MV marked this pull request as draft August 26, 2024 09:00
@WangWeiLin-MV
Copy link
Contributor Author

WangWeiLin-MV commented Aug 26, 2024

After not setting CMAKE_CROSSCOMPILING empty, x86-windows fails with 24 error ports and 76 cascaded ports.

SUMMARY FOR x86-windows
SUCCEEDED: 2138
BUILD_FAILED: 24
POST_BUILD_CHECKS_FAILED: 1
CASCADED_DUE_TO_MISSING_DEPENDENCIES: 76

It is basically be caused that these packages have different behavior depending on the variable CMAKE_CROSSCOMPILING.

Temporarily suspended modifications.

@WangWeiLin-MV WangWeiLin-MV force-pushed the toolchains/macOS-arm64-lower-case branch from 07a1cdf to 60671a8 Compare August 26, 2024 10:16
@dg0yt
Copy link
Contributor

dg0yt commented Aug 26, 2024

This is expected. The x86 windows hack hides the poor cross-build capabilities of several ports. The android triplets where the first real cross builds in CI.

@WangWeiLin-MV WangWeiLin-MV marked this pull request as ready for review August 28, 2024 07:34
Cheney-W
Cheney-W previously approved these changes Aug 29, 2024
@Cheney-W Cheney-W added the info:reviewed Pull Request changes follow basic guidelines label Aug 29, 2024
@BillyONeal
Copy link
Member

This was intentionally changed in #26240 with the expectation that arm64 macs can run x64 tools, and setting it to off can allow some ports to work that would not otherwise. It isn't clear to me that rolling this change back as this PR does will lead to a better user experience; in particular, it isn't clear that it fixes the original reported issue in #40474 .

Is the real problem that we need to supply a native arm64 cmake?

When cross compiling, setting CMAKE_CROSSCOMPILING to empty leads confusion.

set(CMAKE_CROSSCOMPILING OFF CACHE STRING "")

sets CMAKE_CROSSCOMPILING to OFF, not "empty". The "" is the "doc string"

@BillyONeal BillyONeal added requires:vcpkg-team-review This PR or issue requires someone on the vcpkg team to take a further look. and removed info:reviewed Pull Request changes follow basic guidelines labels Sep 10, 2024
@BillyONeal
Copy link
Member

@AugP @vicroms @JavierMatosD @data-queue @ras0219-msft We aren't sure how this change fixes the problems described in #40474 and agree with the original design intent clearing CMAKE_CROSSCOMPILING at this time. That said, if it's better explained why this fixes the original problem, we would consider changing the existing CMAKE_CROSSCOMPILING design.

@BillyONeal
Copy link
Member

Speaking personally, I think if we were to make this change on macOS we should make similar changes for Windows, and that is going to break a lot of users. So we need a big carrot to go with that stick.

@BillyONeal BillyONeal marked this pull request as draft September 12, 2024 22:22
@BillyONeal BillyONeal removed the requires:vcpkg-team-review This PR or issue requires someone on the vcpkg team to take a further look. label Sep 12, 2024
@WangWeiLin-MV
Copy link
Contributor Author

This is expected. The x86 windows hack hides the poor cross-build capabilities of several ports. The android triplets where the first real cross builds in CI.

Speaking personally, I think if we were to make this change on macOS we should make similar changes for Windows, and that is going to break a lot of users. So we need a big carrot to go with that stick.

As @dg0yt said, there are many ports depends on this hack for x86 windows. I'll try to fix these portability issues.


We aren't sure how this change fixes the problems described in #40474

I'll change this PR to fix the original issue.

@WangWeiLin-MV WangWeiLin-MV changed the title [vcpkg scripts] Restore original intention of CMAKE_CROSSCOMPILING [vcpkg scripts] toolchain/osx: Fix CMAKE_HOST_SYSTEM_PROCESSOR condition with lower case arm64 Sep 13, 2024
@WangWeiLin-MV
Copy link
Contributor Author

WangWeiLin-MV commented Sep 13, 2024

Failed ports on x86-windows cross compiling

  • ampl-asl:x86-windows [ampl-asl] Update to 20240201 and disable generated header when cross-compiling #40958
  • aws-sdk-cpp:x86-windows
  • blas-test:x86-windows
  • clamav:x86-windows
  • cmake-user:x86-windows
  • cppmicroservices:x86-windows
  • dcmtk:x86-windows
  • discount:x86-windows
  • fastrtps:x86-windows
  • forge:x86-windows
  • gamedev-framework:x86-windows
  • halide:x86-windows
  • itk:x86-windows
  • kf5plotting:x86-windows
  • kf5sonnet:x86-windows
  • kf5syntaxhighlighting:x86-windows
  • kfr:x86-windows
  • lapack-test:x86-windows
  • libfreenect2:x86-windows
  • ois:x86-windows
  • openblas:x86-windows
  • opencc:x86-windows
  • plplot:x86-windows
  • qtactiveqt:x86-windows
  • salome-medcoupling:x86-windows
  • torch-th:x86-windows
  • vulkan-loader:x86-windows

@WangWeiLin-MV WangWeiLin-MV marked this pull request as ready for review September 19, 2024 02:33
@Cheney-W Cheney-W added the info:reviewed Pull Request changes follow basic guidelines label Sep 19, 2024
@BillyONeal
Copy link
Member

Thanks to @JavierMatosD for testing this on an arm64 macOS device for me. It fixes things to have the intended behavior for him.

@BillyONeal BillyONeal merged commit fec650a into microsoft:master Sep 19, 2024
16 checks passed
@dg0yt
Copy link
Contributor

dg0yt commented Sep 19, 2024

And it makes vcpkg build host tools for the arm64-osx host triplet but not reliably use them when targeting x64-osx, leaving more port bugs hidden.
(Yes, I still think these lines should be removed!)

@WangWeiLin-MV WangWeiLin-MV deleted the toolchains/macOS-arm64-lower-case branch September 20, 2024 02:09
@WangWeiLin-MV
Copy link
Contributor Author

@dg0yt Except the ports that can be disabled cross-compiling check, for the other ports that require tools to be compiled and run in-place , we will try make these depend on their own host ports. Subsequent fixes will be updated in the list above for the time being.

@dg0yt
Copy link
Contributor

dg0yt commented Sep 20, 2024

I don't believe this will happen.
This PR added a shortcut which wasn't needed in the past, and it shouldn't be needed in the future. Except for hiding poor port quality.

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:internal This PR or Issue was filed by the vcpkg team. info:reviewed Pull Request changes follow basic guidelines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[qt] Cross-compiled build results in "moc" tool targeting the host architecture
5 participants