-
Notifications
You must be signed in to change notification settings - Fork 10.5k
[build-script] Add support for using isolated -impl actions. #2988
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -50,7 +50,11 @@ from swift_build_support.cmake import CMake # noqa (E402) | |
import swift_build_support.workspace # noqa (E402) | ||
|
||
|
||
def call_without_sleeping(command, env=None, dry_run=False): | ||
build_script_impl = os.path.join( | ||
SWIFT_SOURCE_ROOT, "swift", "utils", "build-script-impl") | ||
|
||
|
||
def call_without_sleeping(command, env=None, dry_run=False, echo=False): | ||
""" | ||
Execute a command during which system sleep is disabled. | ||
|
||
|
@@ -62,7 +66,7 @@ def call_without_sleeping(command, env=None, dry_run=False): | |
# Don't mutate the caller's copy of the arguments. | ||
command = ["caffeinate"] + list(command) | ||
|
||
shell.call(command, env=env, dry_run=dry_run) | ||
shell.call(command, env=env, dry_run=dry_run, echo=echo) | ||
|
||
|
||
class HostSpecificConfiguration(object): | ||
|
@@ -774,6 +778,115 @@ class BuildScriptInvocation(object): | |
|
||
return options | ||
|
||
def compute_product_classes(self): | ||
"""compute_product_classes() -> list | ||
|
||
Compute the list of all Product classes used in this build. This list | ||
is constructed in dependency order. | ||
""" | ||
|
||
# FIXME: This is a weird division (returning a list of class objects), | ||
# but it matches the existing structure of the `build-script-impl`. | ||
|
||
product_classes = [] | ||
product_classes.append(products.CMark) | ||
product_classes.append(products.LLVM) | ||
product_classes.append(products.Swift) | ||
if self.args.build_lldb: | ||
product_classes.append(products.LLDB) | ||
if self.args.build_llbuild: | ||
product_classes.append(products.LLBuild) | ||
if self.args.build_libdispatch: | ||
product_classes.append(products.LibDispatch) | ||
if self.args.build_foundation: | ||
product_classes.append(products.Foundation) | ||
if self.args.build_xctest: | ||
product_classes.append(products.XCTest) | ||
if self.args.build_swiftpm: | ||
product_classes.append(products.SwiftPM) | ||
return product_classes | ||
|
||
def execute(self): | ||
"""Execute the invocation with the configured arguments.""" | ||
|
||
# Convert to a build-script-impl invocation. | ||
(impl_env, impl_args) = self.convert_to_impl_arguments() | ||
|
||
# If using the legacy implementation, delegate all behavior to | ||
# `build-script-impl`. | ||
if self.args.legacy_impl: | ||
# Execute the underlying build script implementation. | ||
call_without_sleeping([build_script_impl] + impl_args, | ||
env=impl_env, echo=True) | ||
return | ||
|
||
# Otherwise, we compute and execute the individual actions ourselves. | ||
|
||
def execute_one_impl_action(host=None, product_class=None, name=None): | ||
if host is None: | ||
assert (product_class is None and | ||
name == "merged-hosts-lipo"), "invalid action" | ||
action_name = name | ||
elif product_class is None: | ||
assert name == "package", "invalid action" | ||
action_name = "{}-{}".format(host.name, name) | ||
else: | ||
assert name is not None, "invalid action" | ||
action_name = "{}-{}-{}".format( | ||
host.name, product_class.product_name(), name) | ||
call_without_sleeping( | ||
[build_script_impl] + impl_args + [ | ||
"--only-execute", action_name], | ||
env=impl_env, echo=self.args.verbose_build) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Note that this mode drops echoing subcommands by default, it is too verbose until we finish killing the |
||
|
||
# Compute the list of hosts to operate on. | ||
all_host_names = [ | ||
self.args.host_target] + self.args.cross_compile_hosts | ||
all_hosts = [StdlibDeploymentTarget.get_target_for_name(name) | ||
for name in all_host_names] | ||
|
||
# Compute the list of product classes to operate on. | ||
# | ||
# FIXME: This should really be per-host, but the current structure | ||
# matches that of `build-script-impl`. | ||
product_classes = self.compute_product_classes() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The reason it's not per-host is (as usual) because we don't have the build-script argument structure necessary to define a list of products per host; they are defined with flags such as "--foundation". |
||
|
||
# Execute each "pass". | ||
|
||
# Build... | ||
for host_target in all_hosts: | ||
# FIXME: We should only compute these once. | ||
config = HostSpecificConfiguration(host_target.name, self) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, BuildScriptInvocation is not very well structured here. It should explicitly calculate the configurations in its initialiser, instead of in Incidentally, HostSpecificConfiguration doesn't store the symbolicated versions of the targets (or host_target), only the strings. That's one of the incidental things I changed in #2835. Moving that in to the argument parsing logic is another possibility. Actually, looking at the entire build-script after this change, it's looking pretty... messy. Lots has changed very quickly, porting build-script-impl verbatim, and we've got lots of duplication and stuff that just plain doesn't make sense. Rather than pushing full-steam-ahead with the python conversion, I think we should use this opportunity to clean things up a little. Specifically:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I basically agree with all your points, but the problem is that it is very hard to enact cleanups when the implementation is split between the Python code and the bash code. I really think we are better off getting the migration done and then continuing to clean up the code. More specific comments:
|
||
print("Building the standard library for: {}".format( | ||
" ".join(config.swift_stdlib_build_targets))) | ||
if config.swift_test_run_targets and ( | ||
self.args.test or self.args.long_test): | ||
print("Running Swift tests for: {}".format( | ||
" ".join(config.swift_test_run_targets))) | ||
if config.swift_benchmark_run_targets and self.args.benchmark: | ||
print("Running Swift benchmarks for: {}".format( | ||
" ".join(config.swift_benchmark_run_targets))) | ||
|
||
for product_class in product_classes: | ||
execute_one_impl_action(host_target, product_class, "build") | ||
|
||
# Test... | ||
for host_target in all_hosts: | ||
for product_class in product_classes: | ||
execute_one_impl_action(host_target, product_class, "test") | ||
|
||
# Install... | ||
for host_target in all_hosts: | ||
for product_class in product_classes: | ||
execute_one_impl_action(host_target, product_class, "install") | ||
|
||
# Package... | ||
for host_target in all_hosts: | ||
execute_one_impl_action(host_target, name="package") | ||
|
||
# Lipo... | ||
execute_one_impl_action(name="merged-hosts-lipo") | ||
|
||
|
||
# Main entry point for the preset mode. | ||
def main_preset(): | ||
|
@@ -1042,6 +1155,11 @@ details of the setups of other systems or automated environments.""") | |
"them", | ||
action="store_true", | ||
default=False) | ||
parser.add_argument( | ||
"--no-legacy-impl", dest="legacy_impl", | ||
help="avoid legacy implementation", | ||
action="store_false", | ||
default=True) | ||
|
||
targets_group = parser.add_argument_group( | ||
title="Host and cross-compilation targets") | ||
|
@@ -1739,9 +1857,6 @@ details of the setups of other systems or automated environments.""") | |
|
||
args = migration.parse_args(parser, sys.argv[1:]) | ||
|
||
build_script_impl = os.path.join( | ||
SWIFT_SOURCE_ROOT, "swift", "utils", "build-script-impl") | ||
|
||
if args.build_script_impl_args: | ||
# If we received any impl args, check if `build-script-impl` would | ||
# accept them or not before any further processing. | ||
|
@@ -1795,13 +1910,8 @@ details of the setups of other systems or automated environments.""") | |
if args.build_ninja: | ||
invocation.build_ninja() | ||
|
||
# Convert to a build-script-impl invocation. | ||
(build_script_impl_env, build_script_impl_args) = \ | ||
invocation.convert_to_impl_arguments() | ||
|
||
# Execute the underlying build script implementation. | ||
call_without_sleeping([build_script_impl] + build_script_impl_args, | ||
env=build_script_impl_env) | ||
invocation.execute() | ||
|
||
if args.symbols_package: | ||
print('--- Creating symbols package ---') | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,17 @@ | ||
# swift_build_support/products/cmark.py -------------------------*- python -*- | ||
# | ||
# This source file is part of the Swift.org open source project | ||
# | ||
# Copyright (c) 2014 - 2016 Apple Inc. and the Swift project authors | ||
# Licensed under Apache License v2.0 with Runtime Library Exception | ||
# | ||
# See http://swift.org/LICENSE.txt for license information | ||
# See http://swift.org/CONTRIBUTORS.txt for the list of Swift project authors | ||
# | ||
# ---------------------------------------------------------------------------- | ||
|
||
from . import product | ||
|
||
|
||
class CMark(product.Product): | ||
pass |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,17 @@ | ||
# swift_build_support/products/foundation.py ---------------------*- python -*- | ||
# | ||
# This source file is part of the Swift.org open source project | ||
# | ||
# Copyright (c) 2014 - 2016 Apple Inc. and the Swift project authors | ||
# Licensed under Apache License v2.0 with Runtime Library Exception | ||
# | ||
# See http://swift.org/LICENSE.txt for license information | ||
# See http://swift.org/CONTRIBUTORS.txt for the list of Swift project authors | ||
# | ||
# ---------------------------------------------------------------------------- | ||
|
||
from . import product | ||
|
||
|
||
class Foundation(product.Product): | ||
pass |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,17 @@ | ||
# swift_build_support/products/libdispatch.py -------------------*- python -*- | ||
# | ||
# This source file is part of the Swift.org open source project | ||
# | ||
# Copyright (c) 2014 - 2016 Apple Inc. and the Swift project authors | ||
# Licensed under Apache License v2.0 with Runtime Library Exception | ||
# | ||
# See http://swift.org/LICENSE.txt for license information | ||
# See http://swift.org/CONTRIBUTORS.txt for the list of Swift project authors | ||
# | ||
# ---------------------------------------------------------------------------- | ||
|
||
from . import product | ||
|
||
|
||
class LibDispatch(product.Product): | ||
pass |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,17 @@ | ||
# swift_build_support/products/clang.py -------------------------*- python -*- | ||
# | ||
# This source file is part of the Swift.org open source project | ||
# | ||
# Copyright (c) 2014 - 2016 Apple Inc. and the Swift project authors | ||
# Licensed under Apache License v2.0 with Runtime Library Exception | ||
# | ||
# See http://swift.org/LICENSE.txt for license information | ||
# See http://swift.org/CONTRIBUTORS.txt for the list of Swift project authors | ||
# | ||
# ---------------------------------------------------------------------------- | ||
|
||
from . import product | ||
|
||
|
||
class LLBuild(product.Product): | ||
pass |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,17 @@ | ||
# swift_build_support/products/lldb.py --------------------------*- python -*- | ||
# | ||
# This source file is part of the Swift.org open source project | ||
# | ||
# Copyright (c) 2014 - 2016 Apple Inc. and the Swift project authors | ||
# Licensed under Apache License v2.0 with Runtime Library Exception | ||
# | ||
# See http://swift.org/LICENSE.txt for license information | ||
# See http://swift.org/CONTRIBUTORS.txt for the list of Swift project authors | ||
# | ||
# ---------------------------------------------------------------------------- | ||
|
||
from . import product | ||
|
||
|
||
class LLDB(product.Product): | ||
pass |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,17 @@ | ||
# swift_build_support/products/llvm.py --------------------------*- python -*- | ||
# | ||
# This source file is part of the Swift.org open source project | ||
# | ||
# Copyright (c) 2014 - 2016 Apple Inc. and the Swift project authors | ||
# Licensed under Apache License v2.0 with Runtime Library Exception | ||
# | ||
# See http://swift.org/LICENSE.txt for license information | ||
# See http://swift.org/CONTRIBUTORS.txt for the list of Swift project authors | ||
# | ||
# ---------------------------------------------------------------------------- | ||
|
||
from . import product | ||
|
||
|
||
class LLVM(product.Product): | ||
pass |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,32 @@ | ||
# swift_build_support/products/product.py -----------------------*- python -*- | ||
# | ||
# This source file is part of the Swift.org open source project | ||
# | ||
# Copyright (c) 2014 - 2016 Apple Inc. and the Swift project authors | ||
# Licensed under Apache License v2.0 with Runtime Library Exception | ||
# | ||
# See http://swift.org/LICENSE.txt for license information | ||
# See http://swift.org/CONTRIBUTORS.txt for the list of Swift project authors | ||
# | ||
# ---------------------------------------------------------------------------- | ||
|
||
|
||
class Product(object): | ||
@classmethod | ||
def product_name(cls): | ||
"""product_name() -> str | ||
|
||
The identifier-style name to use for this product. | ||
""" | ||
return cls.__name__.lower() | ||
|
||
@classmethod | ||
def get_build_directory_name(cls, host_target): | ||
return "{}-{}".format(cls.product_name(), | ||
host_target.name) | ||
|
||
def __init__(self, args, toolchain, source_dir, build_dir): | ||
self.args = args | ||
self.toolchain = toolchain | ||
self.source_dir = source_dir | ||
self.build_dir = build_dir |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,17 @@ | ||
# swift_build_support/products/swift.py -------------------------*- python -*- | ||
# | ||
# This source file is part of the Swift.org open source project | ||
# | ||
# Copyright (c) 2014 - 2016 Apple Inc. and the Swift project authors | ||
# Licensed under Apache License v2.0 with Runtime Library Exception | ||
# | ||
# See http://swift.org/LICENSE.txt for license information | ||
# See http://swift.org/CONTRIBUTORS.txt for the list of Swift project authors | ||
# | ||
# ---------------------------------------------------------------------------- | ||
|
||
from . import product | ||
|
||
|
||
class Swift(product.Product): | ||
pass |
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.
Do we really need to introduce a new concept called product classes. It seems like what you are really talking about are projects, no? Why not just make this a property called: "projects".
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.
After reading the rest of the commit, I see what you are doing in terms of product_classes. But still, why not just make it a property called product_classes and then cache the result?
Uh oh!
There was an error while loading. Please reload this page.
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 will eventually need this to be host-specific, so it doesn't make sense to be a property. (I also agree about preferring projects to products as a name, but that is an existing
build-script-impl
convention this patch isn't changing)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.
As I mention in a longer comment below, I think it should be a property on HostSpecificConfiguration. For now we'd set every instance to the same parsed set of products.