[build-script] Turn on --no-legacy-impl by default #21772

benlangmuir · 2019-01-10T21:21:49Z

Turns on the --no-legacy-impl option to build-script by default; the
old behaviour is temporarily still available as --legacy-impl.

This causes build-script to invoke build-script-impl for every
individual build/test/install/etc. action rather than a single global
invocation. For example, a single invocation might be for
macosx-swift-install. This will enable the python code in build-script
to drive the overall process and add additional steps in between actions
without the involvement of build-script-impl. It also provides a path to
refactoring the existing actions out of build-script-impl individually.

Discussed as part of https://forums.swift.org/t/rfc-building-swift-packages-in-build-script/18920

The --no-legacy-impl flag was originally disabled by default because of
concerns about the performance of null builds due to the increased
number of script invocations. There is a small optimization in this
commit to use tr when processing command-line options instead of
bash's builtin substitution, which eliminates most of the overhead.
After this change, a null build of llvm+swift changes from 1.6 s to
2.1 s on Linux, and from 5 s to 6 s on macOS. Non-null builds and
builds that involve more build products than just llvm+swift (e.g.
corelibs) are basically unaffected since they are not correctly
incremental to begin with.

The changes to build-script-impl in this commit are to fix the behaviour
of --no-legacy-impl, which had bitrotted since it was introduced. These
changes are to make various parts of the script not rely on variables
defined in "earlier" parts of the script, which is good hygiene in
general.

benlangmuir · 2019-01-10T21:25:02Z

cc @compnerd @Rostepher @gottesmm

This is a preliminary for https://forums.swift.org/t/rfc-building-swift-packages-in-build-script/18920

compnerd · 2019-01-10T22:33:29Z

utils/build-script

-            impl_args += [
-                "--%s-cmake-options=%s" % (product_name, ' '.join(cmake_opts))
-            ]
+            if len(cmake_opts) > 0:


The more idiomatic way to write this would be:

if cmake_opts:

compnerd · 2019-01-10T22:36:59Z

utils/build-script-impl

+                        -DSWIFT_${SWIFT_HOST_VARIANT_SDK}_${SWIFT_HOST_VARIANT_ARCH}_ICU_I18N_INCLUDE:STRING="${ICU_TMPINSTALL}/include"
+                        -DSWIFT_${SWIFT_HOST_VARIANT_SDK}_${SWIFT_HOST_VARIANT_ARCH}_ICU_STATICLIB:BOOL=TRUE
+                        -DICU_UC_LIBDIR:PATH="${LIBICU_BUILD_DIR}/lib"
+                        -DICU_I18N_LIBDIR:PATH="${LIBICU_BUILD_DIR}/lib"


Hmm, does this work? I thought that it requires that the path to the library is given instead. I think it would be nicer if we just used -DSWIFT_${SWIFT_HOST_VARIANT_SDK}_${SWIFT_HOST_VARIANT_ARCH}_ICU_{UC,I18N}=

I just kept these the same as they were before. Maybe they were always dead?

compnerd · 2019-01-10T22:38:09Z

utils/build-script-impl

@@ -2623,12 +2637,14 @@ for host in "${ALL_HOSTS[@]}"; do
                  echo "Cleaning the XCTest build directory"
                  call rm -rf "${XCTEST_BUILD_DIR}"

+                  LLVM_BIN="$(build_directory_bin ${LOCAL_HOST} llvm)"


Where is this new variable used?

In the options immediately below, e.g. Line 2645. Previously it depended on this being set somewhere else

compnerd · 2019-01-10T22:40:06Z

utils/build-script-impl

+                # Set the PKG_CONFIG_PATH so that core-foundation can find the libraries and
+                # header files
+                LIBICU_BUILD_DIR="$(build_directory ${host} libicu)"
+                export PKG_CONFIG_PATH="${LIBICU_BUILD_DIR}/config:${PKG_CONFIG_PATH}"


Why were the CMake options removed?

Which ones? Maybe this comment was on the wrong line? There was a set of options being appended to swift_cmake_options during the setup of ICU that I moved directly into the configuration for swift.

benlangmuir · 2019-01-11T22:44:11Z

@swift-ci please test

benlangmuir · 2019-01-11T22:44:18Z

@swift-ci please build toolchain

swift-ci · 2019-01-11T22:46:08Z

Build failed
Swift Test OS X Platform
Git Sha - ce4c85a

swift-ci · 2019-01-11T22:46:15Z

Build failed
Swift Test Linux Platform
Git Sha - ce4c85a

benlangmuir · 2019-01-14T19:56:41Z

Any additional feedback, @compnerd?

Rostepher · 2019-01-14T21:55:58Z

utils/build-script

-            ]
+            if cmake_opts:
+                impl_args += [
+                    "--%s-cmake-options=%s" %


(Nit-pick) We should be using the more modern Python string formatting via the str.format method. This could be re-written as: '--{}-cmake-options={}'.format(product_name, ' '.join(cmake_opts)).

benlangmuir · 2019-01-14T22:39:53Z

@swift-ci please smoke test

benlangmuir · 2019-01-15T18:48:41Z

@swift-ci please smoke test

benlangmuir · 2019-01-17T19:19:09Z

@compnerd or @Rostepher any additional feedback, or is this good to go?

Rostepher · 2019-01-17T22:04:24Z

I'm fine with merging this.

Turns on the `--no-legacy-impl` option to build-script by default; the old behaviour is temporarily still available as `--legacy-impl`. This causes build-script to invoke build-script-impl for every individual build/test/install/etc. action rather than a single global invocation. For example, a single invocation might be for `macosx-swift-install`. This will enable the python code in build-script to drive the overall process and add additional steps in between actions without the involvement of build-script-impl. It also provides a path to refactoring the existing actions out of build-script-impl individually. Discussed as part of https://forums.swift.org/t/rfc-building-swift-packages-in-build-script/18920 The --no-legacy-impl flag was originally disabled by default because of concerns about the performance of null builds due to the increased number of script invocations. There is a small optimization in this commit to use `tr` when processing command-line options instead of bash's builtin substitution, which eliminates most of the overhead. After this change, a null build of llvm+swift changes from 1.6 s to 2.1 s on Linux, and from 5 s to 6 s on macOS. Non-null builds and builds that involve more build products than just llvm+swift (e.g. corelibs) are basically unaffected since they are not correctly incremental to begin with. The changes to build-script-impl in this commit are to fix the behaviour of --no-legacy-impl, which had bitrotted since it was introduced. These changes are to make various parts of the script not rely on variables defined in "earlier" parts of the script, which is good hygiene in general.

benlangmuir · 2019-01-23T00:18:21Z

@swift-ci please smoke test

benlangmuir · 2019-01-23T17:49:46Z

@swift-ci please smoke test macOS

benlangmuir · 2019-01-23T17:57:06Z

@compnerd is this ready to go, or did you have more feedback?

compnerd

If the builds are happy, I'm happy.

benlangmuir · 2019-01-24T15:20:40Z

@swift-ci please build toolchain

benlangmuir mentioned this pull request Jan 10, 2019

DO NOT MERGE [build-script] Flip the default value of legacy_impl #21020

Closed

benlangmuir requested review from Rostepher and compnerd January 10, 2019 21:23

compnerd reviewed Jan 10, 2019

View reviewed changes

Rostepher reviewed Jan 14, 2019

View reviewed changes

benlangmuir force-pushed the no-legacy-rebase branch from fc6a94d to 1f5f24f Compare January 15, 2019 18:43

benlangmuir added 3 commits January 22, 2019 15:37

Update for feedback

5dbe28b

Update --*-cmake-options to use format()

08eabb4

benlangmuir force-pushed the no-legacy-rebase branch from 1f5f24f to 08eabb4 Compare January 22, 2019 23:59

compnerd approved these changes Jan 24, 2019

View reviewed changes

benlangmuir merged commit dc2ea3e into swiftlang:master Jan 24, 2019

benlangmuir deleted the no-legacy-rebase branch January 24, 2019 17:14

[build-script] Turn on --no-legacy-impl by default #21772

[build-script] Turn on --no-legacy-impl by default #21772

Uh oh!

Conversation

benlangmuir commented Jan 10, 2019

Uh oh!

benlangmuir commented Jan 10, 2019

Uh oh!

compnerd Jan 10, 2019

Choose a reason for hiding this comment

Uh oh!

compnerd Jan 10, 2019

Choose a reason for hiding this comment

Uh oh!

benlangmuir Jan 10, 2019

Choose a reason for hiding this comment

Uh oh!

compnerd Jan 10, 2019

Choose a reason for hiding this comment

Uh oh!

benlangmuir Jan 10, 2019

Choose a reason for hiding this comment

Uh oh!

compnerd Jan 10, 2019

Choose a reason for hiding this comment

Uh oh!

benlangmuir Jan 10, 2019

Choose a reason for hiding this comment

Uh oh!

benlangmuir commented Jan 11, 2019

Uh oh!

benlangmuir commented Jan 11, 2019

Uh oh!

swift-ci commented Jan 11, 2019

Uh oh!

swift-ci commented Jan 11, 2019

Uh oh!

benlangmuir commented Jan 14, 2019

Uh oh!

Rostepher Jan 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

benlangmuir commented Jan 14, 2019

Uh oh!

benlangmuir commented Jan 15, 2019

Uh oh!

benlangmuir commented Jan 17, 2019

Uh oh!

Rostepher commented Jan 17, 2019

Uh oh!

benlangmuir commented Jan 23, 2019

Uh oh!

benlangmuir commented Jan 23, 2019

Uh oh!

benlangmuir commented Jan 23, 2019

Uh oh!

compnerd left a comment

Choose a reason for hiding this comment

Uh oh!

benlangmuir commented Jan 24, 2019

Uh oh!

Uh oh!

Rostepher Jan 14, 2019 •

edited

Loading