Skip to content

[build-script] Introduce ProductBuilder. Transform Ninja to use it. #23822

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
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
7 changes: 4 additions & 3 deletions utils/build-script
Original file line number Diff line number Diff line change
Expand Up @@ -444,11 +444,12 @@ class BuildScriptInvocation(object):
"can't find source directory for ninja "
"(tried %s)" % (self.workspace.source_dir("ninja")))

ninja_build = products.Ninja(
ninja_build = products.Ninja.new_builder(
args=self.args,
toolchain=self.toolchain,
source_dir=self.workspace.source_dir("ninja"),
build_dir=self.workspace.build_dir("build", "ninja"))
workspace=self.workspace,
host=StdlibDeploymentTarget.get_target_for_name(
Copy link
Member

Choose a reason for hiding this comment

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

None of this is your doing, I'm wondering if you know the history. Why is this in StdlibDeploymentTarget? Why is this being built for the host_target and not build_target? This is the tool being built for the build right?

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 think StdlibDeploymentTarget is a little misnamed at this point, because it is used for more things that just the stdlib. If you think about it as CompilationTarget or DeploymentTarget is easier to understand.

Ninja is build only for the machine building the rest of the pieces (it is build if it cannot be found in the toolchain/path), so it is fine to “hardcode” here to host_target (which is the current machine).

If you look at the implementation of make_builder, it actually ignores the target. The previous implementation didn’t care about the target (so it builds for the local target).

PD: there's a big mess of names in the build script. Sometimes host refers to the device that run the code (arguments --host-test and --skip-test-ios-host which control to run or not tests on the external devices), but sometimes refers to the machine doing the build (like this host_target and arguments like --build-runtime-with-host-compiler and --host-cc).

Copy link
Member

Choose a reason for hiding this comment

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

Right, I figured it was a poorly named variable at this point. I was thinking of it as TargetInformation. I was wondering if you knew why this had come to be poorly named at this point.

As to the Ninja bits, right - its again a name thing. target is the target where generated code will be run, host is where the generated code generator will run, and build is the machine where the build is occurring. These are well documented names, so it was a question of does the current machinery not name things accordingly?

I think that making it deal with cross-compilation is great thing, but way outside the scope of this current change .

Copy link
Contributor Author

Choose a reason for hiding this comment

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

About the renaming, from comments and clues in the code, I think there was a previous try to make everything more standard, but it was necessary to keep the argument names the same for compatibility, which probably makes things a little bit more complicated. I can see if something can be done as a cleanup when all these pieces fall into place (I prefer to have the “wrong” names, since they match better what the build-script-impl have).

self.args.host_target))
ninja_build.build()
self.toolchain.ninja = ninja_build.ninja_bin_path

Expand Down
14 changes: 14 additions & 0 deletions utils/swift_build_support/swift_build_support/products/ninja.py
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,20 @@ class Ninja(product.Product):
def is_build_script_impl_product(cls):
return False

@classmethod
def new_builder(cls, args, toolchain, workspace, host):
return NinjaBuilder(cls, args, toolchain, workspace)


class NinjaBuilder(product.ProductBuilder):
def __init__(self, product_class, args, toolchain, workspace):
self.source_dir = workspace.source_dir(
product_class.product_source_name())
self.build_dir = workspace.build_dir('build',
product_class.product_name())
self.args = args
self.toolchain = toolchain

@cache_util.reify
def ninja_bin_path(self):
return os.path.join(self.build_dir, 'ninja')
Expand Down
75 changes: 75 additions & 0 deletions utils/swift_build_support/swift_build_support/products/product.py
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@
#
# ----------------------------------------------------------------------------

import abc


class Product(object):
@classmethod
Expand Down Expand Up @@ -58,3 +60,76 @@ def __init__(self, args, toolchain, source_dir, build_dir):
self.source_dir = source_dir
self.build_dir = build_dir
self.cmake_options = []


class ProductBuilder(object):
"""
Abstract base class for all ProductBuilders.

An specific ProductBuilder will implement the interface methods depending
how the product want to be build. Multiple products can use the same
product builder if parametrized right (for example all the products build
using CMake).

Ideally a ProductBuilder will be initialized with references to the
invocation arguments, the calculated toolchain, the calculated workspace,
and the target host, but the base class doesn't impose those requirements
in order to be flexible.

NOTE: Python doesn't need an explicit abstract base class, but it helps
documenting the interface.
"""

@abc.abstractmethod
def __init__(self, product_class, args, toolchain, workspace):
"""
Create a product builder for the given product class.

Parameters
----------
product_class : class
A subtype of `Product` which describes the product being built by
this builder.
args : `argparse.Namespace`
The arguments passed by the user to the invocation of the script. A
builder should consider this argument read-only.
toolchain : `swift_build_support.toolchain.Toolchain`
The toolchain being used to build the product. The toolchain will
point to the tools that the builder should use to build (like the
compiler or the linker).
workspace : `swift_build_support.workspace.Workspace`
The workspace where the source code and the build directories have
to be located. A builder should use the workspace to access its own
source/build directory, as well as other products source/build
directories.
"""
pass

@abc.abstractmethod
def build(self):
"""
Perform the build phase for the product.

This phase might also imply a configuration phase, but each product
builder is free to determine how to do it.
"""
pass

@abc.abstractmethod
def test(self):
"""
Perform the test phase for the product.

This phase might build and execute the product tests.
"""
pass

@abc.abstractmethod
def install(self):
"""
Perform the install phase for the product.

This phase might copy the artifacts from the previous phases into a
destination directory.
"""
pass
29 changes: 17 additions & 12 deletions utils/swift_build_support/tests/products/test_ninja.py
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
from swift_build_support import shell
from swift_build_support import xcrun
from swift_build_support.products import Ninja
from swift_build_support.targets import StdlibDeploymentTarget
from swift_build_support.toolchain import host_toolchain
from swift_build_support.workspace import Workspace

Expand All @@ -41,6 +42,8 @@ def setUp(self):
self.workspace = Workspace(source_root=tmpdir1,
build_root=tmpdir2)

self.host = StdlibDeploymentTarget.host_target()

# Setup toolchain
self.toolchain = host_toolchain()
self.toolchain.cc = '/path/to/cc'
Expand Down Expand Up @@ -71,20 +74,23 @@ def tearDown(self):
self.args = None

def test_ninja_bin_path(self):
ninja_build = Ninja(
ninja_build = Ninja.new_builder(
args=self.args,
toolchain=self.toolchain,
source_dir='/path/to/src',
build_dir='/path/to/build')
workspace=self.workspace,
host=self.host)

self.assertEqual(ninja_build.ninja_bin_path, '/path/to/build/ninja')
self.assertEqual(ninja_build.ninja_bin_path,
os.path.join(
self.workspace.build_dir('build', 'ninja'),
'ninja'))

def test_build(self):
ninja_build = Ninja(
ninja_build = Ninja.new_builder(
args=self.args,
toolchain=self.toolchain,
source_dir=self.workspace.source_dir('ninja'),
build_dir=self.workspace.build_dir('build', 'ninja'))
workspace=self.workspace,
host=self.host)

ninja_build.build()

Expand Down Expand Up @@ -113,8 +119,7 @@ def test_build(self):
+ pushd {build_dir}
+ {expect_env}{python} configure.py --bootstrap
+ popd
""".format(
source_dir=os.path.join(self.workspace.source_root, 'ninja'),
build_dir=os.path.join(self.workspace.build_root, 'ninja-build'),
expect_env=expect_env,
python=sys.executable))
""".format(source_dir=self.workspace.source_dir('ninja'),
build_dir=self.workspace.build_dir('build', 'ninja'),
expect_env=expect_env,
python=sys.executable))