Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
Always build (and use) the new SwiftDriver as the default compiler dr…
…iver.

This will make sure that compiler developers are using the new driver when they build the compiler locally and use it.

- Adds a new build-script product category: before_build_script_impl for products we wish to build before the impl products.
- Adds a new EarlySwiftDriver product to that category, which gets built with the host toolchain.
- Adds an escape hatch: --skip-early-swift-driver
- Adjusts the swift CMake configuration with an additional step: swift_create_early_driver_symlinks which (if one was built) creates symlinks in the swift build bin directory to the EarlySwiftDriver swift-driver and swift-help executables.
- Adds a new test subset : only_early_swiftdriver, which will get built into a corresponding CMake test target: check-swift-only_early_swiftdriver-* which runs a small subset of driver-related tests against the Early SwiftDriver.
  - This subset is run always when the compiler itself is tested (--test is specified)
  - With an escape disable-switch: --skip-test-early-swift-driver
  - All tests outside of only_early_swiftdriver are forced to run using the legacy C++ driver (to ensure it gets tested, still).

NOTE: SwiftDriver product (no 'Early') is still the main product used to build the driver for toolchain installation into and for executing the product's own tests. This change does not affect that.
  • Loading branch information
artemcm committed Apr 26, 2021
commit c2dc8e3d0748597e2c964a48aaeb9c20426f618f
29 changes: 29 additions & 0 deletions cmake/modules/SwiftUtils.cmake
Original file line number Diff line number Diff line change
Expand Up @@ -152,6 +152,35 @@ function(swift_create_post_build_symlink target)
COMMENT "${CS_COMMENT}")
endfunction()

# Once swift-frontend is built, if the standalone (early) swift-driver has been built,
# we create a `swift-driver` symlink adjacent to the `swift` and `swiftc` executables
# to ensure that `swiftc` forwards to the standalone driver when invoked.
function(swift_create_early_driver_symlinks target)
# Early swift-driver is built adjacent to the compiler (swift build dir)
set(driver_bin_dir "${CMAKE_BINARY_DIR}/../earlyswiftdriver-${SWIFT_HOST_VARIANT}-${SWIFT_HOST_VARIANT_ARCH}/release/bin")
set(swift_bin_dir "${SWIFT_RUNTIME_OUTPUT_INTDIR}")
# If early swift-driver wasn't built, nothing to do here.
if(NOT EXISTS "${driver_bin_dir}/swift-driver" OR NOT EXISTS "${driver_bin_dir}/swift-help")
message(STATUS "Skipping creating early SwiftDriver symlinks - no early SwiftDriver build found.")
return()
endif()

message(STATUS "Creating early SwiftDriver symlinks.")
message(STATUS "From: ${driver_bin_dir}/swift-driver")
message(STATUS "To: ${swift_bin_dir}/swift-driver")
swift_create_post_build_symlink(swift-frontend
SOURCE "${driver_bin_dir}/swift-driver"
DESTINATION "${swift_bin_dir}/swift-driver"
COMMENT "Creating early SwiftDriver symlinks: swift-driver")

message(STATUS "From: ${driver_bin_dir}/swift-help")
message(STATUS "To: ${swift_bin_dir}/swift-help")
swift_create_post_build_symlink(swift-frontend
SOURCE "${driver_bin_dir}/swift-help"
DESTINATION "${swift_bin_dir}/swift-help"
COMMENT "Creating early SwiftDriver symlinks: swift-help")
endfunction()

function(dump_swift_vars)
set(SWIFT_STDLIB_GLOBAL_CMAKE_CACHE)
get_cmake_property(variableNames VARIABLES)
Expand Down
2 changes: 2 additions & 0 deletions test/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -188,6 +188,7 @@ set(TEST_SUBSETS
only_validation
only_long
only_stress
only_early_swiftdriver
)

if(NOT "${COVERAGE_DB}" STREQUAL "")
Expand Down Expand Up @@ -353,6 +354,7 @@ foreach(SDK ${SWIFT_SDKS})
(test_subset STREQUAL "validation") OR
(test_subset STREQUAL "only_long") OR
(test_subset STREQUAL "only_stress") OR
(test_subset STREQUAL "only_early_swiftdriver") OR
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't you also need to add only_early_swiftdriver to the if statement on 361?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I do not think so. Here, depending on which test subset target is being built, we are adding a list of which test directories need to be passed to lit. For the only_early_swiftdriver subset, we do need the test_bin_dir, because that subset only runs tests inside test/Driver/. We do not need to add the validation_test_bin_dir for this subset, because it does not require any of the tests from that directory.

The same build may also be performing validation testing, but that will be built as a different target, with a different subset, which will include test directories relevant to it.

Copy link
Contributor

Choose a reason for hiding this comment

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

alright. I am fine with this.

(test_subset STREQUAL "all"))
list(APPEND directories "${test_bin_dir}")
endif()
Expand Down
2 changes: 1 addition & 1 deletion test/Unit/lit.cfg
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ elif swift_test_subset == 'only_stress':
# Currently those tests are very fast so it doesn't matter much.
pass
else:
lit_config.fatal("Unknown test mode %r" % swift_test_subset)
lit_config.fatal("Unknown test subset %r" % swift_test_subset)
Copy link
Contributor

Choose a reason for hiding this comment

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

What about if we run long tests... why not run long tests with early swift driver?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The intent is to have a small set of tests that verify the early driver's basic functionality.

When running compiler's test suites (such as the long tests), we run them with the legacy driver, because that too needs validation. But most importantly, when building and testing the actual SwiftDriver build product (not early), it has a setup where it will run driver-relevant lit tests https://github.com/apple/swift-driver/blob/main/Tests/SwiftDriverTests/IntegrationTests.swift#L103. This is where we should actually thoroughly test things, and consider running the long tests.

There are two separate directions for the future:

  • Reduce the lit tests that exercise the legacy driver over time to verify only the most basic functionality, transitioning them to instead run when testing the new driver.
  • Gradually port over driver-specific tests to the driver repository's XCTests, instead of lit.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok. I am fine with this. I was just curious if this was an over sight.


# test_source_root: The root path where tests are located.
# test_exec_root: The root path where tests should be run.
Expand Down
11 changes: 11 additions & 0 deletions test/lit.cfg
Original file line number Diff line number Diff line change
Expand Up @@ -631,6 +631,10 @@ elif swift_test_subset == 'only_stress':
config.available_features.add("stress_test")
config.limit_to_features.add("stress_test")
config.limit_to_features.discard("executable_test")
elif swift_test_subset == 'only_early_swiftdriver':
# Point this subset at a driver-specific set of tests. These are the known reduced subset
# of tests to verify the basic functionality of the standalone (early) swift-driver.
config.test_source_root = os.path.join(config.test_source_root, 'Driver', 'Dependencies')
else:
lit_config.fatal("Unknown test mode %r" % swift_test_subset)

Expand All @@ -640,6 +644,13 @@ if 'swift_evolve' in lit_config.params:
if not 'swift_driver' in lit_config.params:
config.available_features.add("cplusplus_driver")

# Check if we need to run lit tests using the legacy driver or the new driver
# The default for existing test runs is to use the legacy driver.
# The new driver is tested separately.
if swift_test_subset != 'only_early_swiftdriver' and\
os.environ.get('SWIFT_FORCE_TEST_NEW_DRIVER') is None:
config.environment['SWIFT_USE_OLD_DRIVER'] = '1'

# Enable benchmark testing when the binary is found (has fully qualified path).
if config.benchmark_o != 'Benchmark_O':
config.available_features.add('benchmark')
Expand Down
4 changes: 4 additions & 0 deletions tools/driver/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,10 @@ target_link_libraries(swift-frontend
swiftSymbolGraphGen
LLVMBitstreamReader)

# Create a `swift-driver` symlinks adjacent to the `swift-frontend` executable
# to ensure that `swiftc` forwards to the standalone driver when invoked.
swift_create_early_driver_symlinks(swift-frontend)

swift_create_post_build_symlink(swift-frontend
SOURCE "swift-frontend${CMAKE_EXECUTABLE_SUFFIX}"
DESTINATION "swift${CMAKE_EXECUTABLE_SUFFIX}"
Expand Down
102 changes: 66 additions & 36 deletions utils/build-script
Original file line number Diff line number Diff line change
Expand Up @@ -848,14 +848,24 @@ class BuildScriptInvocation(object):
return options

def compute_product_classes(self):
"""compute_product_classes() -> (list, list)

Compute the list first of all build-script-impl products and then all
non-build-script-impl products. It is assumed that concatenating the two
lists together will result in a valid dependency graph for the
compilation.
"""compute_product_classes() -> (list, list, list)

Compute the list first of all pre-build-script-impl products, then all
build-script-impl products and then all non-build-script-impl products.
It is assumed that concatenating the three lists together will result in a
valid dependency graph for the compilation.
"""
before_impl_product_classes = []
# If --skip-early-swift-driver is passed in, swift will be built
# as usual, but relying on its own C++-based (Legacy) driver.
# Otherwise, we build an "early" swift-driver using the host
# toolchain, which the later-built compiler will forward
# `swiftc` invocations to. That is, if we find a Swift compiler
# in the host toolchain. If the host toolchain is not equpipped with
# a Swift compiler, a warning is emitted. In the future, it may become
# mandatory that the host toolchain come with its own `swiftc`.
if self.args.build_early_swift_driver:
before_impl_product_classes.append(products.EarlySwiftDriver)

# FIXME: This is a weird division (returning a list of class objects),
# but it matches the existing structure of the `build-script-impl`.
Expand Down Expand Up @@ -940,6 +950,7 @@ class BuildScriptInvocation(object):
# non-build-script-impl products. Otherwise, it would be unsafe to
# re-order build-script-impl products in front of non
# build-script-impl products.
before_impl_product_classes = []
impl_product_classes = []
product_classes = []
is_darwin = platform.system() == 'Darwin'
Expand All @@ -950,15 +961,19 @@ class BuildScriptInvocation(object):

if p.is_build_script_impl_product():
impl_product_classes.append(p)
elif p.is_before_build_script_impl_product():
before_impl_product_classes.append(p)
else:
product_classes.append(p)
if self.args.verbose_build:
print("Final Build Order:")
for p in before_impl_product_classes:
print(" {}".format(p.product_name()))
for p in impl_product_classes:
print(" {}".format(p.product_name()))
for p in product_classes:
print(" {}".format(p.product_name()))
return (impl_product_classes, product_classes)
return (before_impl_product_classes, impl_product_classes, product_classes)

def execute(self):
"""Execute the invocation with the configured arguments."""
Expand All @@ -975,7 +990,6 @@ class BuildScriptInvocation(object):
return

# Otherwise, we compute and execute the individual actions ourselves.

# Compute the list of hosts to operate on.
all_host_names = [
self.args.host_target] + self.args.cross_compile_hosts
Expand All @@ -986,10 +1000,22 @@ class BuildScriptInvocation(object):
#
# FIXME: This should really be per-host, but the current structure
# matches that of `build-script-impl`.
(impl_product_classes, product_classes) = self.compute_product_classes()
(before_impl_product_classes, impl_product_classes, product_classes) =\
self.compute_product_classes()

# Execute each "pass".

# Pre-build-script-impl products...
# Note: currently only supports building for the host.
for host_target in [self.args.host_target]:
for product_class in before_impl_product_classes:
if product_class.is_build_script_impl_product():
continue
if not product_class.is_before_build_script_impl_product():
continue
# Execute clean, build, test, install
self.execute_product_build_steps(product_class, host_target)

# Build...
for host_target in all_hosts:
# FIXME: We should only compute these once.
Expand Down Expand Up @@ -1029,33 +1055,8 @@ class BuildScriptInvocation(object):
for product_class in product_classes:
if product_class.is_build_script_impl_product():
continue
product_source = product_class.product_source_name()
product_name = product_class.product_name()
if product_class.is_swiftpm_unified_build_product():
build_dir = self.workspace.swiftpm_unified_build_dir(
host_target)
else:
build_dir = self.workspace.build_dir(
host_target, product_name)
product = product_class(
args=self.args,
toolchain=self.toolchain,
source_dir=self.workspace.source_dir(product_source),
build_dir=build_dir)
if product.should_clean(host_target):
print("--- Cleaning %s ---" % product_name)
product.clean(host_target)
if product.should_build(host_target):
print("--- Building %s ---" % product_name)
product.build(host_target)
if product.should_test(host_target):
print("--- Running tests for %s ---" % product_name)
product.test(host_target)
print("--- Finished tests for %s ---" % product_name)
if product.should_install(host_target) or \
(self.install_all and product.should_build(host_target)):
print("--- Installing %s ---" % product_name)
product.install(host_target)
# Execute clean, build, test, install
self.execute_product_build_steps(product_class, host_target)

# Extract symbols...
for host_target in all_hosts:
Expand Down Expand Up @@ -1103,6 +1104,35 @@ class BuildScriptInvocation(object):
["--only-execute", action_name],
env=self.impl_env, echo=self.args.verbose_build)

def execute_product_build_steps(self, product_class, host_target):
product_source = product_class.product_source_name()
product_name = product_class.product_name()
if product_class.is_swiftpm_unified_build_product():
build_dir = self.workspace.swiftpm_unified_build_dir(
host_target)
else:
build_dir = self.workspace.build_dir(
host_target, product_name)
product = product_class(
args=self.args,
toolchain=self.toolchain,
source_dir=self.workspace.source_dir(product_source),
build_dir=build_dir)
if product.should_clean(host_target):
print("--- Cleaning %s ---" % product_name)
product.clean(host_target)
if product.should_build(host_target):
print("--- Building %s ---" % product_name)
product.build(host_target)
if product.should_test(host_target):
print("--- Running tests for %s ---" % product_name)
product.test(host_target)
print("--- Finished tests for %s ---" % product_name)
if product.should_install(host_target) or \
(self.install_all and product.should_build(host_target)):
print("--- Installing %s ---" % product_name)
product.install(host_target)


# -----------------------------------------------------------------------------
# Main (preset)
Expand Down
15 changes: 14 additions & 1 deletion utils/build_swift/build_swift/driver_arguments.py
Original file line number Diff line number Diff line change
Expand Up @@ -193,6 +193,12 @@ def _apply_default_arguments(args):
args.test_swiftevolve = False
args.test_toolchainbenchmarks = False

# --test implies --test-early-swift-driver
# (unless explicitly skipped with `--skip-test-early-swift-driver`)
if args.test and (args.build_early_swift_driver and
args.test_early_swift_driver is None):
args.test_early_swift_driver = True

# --skip-test-ios is merely a shorthand for host and simulator tests.
if not args.test_ios:
args.test_ios_host = False
Expand Down Expand Up @@ -610,6 +616,9 @@ def create_argument_parser():
option(['--swift-driver'], toggle_true('build_swift_driver'),
help='build swift-driver')

option(['--skip-early-swift-driver'], toggle_false('build_early_swift_driver'),
help='skip building the early swift-driver')

option(['--indexstore-db'], toggle_true('build_indexstoredb'),
help='build IndexStoreDB')
option('--test-indexstore-db-sanitize-all',
Expand Down Expand Up @@ -1036,9 +1045,13 @@ def create_argument_parser():
toggle_false('test_android_host'),
help='skip testing Android device targets on the host machine (the '
'phone itself)')

option('--skip-clean-llbuild', toggle_false('clean_llbuild'),
help='skip cleaning up llbuild')
option('--clean-early-swift-driver', toggle_true('clean_early_swift_driver'),
help='Clean up the early SwiftDriver')
option('--skip-test-early-swift-driver',
store('test_early_swift_driver', const=False),
help='Test the early SwiftDriver against the host toolchain')
option('--skip-clean-swiftpm', toggle_false('clean_swiftpm'),
help='skip cleaning up swiftpm')
option('--skip-clean-swift-driver', toggle_false('clean_swift_driver'),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -586,6 +586,14 @@ def test_implied_defaults_test_optimize_for_size(self):
namespace = self.parse_default_args(['--test-optimize-for-size'])
self.assertTrue(namespace.test)

def test_implied_defaults_test_early_swift_driver(self):
namespace = self.parse_default_args(['--test'])
self.assertTrue(namespace.test_early_swift_driver)

def test_implied_defaults_test_no_early_swift_driver(self):
namespace = self.parse_default_args(['--test --skip-early-swift-driver'])
self.assertTrue(namespace.test_early_swift_driver is None)

def test_implied_defaults_test_optimize_none_with_implicit_dynamic(self):
namespace = self.parse_default_args(
['--test-optimize-none-with-implicit-dynamic'])
Expand Down
9 changes: 8 additions & 1 deletion utils/build_swift/tests/expected_options.py
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,7 @@
'build_swift_stdlib_unittest_extra': False,
'build_swiftpm': False,
'build_swift_driver': False,
'build_early_swift_driver': True,
'build_swiftsyntax': False,
'build_libparser_only': False,
'build_skstresstester': False,
Expand All @@ -99,8 +100,8 @@
'test_sourcekitlsp_sanitize_all': False,
'build_sourcekitlsp': False,
'install_swiftpm': False,
'install_swift_driver': False,
'install_swiftsyntax': False,
'install_swift_driver': False,
'swiftsyntax_verify_generated_files': False,
'install_playgroundsupport': False,
'install_sourcekitlsp': False,
Expand Down Expand Up @@ -215,7 +216,9 @@
'clean_llbuild': True,
'clean_swiftpm': True,
'clean_swift_driver': True,
'clean_early_swift_driver': False,
'test': None,
'test_early_swift_driver': None,
'test_android': False,
'test_android_host': False,
'test_cygwin': False,
Expand Down Expand Up @@ -461,6 +464,8 @@ class BuildScriptImplOption(_BaseOption):
SetOption('--skip-ios', dest='ios', value=False),
SetOption('--skip-tvos', dest='tvos', value=False),
SetOption('--skip-watchos', dest='watchos', value=False),
SetOption('--skip-test-early-swift-driver',
dest='test_early_swift_driver', value=False),

SetTrueOption('--benchmark'),
SetTrueOption('--clean'),
Expand Down Expand Up @@ -555,6 +560,7 @@ class BuildScriptImplOption(_BaseOption):
EnableOption('--watchos'),
EnableOption('--xctest', dest='build_xctest'),
EnableOption('--swift-disable-dead-stripping'),
EnableOption('--clean-early-swift-driver', dest='clean_early_swift_driver'),

DisableOption('--skip-build-cmark', dest='build_cmark'),
DisableOption('--skip-build-llvm', dest='build_llvm'),
Expand All @@ -580,6 +586,7 @@ class BuildScriptImplOption(_BaseOption):
DisableOption('--skip-build-watchos-simulator',
dest='build_watchos_simulator'),
DisableOption('--skip-clean-llbuild', dest='clean_llbuild'),
DisableOption('--skip-early-swift-driver', dest='build_early_swift_driver'),
DisableOption('--skip-clean-swiftpm', dest='clean_swiftpm'),
DisableOption('--skip-clean-swift-driver', dest='clean_swift_driver'),
DisableOption('--skip-test-android', dest='test_android'),
Expand Down
1 change: 1 addition & 0 deletions utils/run-test
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ TEST_SUBSETS = [
'only_validation',
'only_long',
'only_stress',
'only_early_swiftdriver'
]

SWIFT_SOURCE_DIR = os.path.join(SWIFT_SOURCE_ROOT, 'swift')
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -167,6 +167,14 @@ def __init__(self, host_target, args):
else:
subset_suffix = ""

# If the compiler is being tested after being built to use the
# standalone swift-driver, we build a test-target to
# run a reduced set of lit-tests that verify the early swift-driver.
if getattr(args, 'test_early_swift_driver', False) and\
Copy link
Contributor

Choose a reason for hiding this comment

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

I am pretty sure that the getattr here is not necessary. But I am ok with it.

not test_host_only:
self.swift_test_run_targets.append(
"check-swift-only_early_swiftdriver-{}".format(name))

# Support for running the macCatalyst tests with
# the iOS-like target triple.
macosx_platform_match = re.search("macosx-(.*)", name)
Expand All @@ -180,6 +188,7 @@ def __init__(self, host_target, args):
(self.swift_test_run_targets
.append("check-swift{}{}-{}".format(
subset_suffix, suffix, name)))

if args.test_optimized and not test_host_only:
self.swift_test_run_targets.append(
"check-swift{}-optimize-{}".format(
Expand Down
Loading