-
-
Notifications
You must be signed in to change notification settings - Fork 33.6k
GH-141362: Make get_externals handle fetching platform-specific release artifacts
#142405
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
base: main
Are you sure you want to change the base?
GH-141362: Make get_externals handle fetching platform-specific release artifacts
#142405
Conversation
PCbuild/get_external.py
Outdated
|
|
||
| def fetch_release(tag, tarball_dir, *, org='python', verbose=False): | ||
| url = f'https://github.com/{org}/cpython-bin-deps/releases/download/{tag}/{tag}.tar.xz' | ||
| arch = platform.machine() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we instead based this on %PreferredToolArchitecture% (will be x86, x64 or ARM64), and possibly update the Exec task in the build files to pass it through? It's set as a build-time variable ($(PreferredToolArchitecture)) but if it's overridden (from the current default, x86) then likely it's set as an environment variable.
This is the same setting we use for all "things that run during build", and is used by MSVC when selecting which version of its own LLVM to use.
(If I'm misunderstanding why we need to select the architecture here now and it's because of the target architecture, not the build machine architecture, then same suggested but use $(Platform) instead.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure PreferredToolArchitecture would give us the right answer here - we need LLVM that can actually run on the build host, not LLVM that matches a tool preference. For example, if someone set PreferredToolArchitecture=x86 on an ARM64 machine, we'd still want ARM64 LLVM since that's what can execute natively. Does that make sense, or am I missing a scenario?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's what PreferredToolArchitecture is for: to specify the architecture of the tool chain that is used for compiling on the host when running msbuild. See https://learn.microsoft.com/en-us/cpp/build/reference/msbuild-visual-cpp-overview?view=msvc-170. Although at the beginning of the page the arm specific locations are specified, ARM64 is missing in the section PreferredToolArchitecture, but looking at
<VS install path>\Community\MSBuild\Microsoft\VC\v160\Microsoft.Cpp.ClangCl.Common.props
we can see that it is indeed used to pick up the tool chain used for compiling:
<_DefaultLLVMInstallDir Condition="'$(_DefaultLLVMInstallDir)' == '' AND '$(PreferredToolArchitecture)' == 'arm64'">$(VsInstallRoot)\VC\Tools\Llvm\ARM64</_DefaultLLVMInstallDir>
<_DefaultLLVMInstallDir Condition="'$(_DefaultLLVMInstallDir)' == '' AND '$(PreferredToolArchitecture)' == 'x64'">$(VsInstallRoot)\VC\Tools\Llvm\x64</_DefaultLLVMInstallDir>
<_DefaultLLVMInstallDir Condition="'$(_DefaultLLVMInstallDir)' == '' AND '$(PreferredToolArchitecture)' != 'x64'">$(VsInstallRoot)\VC\Tools\Llvm</_DefaultLLVMInstallDir>
<LLVMInstallDir Condition="'$(LLVMInstallDir)' == ''">$(_DefaultLLVMInstallDir)</LLVMInstallDir>
Thus, I think that msbuild by default will set PreferredToolArchitecture=ARM64 on a native Windows on Arm machine. Unfortunately, I cannot check, because I don't have access to WoA machine. And if someone specifies PreferredToolArchitecture=x86 on such a machine, that would mean they'd prefer emulation (for whatever reason they'd like to do that).
However, build.bat does not call get_externals.bat in the context of msbuild, so I don't think there is a way to use PreferredToolArchitecture?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm pretty sure PreferredToolArchitecture is still x86 everywhere by default (Platform specifies the target), because it's supported everywhere and any change is a risk (and the team are better at compatibility than to just change it). I don't particularly want to override it ourselves in the config, for much the same reasons, but if we wanted to set variables in CI machine configurations then that'd be fine.
And yeah, you may be right that get_externals being called separately is going to mean that the variable doesn't apply. But that's why I originally suggested using the environment variable, so at least the most likely way to override it (and the way that I usually recommend when people want it) is also going to influence our script. If we do start picking it up this way, then we could more formally document that this is how to choose the build tooling architecture.
But also, just using the x64 tooling everywhere would work fine for our supported range of build architectures. I don't think we care about building on true 32-bit OS anymore.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see - so would it make sense to just check the if PreferredToolArchitecture is set and then fall back to platform.machine() if it isn't?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, or just fall back to x64 (or x86), since that's almost certainly what they'll be getting for all the other tools.
| with: | ||
| python-version: '3.11' | ||
|
|
||
| # test comment to trigger JIT CI |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will remove later :D
Co-authored-by: Steve Dower <steve.dower@microsoft.com>
This PR adds support for downloading platform-specific LLVM binaries (AMD64, ARM64) instead of using a single binary with emulation (see https://github.com/python/cpython-bin-deps/releases/tag/llvm-21.1.4.0 for release artifacts). Since this won't naturally trigger
jit.yml, you can see a successful run here, including print statements to show that we are in fact downloading the correct binary 😄.get_externalshandle fetching platform-specific release artifacts #141362