-
Notifications
You must be signed in to change notification settings - Fork 10.4k
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
Add cross-compiling support for the windows toolchain #71584
Conversation
@swift-ci please build toolchain Windows platform |
1623490
to
7211d06
Compare
@swift-ci please build toolchain Windows platform |
CC: @shahmishal - as we do not currently have a toolchain that has ARM64 SDKs, we need to use the builds from The Browser Company. If we can roll a new release with the changes, we can adjust that URL. |
@swift-ci please build toolchain Windows platform |
utils/build.ps1
Outdated
$PinnedSHA256 = "006266d8c2a6a9c70e21b9d161ec35c07bcbb8a452b17e145899d814d07a29e7" | ||
} | ||
"ARM64" { | ||
$PinnedBuild = "https://github.com/thebrowsercompany/swift-build/releases/download/20231117.3/installer-arm64.exe" |
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.
@shahmishal - this is the only ARM64 installer currently available that will run on ARM64. Since this script is used by developers and we are using it for ARM64 hosted builds, we do need to encode this.
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.
Removed the hardcoded path to the ARM64 installer
utils/build.ps1
Outdated
LLDB_PYTHON_EXE_RELATIVE_PATH = "python.exe"; | ||
LLDB_PYTHON_EXT_SUFFIX = ".pyd"; | ||
LLDB_PYTHON_RELATIVE_PATH = "lib/site-packages"; | ||
LLDB_TABLEGEN = (Join-Path -Path $BuildTools -ChildPath "lldb-tblgen.exe"); | ||
LLVM_CONFIG_PATH = (Join-Path -Path $BuildTools -ChildPath "llvm-config.exe"); | ||
LLVM_EXTERNAL_SWIFT_SOURCE_DIR = "$SourceCache\swift"; | ||
LLVM_INCLUDE_BENCHMARKS = "NO"; |
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.
why do we need not to include the benchmarks now?
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.
This turned out to be unnecessary. Reverted.
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 think that this might be needed still - this avoids unnecessary checks and configuration. I can do the change separately, but the problem was that one of the checks required execution which would not work with cross-compiling.
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 may be right. When I rerun without these changes, the build worked successfully.
6906279
to
dab2f44
Compare
Python3_EXECUTABLE = "$python"; | ||
Python3_INCLUDE_DIR = "$BinaryCache\Python$($Arch.CMakeName)-$PythonVersion\tools\include"; | ||
Python3_LIBRARY = "$BinaryCache\Python$($Arch.CMakeName)-$PythonVersion\tools\libs\python39.lib"; | ||
Python3_ROOT_DIR = "$BinaryCache\Python$($Arch.CMakeName)-$PythonVersion\tools"; | ||
SWIFT_BUILD_SWIFT_SYNTAX = "YES"; |
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 notice you are cross-compiling the new swift-syntax parser too. Do you have further CMake patches to make that work (similar to my #71552), pass in the extra flags manually, or is this disabled elsewhere?
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.
Or is this just cross-compilation support to build SDKs, not to cross-compile the toolchain itself?
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.
This is cross-compiling the toolchain and we did not find a need to add any additional flags.
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.
This is cross-compiling the toolchain
OK
we did not find a need to add any additional flags.
No, I looked into it now and you were passing in extra SDK flags manually before and are still doing so now, as that's the only way this cross-compile works. You should try my cross-compilation pull out with your cross-compilation workflow, as I set the flags for a freshly-built cross-compilation SDK whereas it appears you manually pass in the extra flags for a prebuilt Windows ARM cross-compilation SDK. I think both should work fine together, as externally supplied flags should override the internal ones, but it would be good to verify that.
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.
@finagolfin Can you elaborate what "try my cross-compilation pull out with your cross-compilation workflow" means? How exactly do we "verify that"? I'd be happy to run or test something.
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.
@finagolfin I reran the cross-compile build with your patch #71552 and it was successful with no issues. The log also didn't say "CROSSCOMPILE". So it seems all good.
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.
Great, what bootstrap mode are you using though?
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.
We don't use any bootstrap mode. We use the host tools.
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.
OK, that is one of the bootstrapping modes, as you can see on the official linux CI, -- Bootstrapping: HOSTTOOLS
. Yes, technically that doesn't make sense, but that was the wrong name that was chosen for this internal build variable, instead of something accurate like "Swift compiler source compilation mode."
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.
Gotcha
dab2f44
to
41545df
Compare
@swift-ci please build toolchain Windows platform |
f9bcd16
to
6407d4a
Compare
Rebased |
@swift-ci please build toolchain Windows platform |
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 think that this should be mostly fine, outside of the hardcoded Windows bit which interferes with other work. I suppose that I can try to address that, but I'd definitely appreciate it if you could try to proactively avoid some of that pain.
utils/build.ps1
Outdated
TryAdd-KeyValue $Defines CMAKE_SYSTEM_NAME Windows | ||
TryAdd-KeyValue $Defines CMAKE_SYSTEM_PROCESSOR $Arch.CMakeName |
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.
Hmm, can we hoist this? I think that this might cause some trouble for adding Android support (see #72014).
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.
Done
6407d4a
to
7fdf14f
Compare
@compnerd PTAL. The lastest revision is rebased and passes local cross-compiling tests. |
@swift-ci please test |
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.
LGTM. Eventually it might make sense to have toolchain files and some examples of cache files that can set these variables rather than having a one-size-fits-all script force the values through.
Which variables are you thinking of? I don't think that we want those files because the majority of the flags being passed are just paths that are dependent on how the build is being executed. The other flags that control what and how should be in cache files. The only exception that we currently have to that are the experimental modules. |
CMAKE__COMPILER_TARGET, the CMAKE_SYSTEM_PROCESSOR, etc. a lot of this is forced through the script when the cache and toolchain would do the job nicely. For the system processor, CMake docs explicitly call out using the toolchain file to set that variable. In this case it's fine, I'm just tired of the prescriptive nature of build-script making it really difficult to build things where you do need the customization, and then making it impossible to upstream things without breaking someone else. |
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 think that with the changes for dependencies, this should be ready to go.
7fdf14f
to
208a066
Compare
@swift-ci please smoke test |
Co-authrored with @compnerd
208a066
to
f6ab729
Compare
Rebased and tested now. |
@swift-ci please test |
@swift-ci test windows |
Extend build.ps1 to enable cross-compile the windows toolchain. For example, building the Windows ARM64 toolchain on a Windows X64 machine.