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

[python3] Add patch to fix Windows 11 SDK build failure. #20292

Merged
merged 3 commits into from
Sep 29, 2021

Conversation

Hoikas
Copy link
Contributor

@Hoikas Hoikas commented Sep 22, 2021

This adds a patch to work around a compile failure introduced by the Windows SDK 10.0.22000.0. The patch introduced by upstream at python/cpython#28393 is insufficient because vcpkg seems to prefer the usage of the latest (or whatever is set by the environment) Windows SDK. As of actions/runner-images#4014, this issue affects anyone trying to build the python3 port on the GitHub Actions windows-2019 (aka windows-latest) runners.

The specific problem is in the Windows SDK's um/winnt.h file. In 10.0.22000.0, it became an error if SYSTEM_CACHE_ALIGNMENT_SIZE could not be determined. It appears that the RC compiler does not perform certain defines that allow this to succeed. Here are the problematic lines in 10.22000.0:

#ifndef SYSTEM_CACHE_ALIGNMENT_SIZE
#if defined(_AMD64_) || defined(_X86_)
#define SYSTEM_CACHE_ALIGNMENT_SIZE X86_CACHE_ALIGNMENT_SIZE
#elif defined(_ARM64_) || defined(_ARM_)
#define SYSTEM_CACHE_ALIGNMENT_SIZE ARM_CACHE_ALIGNMENT_SIZE
#else
#error Must define a target architecture.
#endif
#endif // SYSTEM_CACHE_ALIGNMENT_SIZE

Previously, this implicitly succeeded, as seen in 10.0.18362.0's um/winnt.h:

#ifndef SYSTEM_CACHE_ALIGNMENT_SIZE
#if defined(_AMD64_) || defined(_X86_)
#define SYSTEM_CACHE_ALIGNMENT_SIZE 64
#else
#define SYSTEM_CACHE_ALIGNMENT_SIZE 128
#endif
#endif

Therefore I have added a patch that sets SYSTEM_CACHE_ALIGNMENT_SIZE to the x86 value when the resource compiler is invoked.

If you are still working on the PR, open it as a Draft: https://github.blog/2019-02-14-introducing-draft-pull-requests/

@Hoikas Hoikas marked this pull request as draft September 22, 2021 19:31
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

This is a new experimental fast check for PR issues. Please let us know if this bot is helpful!

After committing all other changes, the version database must be updated
git add -u && git commit
git checkout 451d6664e44156035c59cf8a67df91650f51b80b -- versions
./vcpkg x-add-version --all
Diff
diff --git a/versions/p-/python3.json b/versions/p-/python3.json
index e9972c5..719d238 100644
--- a/versions/p-/python3.json
+++ b/versions/p-/python3.json
@@ -1,7 +1,7 @@
 {
   "versions": [
     {
-      "git-tree": "685ca96439931362657ed1410627861e97155267",
+      "git-tree": "ba36ad8882cebd37de0409abe83dcf043b62e27e",
       "version-semver": "3.9.7",
       "port-version": 1
     },

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

This is a new experimental fast check for PR issues. Please let us know if this bot is helpful!

After committing all other changes, the version database must be updated
git add -u && git commit
git checkout 9d892ff9270f2d2fe3b8e490471f0a7dfa78981e -- versions
./vcpkg x-add-version --all
Diff
diff --git a/versions/p-/python3.json b/versions/p-/python3.json
index e9972c5..726088e 100644
--- a/versions/p-/python3.json
+++ b/versions/p-/python3.json
@@ -1,7 +1,7 @@
 {
   "versions": [
     {
-      "git-tree": "685ca96439931362657ed1410627861e97155267",
+      "git-tree": "f45cd41d5c734f0736c7fbfa96fdfb076497c359",
       "version-semver": "3.9.7",
       "port-version": 1
     },

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

This is a new experimental fast check for PR issues. Please let us know if this bot is helpful!

After committing all other changes, the version database must be updated
git add -u && git commit
git checkout 9d892ff9270f2d2fe3b8e490471f0a7dfa78981e -- versions
./vcpkg x-add-version --all
Diff
diff --git a/versions/p-/python3.json b/versions/p-/python3.json
index e9972c5..f4294d7 100644
--- a/versions/p-/python3.json
+++ b/versions/p-/python3.json
@@ -1,7 +1,7 @@
 {
   "versions": [
     {
-      "git-tree": "685ca96439931362657ed1410627861e97155267",
+      "git-tree": "29b6b1f6fd4216b50c261c162140cb26f6c46d0f",
       "version-semver": "3.9.7",
       "port-version": 1
     },

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

This is a new experimental fast check for PR issues. Please let us know if this bot is helpful!

After committing all other changes, the version database must be updated
git add -u && git commit
git checkout 9d892ff9270f2d2fe3b8e490471f0a7dfa78981e -- versions
./vcpkg x-add-version --all
Diff
diff --git a/versions/p-/python3.json b/versions/p-/python3.json
index e9972c5..726088e 100644
--- a/versions/p-/python3.json
+++ b/versions/p-/python3.json
@@ -1,7 +1,7 @@
 {
   "versions": [
     {
-      "git-tree": "685ca96439931362657ed1410627861e97155267",
+      "git-tree": "f45cd41d5c734f0736c7fbfa96fdfb076497c359",
       "version-semver": "3.9.7",
       "port-version": 1
     },

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

This is a new experimental fast check for PR issues. Please let us know if this bot is helpful!

After committing all other changes, the version database must be updated
git add -u && git commit
git checkout 9d892ff9270f2d2fe3b8e490471f0a7dfa78981e -- versions
./vcpkg x-add-version --all
Diff
diff --git a/versions/baseline.json b/versions/baseline.json
index c977d55..43971ad 100644
--- a/versions/baseline.json
+++ b/versions/baseline.json
@@ -5286,7 +5286,7 @@
     },
     "python3": {
       "baseline": "3.9.7",
-      "port-version": 0
+      "port-version": 1
     },
     "qca": {
       "baseline": "2.3.1",
diff --git a/versions/p-/python3.json b/versions/p-/python3.json
index 34194e0..b4fc820 100644
--- a/versions/p-/python3.json
+++ b/versions/p-/python3.json
@@ -1,5 +1,10 @@
 {
   "versions": [
+    {
+      "git-tree": "6a1011c32aa2eb7de0b9a6184b2805e34c41f0b5",
+      "version-semver": "3.9.7",
+      "port-version": 1
+    },
     {
       "git-tree": "b3a545b39c982b7f0a12891765dd9909e364ebc6",
       "version-semver": "3.9.7",

@Hoikas Hoikas marked this pull request as ready for review September 23, 2021 01:41
@JackBoosY JackBoosY added the category:port-bug The issue is with a library, which is something the port should already support label Sep 23, 2021
@JackBoosY
Copy link
Contributor

I would like @BillyONeal to review this PR.

@Hoikas Hoikas changed the title [python3] Add patch from upstream to fix Windows SDK detection. [python3] Add patch to fix Windows 11 SDK build failure. Sep 23, 2021
@vi3itor
Copy link

vi3itor commented Sep 23, 2021

Hi @Hoikas! Thank you for the PR and the detailed description of the underlying problem. You saved me at least several hours.

I've been using manifests and its overrides feature to build boost-python with Python 3.6, 3.7, 3.8, and 3.9. It all worked well until last week, when the SDK was added. Your patch will fix the problem for Python 3.9, but do you have any suggestions on how can I tackle this issue for the earlier versions of Python without forking vcpkg and copying the patch or using windows-2016? I was thinking about defining the environment variable WindowsSDKVersion, but it didn't help.

The installation of the packages is triggered in the CMakeLists.txt, and I found a CMake's variable CMAKE_VS_WINDOWS_TARGET_PLATFORM_VERSION_MAXIMUM, but it has no effect on the process of building Python. My workflow file is here. I don't use preinstalled vcpkg, instead I checkout it manually with fetch-depth: 0 to fetch all the history.

If you have any suggestions, please let me know. Thank you for your time!

@Hoikas
Copy link
Contributor Author

Hoikas commented Sep 23, 2021

You raise a good question @vi3itor - we were discussing using that same environment variable yesterday in discord and also found that it didn't work. Ideally, vcpkg would have a way that would let you specify a specific Windows SDK for issues like this, but I don't know what that is or would be. Unless anyone else has a better idea, forking and patching might be the best way to go 😞. I'm sorry I don't have any other ideas right now.

@vi3itor
Copy link

vi3itor commented Sep 24, 2021

Thank you Adam for the reply! Defining all the environment variables that mention 10.0.22000.0 in advance doesn't work.

@JackBoosY @BillyONeal I hope vcpkg will provide a way to select Windows SDK for my use case (manifest mode with version overrides).

@Neumann-A
Copy link
Contributor

@Hoikas
Env passthrough variables are define here:
https://github.com/microsoft/vcpkg-tool/blob/c0485c3e53dce45166ea7d15cfe4c94f028e7d95/src/vcpkg/base/system.process.cpp#L262-L331
WindowsSDKVersion is not one of those so you need to append WindowsSDKVersion to VCPKG_ENV_PASSTHROUGH, set it explicitly in the triplet or add it to the tool.

@vi3itor
Copy link

vi3itor commented Sep 24, 2021

Thank you Alexander for pointing it out!

When running install with --debug flag, I found the following environment variables pointing to the new SDK:
WindowsSDKVersion
WindowsSDKLibVersion
WindowsSdkVerBinPath
WindowsLibPath
UCRTVersion

But I'm not sure which of them are generated based on the others.

@BillyONeal BillyONeal merged commit f819f66 into microsoft:master Sep 29, 2021
@BillyONeal
Copy link
Member

Thanks for fixing Actions!

@Hoikas Hoikas deleted the python3-fix-win-sdk branch November 15, 2021 07:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:port-bug The issue is with a library, which is something the port should already support
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[python3:x64-windows] [windows sdk 10.0.22000.0] build failure
5 participants