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

[build-script] Move --native-{clang,llvm,swift}-tools-path flags into the Python build-script #34437

Merged
merged 1 commit into from
Nov 9, 2020

Conversation

finagolfin
Copy link
Member

Also, fix two places where the LLVM path was wrongly employed to set up clang, and use the Swift path in install_toolchain_path().

This is a continuation of #32922, which enabled building all the corelibs with a prebuilt toolchain, to non-build-script-impl products like the package manager. I applied this small patch to completely skip building any LLVM or corelibs targets and I was able to build and test SPM from the Oct. 22 source snapshot with the official prebuilt snapshot toolchain for Ubuntu:

diff --git a/utils/build-script b/utils/build-script
index 639f790fe3..2d208781e1 100755
--- a/utils/build-script
+++ b/utils/build-script
@@ -321,7 +321,7 @@ def apply_default_arguments(toolchain, args):
     # SwiftPM and XCTest have a dependency on Foundation.
     # On OS X, Foundation is built automatically using xcodebuild.
     # On Linux, we must ensure that it is built manually.
-    if ((args.build_swiftpm or args.build_xctest) and
+    if ((args.build_swift or args.build_xctest) and
             platform.system() != "Darwin"):
         args.build_foundation = True
 
diff --git a/utils/build-script-impl b/utils/build-script-impl
index 185f6bf555..d6a4e01fd0 100755
--- a/utils/build-script-impl
+++ b/utils/build-script-impl
@@ -2456,7 +2456,7 @@ for host in "${ALL_HOSTS[@]}"; do
         # some LLVM tools like TableGen. In the LLVM configure rules
         # above, a small subset of LLVM build_targets are selected
         # when SKIP_BUILD is set.
-        if [[ $(not ${SKIP_BUILD}) || "${product}" == "llvm" ]]; then
+        if [[ $(not ${SKIP_BUILD}) && "${product}" != "llvm" ]]; then
             if [[ "${CMAKE_GENERATOR}" == "Xcode" ]] ; then
                 # Xcode generator uses "ALL_BUILD" instead of "all".
                 # Also, xcodebuild uses -target instead of bare names.

Then I ran this build-script invocation:

PATH=/home/butta/swift/swift-DEVELOPMENT-SNAPSHOT-2020-10-22-a-ubuntu20.04/usr/bin:$PATH
./swift/utils/build-script -R --no-assertions --skip-build-cmark --skip-build-llvm --skip-build-swift
--native-swift-tools-path swift-DEVELOPMENT-SNAPSHOT-2020-10-22-a-ubuntu20.04/usr/bin -p -T --install-swiftpm

This makes it very easy to build and test several Swift repos with a prebuilt Swift toolchain, rather than having to spend hours building the compiler for any given snapshot.

set(CMAKE_CXX_COMPILER "${SWIFT_NATIVE_LLVM_TOOLS_PATH}/clang-cl")
set(CMAKE_C_COMPILER "${SWIFT_NATIVE_LLVM_TOOLS_PATH}/clang-cl")
set(CMAKE_CXX_COMPILER "${SWIFT_NATIVE_CLANG_TOOLS_PATH}/clang-cl")
set(CMAKE_C_COMPILER "${SWIFT_NATIVE_CLANG_TOOLS_PATH}/clang-cl")
Copy link
Member Author

@finagolfin finagolfin Oct 26, 2020

Choose a reason for hiding this comment

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

Obviously this means those currently passing in these flags and giving a different LLVM path than the clang path may now have to change their flag, but since these flags were never documented in the build-script help till now, that's probably just @compnerd and me.

… 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().
@compnerd
Copy link
Member

I want to play with this change a bit, but, from a quick look, I really like this idea. A +1 on the idea at the very least. If @gottesmm beats me to the review, that's fine, don't hold this up on my behalf, Id rather get this cleanup merged.

@gottesmm
Copy link
Contributor

gottesmm commented Nov 6, 2020

I am fine with this in principle. I just want to check that it does not break the minimal config.

@gottesmm
Copy link
Contributor

gottesmm commented Nov 6, 2020

@swift-ci please test stdlib with toolchain

@gottesmm
Copy link
Contributor

gottesmm commented Nov 6, 2020

preset=stdlib_S_standalone_minimal_macho_x86_64,build,test
@swift-ci please test with toolchain and preset

@swift-ci
Copy link
Contributor

swift-ci commented Nov 6, 2020

Build failed
Swift Test OS X Platform
Git Sha - 9ed6766

@finagolfin
Copy link
Member Author

macOS stdlib CI failure is unrelated to this patch:

/Users/buildnode/jenkins/workspace/swift-PR-stdlib-with-toolchain-osx/branch-main/swift/stdlib/public/Concurrency/Task.swift:136:7: error: module 'Builtin' has no member named 'cancelAsyncTask'

@gottesmm
Copy link
Contributor

gottesmm commented Nov 6, 2020

@swift-ci test

@finagolfin
Copy link
Member Author

Windows CI failure is unrelated.

@gottesmm
Copy link
Contributor

gottesmm commented Nov 7, 2020

@shahmishal do you have any concerns about this?

@shahmishal
Copy link
Member

@swift-ci test Windows

Copy link
Member

@shahmishal shahmishal left a comment

Choose a reason for hiding this comment

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

LGTM!

@gottesmm
Copy link
Contributor

gottesmm commented Nov 9, 2020

Ok. This LGTM. Lets merge this!

@gottesmm gottesmm merged commit 8474332 into swiftlang:main Nov 9, 2020
@finagolfin finagolfin deleted the native-flags branch November 10, 2020 01:18
@shahmishal
Copy link
Member

We are seeing toolchain bot failing, it might be related to this PR.

https://ci.swift.org/job/oss-swift-package-osx/5267/

23:50:48 /bin/sh: /Users/buildnode/jenkins/workspace/oss-swift-package-osx/build/buildbot_osx/llvm-macosx-arm64/./bin/clang++: Bad CPU type in executable

@shahmishal
Copy link
Member

@buttaface When you are ready to re-commit this change please run @swift-ci Please build toolchain. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants