Skip to content

[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

Merged
merged 1 commit into from
Jun 23, 2016
Merged
Show file tree
Hide file tree
Changes from all commits
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
132 changes: 121 additions & 11 deletions utils/build-script
Original file line number Diff line number Diff line change
Expand Up @@ -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.

Expand All @@ -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):
Expand Down Expand Up @@ -774,6 +778,115 @@ class BuildScriptInvocation(object):

return options

def compute_product_classes(self):
Copy link
Contributor

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".

Copy link
Contributor

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?

Copy link
Contributor Author

@ddunbar ddunbar Jun 13, 2016

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)

Copy link
Contributor

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.

"""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)
Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 -impl script.


# 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()
Copy link
Contributor

Choose a reason for hiding this comment

The 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)
Copy link
Contributor

@karwa karwa Jun 11, 2016

Choose a reason for hiding this comment

The 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 convert_to_impl_args, and store them as an instance variable. Then iterating over all_hosts here just comes an iteration over the HostSpecificConfigurations (Host available via config.host_target, Targets available via config.stdlib_targets_to_configure/build).

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:

  • I think HostSpecificConfiguration should have its own file
  • As in [CMake] Replace SWIFT_SDKS with SWIFT_CROSS_COMPILE_DEPLOYMENT_TARGETS #2835, it should store its host_target and stdlib_targets in symbolicated form as instance variables
  • It should also know which products to build. When we compute_product_classes here, we should be modifying a list of products on the host config. This should happen during argument parsing, not during execute()
  • I understand that parts of this may be in flux, but we should have some tests for things we are reasonably sure won't go away. For example, calculating the CMake configured SDKs/build targets/test targets. Currently there's a sort of manual test using those host-specific options, but there should be a Python test ready to pick up when build-script-impl finally goes away.
  • I can barely understand BuildScriptInvocation. I find that it's almost like C code - one function calls another function calls another one, and there's a lot of unpicking required to understand what's going on.
  • I think that a better model might be to remove the validation and defaults from BuildScriptInvocation and make it solely responsible for workflow. It would initialise with a list of HostSpecificConfigurations, as well as common objects (like toolchain, maybe CMake, whatever else it needs...), and be responsible for iterating them and calling their build()/test()/etc methods.
    For example, the idea on the mailing list about toolchain-based building could be done with a simple change to BuildScriptInvocation - just installing products at a different point in the workflow.
  • Both HostSpecificConfiguration and BuildScriptInvocation would be more clearly defined after that, and much easier to test IMO.

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 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:

  1. The reason I didn't put HostSpecificConfiguration into its own file was because its logic was so intimately tied to the current args set (which we agree is insufficient). I didn't want to have a constructor in swift_build_support that just used a grab bag of args items, and I also didn't want to try and explode all of the things it currently uses into separate arguments (or its own object). I'm not sure how to resolve this difficulty currently, but agree it would be good to have something better and have tests. Do you have ideas?
  2. I agree about making everything store more structured objects instead of names, and will get to that (eventually). I'm happy to have the [CMake] Replace SWIFT_SDKS with SWIFT_CROSS_COMPILE_DEPLOYMENT_TARGETS #2835 version of this.
  3. I agree we should have an object that represents the parsed arguments better than the existing invocation, and that object should live inside swift_build_support. Having Invocation be the sole object which understand the "command line arguments" and turns that into structured information makes sense to me though, and matches a convention we use inside the Clang and Swift compilers.

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():
Expand Down Expand Up @@ -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")
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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 ---')
Expand Down
19 changes: 19 additions & 0 deletions utils/swift_build_support/swift_build_support/products/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,27 @@
#
# ----------------------------------------------------------------------------

from .cmark import CMark
from .foundation import Foundation
from .libdispatch import LibDispatch
from .llbuild import LLBuild
from .lldb import LLDB
from .llvm import LLVM
from .ninja import Ninja
from .swift import Swift
from .swiftpm import SwiftPM
from .xctest import XCTest

__all__ = [
'CMark',
'Ninja',
'Foundation',
'LibDispatch',
'LLBuild',
'LLDB',
'LLVM',
'Ninja',
'Swift',
'SwiftPM',
'XCTest',
]
17 changes: 17 additions & 0 deletions utils/swift_build_support/swift_build_support/products/cmark.py
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
17 changes: 17 additions & 0 deletions utils/swift_build_support/swift_build_support/products/llbuild.py
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
17 changes: 17 additions & 0 deletions utils/swift_build_support/swift_build_support/products/lldb.py
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
17 changes: 17 additions & 0 deletions utils/swift_build_support/swift_build_support/products/llvm.py
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
10 changes: 2 additions & 8 deletions utils/swift_build_support/swift_build_support/products/ninja.py
Original file line number Diff line number Diff line change
Expand Up @@ -18,18 +18,12 @@
import platform
import sys

from . import product
from .. import cache_util
from .. import shell


class Ninja(object):

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

class Ninja(product.Product):
@cache_util.reify
def ninja_bin_path(self):
return os.path.join(self.build_dir, 'ninja')
Expand Down
32 changes: 32 additions & 0 deletions utils/swift_build_support/swift_build_support/products/product.py
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
17 changes: 17 additions & 0 deletions utils/swift_build_support/swift_build_support/products/swift.py
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
Loading