[build] Make it possible to actually build the stdlib with a prebuilt clang #34628
Conversation
| @@ -1653,7 +1653,8 @@ function(add_swift_target_library name) | |||
| list(APPEND SWIFTLIB_SWIFT_COMPILE_FLAGS "-warn-implicit-overrides") | |||
| endif() | |||
|
|
|||
| if(NOT SWIFT_BUILD_RUNTIME_WITH_HOST_COMPILER AND NOT BUILD_STANDALONE) | |||
| if(NOT SWIFT_BUILD_RUNTIME_WITH_HOST_COMPILER AND NOT BUILD_STANDALONE AND | |||
| NOT SWIFT_PREBUILT_CLANG) | |||
There was a problem hiding this comment.
@gottesmm, I'm guessing you passed in --build-runtime-with-host-compiler=1 to the standalone stdlib preset only to keep this from erroring out? This and the next change make that flag no longer necessary.
|
@swift-ci please test stdlib with toolchain |
|
Build failed |
|
Stdlib CI failure is unrelated to this pull, as it's because three stdlib tests failed. That means this pull did what it's supposed to do and built the stdlib with the passed-in clang, and the test failures are because of other problems. |
|
While this appears to work fine when building the standalone stdlib, I think it will need a couple more tweaks for building the entire toolchain. I have kicked off a full build with those tweaks and will update this pull once that's built later today: hold off on review until then. |
finagolfin
force-pushed
the
native-clang
branch
from
f84992a
to
e4ad8bf
Compare
|
@shahmishal and @edymtt, I have pulled in the reverted commit from #34437 and a fix for that issue in a new commit, let me know what you think. As noted in my last comment, I ran into the same issue when cross-compiling the toolchain for Android and this commit e4ad8bf fixed it for me. |
| @@ -467,10 +467,12 @@ if(SWIFT_PATH_TO_CMARK_BUILD) | |||
| endif() | |||
| message(STATUS "") | |||
|
|
|||
| if("${SWIFT_NATIVE_LLVM_TOOLS_PATH}" STREQUAL "") | |||
| set(SWIFT_CROSS_COMPILING FALSE) | |||
There was a problem hiding this comment.
@shahmishal, I believe the only reason this was set was because SWIFT_NATIVE_LLVM_TOOLS_PATH was used to find clang in stdlib/CMakeLists.txt below. However, since that was a mistake that I tried to fix in #34437, I now use this clang path check and SWIFT_PREBUILT_CLANG here and in cmake/modules/SwiftSharedCMakeConfig.cmake instead. I believe this SWIFT_PREBUILT_CLANG is a better variable name as the clang might not just be passed in for cross-compiling, but also for natively building the standalone stdlib, as this pull was originally meant for.
|
@swift-ci build toolchain |
|
@swift-ci test |
| @@ -110,7 +110,8 @@ endif() | |||
| # First extract the "version" used for Clang's resource directory. | |||
| string(REGEX MATCH "[0-9]+\\.[0-9]+(\\.[0-9]+)?" CLANG_VERSION | |||
| "${LLVM_PACKAGE_VERSION}") | |||
| if(NOT SWIFT_INCLUDE_TOOLS AND SWIFT_BUILD_RUNTIME_WITH_HOST_COMPILER) | |||
| if(NOT SWIFT_INCLUDE_TOOLS AND | |||
| (SWIFT_BUILD_RUNTIME_WITH_HOST_COMPILER OR SWIFT_PREBUILT_CLANG)) | |||
There was a problem hiding this comment.
@gottesmm and @shahmishal, should this be using the passed-in prebuilt compiler's resource directory even when SWIFT_INCLUDE_TOOLS is true, ie should this be if(SWIFT_BUILD_RUNTIME_WITH_HOST_COMPILER OR SWIFT_PREBUILT_CLANG) instead? This only applies when building the standalone stdlib now, but since you guys have started cross-compiling the full Mac toolchain with SWIFT_NATIVE_CLANG_TOOLS_PATH set (as do I when cross-compiling the Termux toolchain for Android), I'm not sure which resource directory should be used, the freshly built one that's now used or the one that comes with the prebuilt clang, or if it matters.
There was a problem hiding this comment.
This code path uses CMAKE_C_COMPILER which no longer is setup by build-script-impl now. How is this intended to work? It seems a bit confusing.
There was a problem hiding this comment.
It is still using it, it may help if you look at all three commits in this pull together.
I agree that all the CMAKE_C_COMPILER invocations are confusing, as I explained in #33675 a couple weeks ago (see that original comment for the links too):
The build-script requires a host_cc to be set,
which it looks up in the path or can be passed
in with --host-cc and then it sets CMAKE_{C,CXX}_COMPILER
for all CMake invocations with that host clang/clang++.
Then, several build products override that with
another CMAKE_{C,CXX}_COMPILER invocation,
either set to the freshly built Swift clang or native_clang_tools_path, #32922.
On top of this, you've now added a third CMAKE_{C,CXX}_COMPILER
invocation for all build-script-impl CMake invocations,
including when compiling CMark, LLVM, and Swift.
I'm only removing that third invocation he added in this pull, CMAKE_C_COMPILER is still set for the stdlib using SWIFT_NATIVE_CLANG_TOOLS_PATH in stdlib/CMakeLists.txt in this pull.
| set(SWIFT_PREBUILT_CLANG FALSE) | ||
| else() | ||
| set(SWIFT_PREBUILT_CLANG TRUE) | ||
| endif() |
There was a problem hiding this comment.
Is there a reason to prefer this over just using standard CMAKE_C{,XX}_COMPILER?
There was a problem hiding this comment.
Yes, because the compiler and stdlib are built using potentially three clang compilers: the --host-cc is used to build a native Swift compiler, while the stdlib can also be built with either a freshly built Swift-forked clang that is usually built alongside this repo or a prebuilt clang that is passed in through --native-clang-tools-path. This new variable and SWIFT_BUILD_RUNTIME_WITH_HOST_COMPILER determine which of those three possible clangs to use when building the C++ portions of the stdlib.
| @@ -110,7 +110,8 @@ endif() | |||
| # First extract the "version" used for Clang's resource directory. | |||
| string(REGEX MATCH "[0-9]+\\.[0-9]+(\\.[0-9]+)?" CLANG_VERSION | |||
| "${LLVM_PACKAGE_VERSION}") | |||
| if(NOT SWIFT_INCLUDE_TOOLS AND SWIFT_BUILD_RUNTIME_WITH_HOST_COMPILER) | |||
| if(NOT SWIFT_INCLUDE_TOOLS AND | |||
| (SWIFT_BUILD_RUNTIME_WITH_HOST_COMPILER OR SWIFT_PREBUILT_CLANG)) | |||
There was a problem hiding this comment.
This code path uses CMAKE_C_COMPILER which no longer is setup by build-script-impl now. How is this intended to work? It seems a bit confusing.
|
Linux Toolchain (Ubuntu 16.04) Install command |
|
All CI passes except for the Mac toolchain build, which fails when it tries to cross-compile the swift-frontend for Mac arm64 and then seemingly run that arm64 binary on the Mac x86_64 host: Since this pull is mostly about setting the clang used, that is most likely an external error unrelated to this pull. |
|
@swift-ci build toolchain |
|
Linux Toolchain (Ubuntu 16.04) Install command |
|
macOS Toolchain Install command |
|
Ping, can someone run the CI and toolchain on this again? |
|
@swift-ci build toolchain |
|
Linux Toolchain (Ubuntu 16.04) Install command |
|
macOS Toolchain Install command |
|
@edymtt or @shahmishal, could one of you re-run the CI and merge this? The only remaining review question was whether one block should be expanded, but Michael told me he probably won't get to answering that this week, so we can always add that in a later pull if wanted. |
finagolfin
force-pushed
the
native-clang
branch
from
e4ad8bf
to
e172b2c
Compare
| @@ -167,8 +167,11 @@ def install_toolchain_path(self, host_target): | |||
| """toolchain_path() -> string | |||
|
|
|||
| Returns the path to the toolchain that is being created as part of this | |||
| build. | |||
| build, or to a native prebuilt toolchain that was passed in. | |||
There was a problem hiding this comment.
Rebased and updated this comment.
…-path flags into the Python build-script"" Also, fix two places where the LLVM path was wrongly employed to set up clang, and use the Swift path in install_toolchain_path().
finagolfin
force-pushed
the
native-clang
branch
from
e172b2c
to
934823a
Compare
|
Ping, CI just needs to be run again and this is ready to go. |
|
@varungandhi-apple, mind running the CI again here? The previous pass was before Thanksgiving. |
|
@swift-ci build toolchain |
|
Linux Toolchain (Ubuntu 16.04) Install command |
|
Mac toolchain build fails with some error about not being able to lipo two dylibs that have the same architecture, seems unrelated. |
|
Ping, just needs the CI run again. |
|
@swift-ci build toolchain |
|
Linux Toolchain (Ubuntu 16.04) Install command |
|
macOS Toolchain Install command |
|
@swift-ci smoke test and merge |
|
Thanks for finally getting this in. |
I'm guessing passing in
--native-clang-tools-pathused to work fine to build the stdlib in the beginning years ago, but it had to be overridden with the--build-runtime-with-host-compiler=1flag in recent years, as @gottesmm did with this standalone stdlib preset which I'm now removing.@kubamracek, this should fix the issue you were addressing in #33675 the right way, rather than building all build-script-impl products with the prebuilt clang, so I revert that patch here. This prebuilt clang was only ever used to build the Swift stdlib for years, until I extended it to build the corelibs too in #32922 in July. Extending it to build everything, to fix an issue with the stdlib, seems too aggressive, maybe it can be tested properly and done later.
EDIT: @edymtt - This addresses rdar://71240426