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

[arm64ec] Fix Windows toolchain. #23139

Closed
wants to merge 1 commit into from

Conversation

BillyONeal
Copy link
Member

  • set(CMAKE_TRY_COMPILE_TARGET_TYPE STATIC_LIBRARY) when linking arm64ec static to workaround softintrin as explained in the comment.
  • Switch to using the _INIT versions of flags now that we require CMake >= 3.7 (as of Change minimum CMake version to 3.7.2. #23134 )

* set(CMAKE_TRY_COMPILE_TARGET_TYPE STATIC_LIBRARY) when linking arm64ec static to workaround softintrin as explained in the comment.
* Switch to using the _INIT versions of flags now that we require CMake >= 3.7 (as of microsoft#23134 )
@BillyONeal BillyONeal added info:internal This PR or Issue was filed by the vcpkg team. depends:different-pr This PR or Issue depends on a PR which has been filed labels Feb 16, 2022
@BillyONeal
Copy link
Member Author

I intentionally cancelled the validation build because this triggers a world rebuild and I would like folks to say it isn't entirely broken first.

@BillyONeal
Copy link
Member Author

Probably should be "rolled up" with #22831

@@ -9,6 +9,11 @@ if(NOT _CMAKE_IN_TRY_COMPILE)
set(VCPKG_CRT_LINK_FLAG_PREFIX "/MD")
elseif(VCPKG_CRT_LINKAGE STREQUAL "static")
set(VCPKG_CRT_LINK_FLAG_PREFIX "/MT")
if (VCPKG_TARGET_ARCHITECTURE STREQUAL "arm64ec")
# When linking statically to the CRT, an executable also has to link against softintrin.lib, but that
Copy link
Contributor

Choose a reason for hiding this comment

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

an executable also has to link against softintrin.lib

So should that be added to the exe link flags?

Copy link
Member Author

Choose a reason for hiding this comment

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

In Phil's case the lib isn't available in the environment he wants; it's something that needs to be fixed in the product itself.

@BillyONeal
Copy link
Member Author

@BillyONeal
Copy link
Member Author

See also: #16111 (thanks to Iluin on Discord for pointing these out)

@JackBoosY JackBoosY added the category:vcpkg-bug The issue is with the vcpkg system (including helper scripts in `scripts/cmake/`) label Feb 17, 2022
@Neumann-A
Copy link
Contributor

(thanks to Iluin on Discord for pointing these out)

That is just me on mobile.

Hmm what is actually the plan behind switching to the _INIT variables?

Maybe it is time to have dedicated toolchains instead of one fits it all?

@BillyONeal
Copy link
Member Author

(thanks to Iluin on Discord for pointing these out)

That is just me on mobile.

Ha! Well thank you nonetheless :)

Hmm what is actually the plan behind switching to the _INIT variables?

There isn't really so much of a "plan" here; I just applied the change that some arm64ec folks are using because it looked mostly harmless. Your comments have indicated that it isn't mostly harmless so I don't know on what timescale this is going to land. I'm still a bit hazy on what the end game for this arm64ec feature is.

Maybe it is time to have dedicated toolchains instead of one fits it all?

I have no opinion on such a change.

@BillyONeal BillyONeal marked this pull request as draft February 17, 2022 20:18
@Neumann-A
Copy link
Contributor

@BillyONeal BillyONeal added category:vcpkg-feature The issue is a new capability of the tool that doesn’t already exist and we haven’t committed and removed depends:different-pr This PR or Issue depends on a PR which has been filed category:vcpkg-bug The issue is with the vcpkg system (including helper scripts in `scripts/cmake/`) labels Feb 17, 2022
@BillyONeal BillyONeal closed this Feb 24, 2022
@BillyONeal BillyONeal deleted the arm64ec branch May 23, 2022 22:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:vcpkg-feature The issue is a new capability of the tool that doesn’t already exist and we haven’t committed info:internal This PR or Issue was filed by the vcpkg team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants