From f1ed28a52ce9b9575467bac301be6239db17c9b0 Mon Sep 17 00:00:00 2001 From: Adam Simpkins Date: Wed, 31 Jul 2019 20:53:07 -0700 Subject: [PATCH] add a ManifestContext and ContextGenerator class Summary: Add a ContextGenerator class so that we actually use the correct per-project context when loading projects and computing dependencies. Previously commands like `build` and `test` would change the contexts for each project as they iterated through and performed the build. However, they did not do this when first loading the projects. This could cause them to use different context values when loading dependencies than when performing the build. For instance, this could cause issues if a project depends on `googletest` only when testing is enabled, as the code previously did not set the "test" parameter when evaluating dependencies. Reviewed By: pkaush Differential Revision: D16477396 fbshipit-source-id: c1e055f07de1cb960861d19594e3bda20a2ccd87 --- build/fbcode_builder/getdeps.py | 64 ++++++++++++----------- build/fbcode_builder/getdeps/buildopts.py | 20 +++++++ build/fbcode_builder/getdeps/load.py | 3 +- build/fbcode_builder/getdeps/manifest.py | 60 +++++++++++++++++++-- build/fbcode_builder/getdeps/platform.py | 19 ------- 5 files changed, 113 insertions(+), 53 deletions(-) diff --git a/build/fbcode_builder/getdeps.py b/build/fbcode_builder/getdeps.py index 2b5ea71a3..1564b0f1f 100755 --- a/build/fbcode_builder/getdeps.py +++ b/build/fbcode_builder/getdeps.py @@ -20,7 +20,7 @@ from getdeps.dyndeps import create_dyn_dep_munger from getdeps.errors import TransientFailure from getdeps.load import load_project, manifests_in_dependency_order from getdeps.manifest import ManifestParser -from getdeps.platform import HostType, context_from_host_tuple +from getdeps.platform import HostType from getdeps.subcmd import SubCmd, add_subcommands, cmd @@ -84,14 +84,14 @@ class FetchCmd(SubCmd): def run(self, args): opts = setup_build_options(args) + ctx_gen = opts.get_context_generator() manifest = load_project(opts, args.project) - ctx = context_from_host_tuple(args.host_type) if args.recursive: - projects = manifests_in_dependency_order(opts, manifest, ctx) + projects = manifests_in_dependency_order(opts, manifest, ctx_gen) else: projects = [manifest] for m in projects: - fetcher = m.create_fetcher(opts, ctx) + fetcher = m.create_fetcher(opts, ctx_gen.get_context(m.name)) fetcher.update() @@ -99,9 +99,10 @@ class FetchCmd(SubCmd): class ListDepsCmd(SubCmd): def run(self, args): opts = setup_build_options(args) + ctx_gen = opts.get_context_generator() + ctx_gen.set_value_for_project(args.project, "test", "on") manifest = load_project(opts, args.project) - ctx = context_from_host_tuple(args.host_type) - for m in manifests_in_dependency_order(opts, manifest, ctx): + for m in manifests_in_dependency_order(opts, manifest, ctx_gen): print(m.name) return 0 @@ -141,9 +142,10 @@ class CleanCmd(SubCmd): class ShowInstDirCmd(SubCmd): def run(self, args): opts = setup_build_options(args) + ctx_gen = opts.get_context_generator() + ctx_gen.set_value_for_project(args.project, "test", "on") manifest = load_project(opts, args.project) - ctx = context_from_host_tuple() - projects = manifests_in_dependency_order(opts, manifest, ctx) + projects = manifests_in_dependency_order(opts, manifest, ctx_gen) manifests_by_name = {m.name: m for m in projects} if args.recursive: @@ -152,6 +154,7 @@ class ShowInstDirCmd(SubCmd): manifests = [manifest] for m in manifests: + ctx = ctx_gen.get_context(m.name) fetcher = m.create_fetcher(opts, ctx) dirs = opts.compute_dirs(m, fetcher, manifests_by_name, ctx) inst_dir = dirs["inst_dir"] @@ -177,16 +180,17 @@ class ShowInstDirCmd(SubCmd): class ShowSourceDirCmd(SubCmd): def run(self, args): opts = setup_build_options(args) + ctx_gen = opts.get_context_generator() + ctx_gen.set_value_for_project(args.project, "test", "on") manifest = load_project(opts, args.project) - ctx = context_from_host_tuple() if args.recursive: - manifests = manifests_in_dependency_order(opts, manifest, ctx) + manifests = manifests_in_dependency_order(opts, manifest, ctx_gen) else: manifests = [manifest] for m in manifests: - fetcher = m.create_fetcher(opts, ctx) + fetcher = m.create_fetcher(opts, ctx_gen.get_context(m.name)) print(fetcher.get_src_dir()) def setup_parser(self, parser): @@ -212,11 +216,13 @@ class BuildCmd(SubCmd): if args.clean: clean_dirs(opts) + ctx_gen = opts.get_context_generator(facebook_internal=args.facebook_internal) + if args.enable_tests: + ctx_gen.set_value_for_project(args.project, "test", "on") manifest = load_project(opts, args.project) - ctx = context_from_host_tuple(facebook_internal=args.facebook_internal) - print("Building on %s" % ctx) - projects = manifests_in_dependency_order(opts, manifest, ctx) + print("Building on %s" % ctx_gen.get_context(args.project)) + projects = manifests_in_dependency_order(opts, manifest, ctx_gen) manifests_by_name = {m.name: m for m in projects} # Accumulate the install directories so that the build steps @@ -224,11 +230,7 @@ class BuildCmd(SubCmd): install_dirs = [] for m in projects: - ctx = dict(ctx) - if args.enable_tests and m.name == manifest.name: - ctx["test"] = "on" - else: - ctx["test"] = "off" + ctx = ctx_gen.get_context(m.name) fetcher = m.create_fetcher(opts, ctx) if args.clean: @@ -312,11 +314,13 @@ class BuildCmd(SubCmd): class FixupDeps(SubCmd): def run(self, args): opts = setup_build_options(args) + ctx_gen = opts.get_context_generator(facebook_internal=args.facebook_internal) + if args.enable_tests: + ctx_gen.set_value_for_project(args.project, "test", "on") manifest = load_project(opts, args.project) - ctx = context_from_host_tuple(facebook_internal=args.facebook_internal) - projects = manifests_in_dependency_order(opts, manifest, ctx) + projects = manifests_in_dependency_order(opts, manifest, ctx_gen) manifests_by_name = {m.name: m for m in projects} # Accumulate the install directories so that the build steps @@ -324,11 +328,7 @@ class FixupDeps(SubCmd): install_dirs = [] for m in projects: - ctx = dict(ctx) - if args.enable_tests and m.name == manifest.name: - ctx["test"] = "on" - else: - ctx["test"] = "off" + ctx = ctx_gen.get_context(m.name) fetcher = m.create_fetcher(opts, ctx) dirs = opts.compute_dirs(m, fetcher, manifests_by_name, ctx) @@ -370,11 +370,14 @@ class FixupDeps(SubCmd): class TestCmd(SubCmd): def run(self, args): opts = setup_build_options(args) - manifest = load_project(opts, args.project) + ctx_gen = opts.get_context_generator(facebook_internal=args.facebook_internal) + if args.test_all: + ctx_gen.set_value_for_all_projects("test", "on") + else: + ctx_gen.set_value_for_project(args.project, "test", "on") - ctx = context_from_host_tuple(facebook_internal=args.facebook_internal) - ctx["test"] = "on" - projects = manifests_in_dependency_order(opts, manifest, ctx) + manifest = load_project(opts, args.project) + projects = manifests_in_dependency_order(opts, manifest, ctx_gen) manifests_by_name = {m.name: m for m in projects} # Accumulate the install directories so that the test steps @@ -382,6 +385,7 @@ class TestCmd(SubCmd): install_dirs = [] for m in projects: + ctx = ctx_gen.get_context(m.name) fetcher = m.create_fetcher(opts, ctx) dirs = opts.compute_dirs(m, fetcher, manifests_by_name, ctx) diff --git a/build/fbcode_builder/getdeps/buildopts.py b/build/fbcode_builder/getdeps/buildopts.py index dd551af62..7e842d36e 100644 --- a/build/fbcode_builder/getdeps/buildopts.py +++ b/build/fbcode_builder/getdeps/buildopts.py @@ -18,6 +18,7 @@ import sys import tempfile from .envfuncs import Env, add_path_entry, path_search +from .manifest import ContextGenerator from .platform import HostType, is_windows @@ -128,6 +129,25 @@ class BuildOptions(object): def is_linux(self): return self.host_type.is_linux() + def get_context_generator(self, host_tuple=None, facebook_internal=False): + """ Create a manifest ContextGenerator for the specified target platform. """ + if host_tuple is None: + host_type = HostType() + elif isinstance(host_tuple, HostType): + host_type = host_tuple + else: + host_type = HostType.from_tuple_string(host_tuple) + + return ContextGenerator( + { + "os": host_type.ostype, + "distro": host_type.distro, + "distro_vers": host_type.distrovers, + "fb": "on" if facebook_internal else "off", + "test": "off", + } + ) + def _compute_hash(self, hash_by_name, manifest, manifests_by_name, ctx): """ This recursive function computes a hash for a given manifest. The hash takes into account some environmental factors on the diff --git a/build/fbcode_builder/getdeps/load.py b/build/fbcode_builder/getdeps/load.py index 26cb5139a..6665c8638 100644 --- a/build/fbcode_builder/getdeps/load.py +++ b/build/fbcode_builder/getdeps/load.py @@ -83,7 +83,7 @@ def load_all_manifests(build_opts): return LOADER.load_all(build_opts) -def manifests_in_dependency_order(build_opts, manifest, ctx): +def manifests_in_dependency_order(build_opts, manifest, ctx_gen): """ Given a manifest, expand its dependencies and return a list of the manifest objects that would need to be built in the order that they would need to be built. This does not evaluate whether @@ -109,6 +109,7 @@ def manifests_in_dependency_order(build_opts, manifest, ctx): # a correct order even if they aren't sorted, but we prefer # to produce the same order regardless of how they are listed # in the project manifest files. + ctx = ctx_gen.get_context(m.name) dep_list = sorted(m.get_section_as_dict("dependencies", ctx).keys()) builder = m.get("build", "builder", ctx=ctx) if builder == "cmake": diff --git a/build/fbcode_builder/getdeps/manifest.py b/build/fbcode_builder/getdeps/manifest.py index 41e637507..5613eae55 100644 --- a/build/fbcode_builder/getdeps/manifest.py +++ b/build/fbcode_builder/getdeps/manifest.py @@ -88,12 +88,10 @@ ALLOWED_EXPR_SECTIONS = [ "install.files", ] -ALLOWED_VARIABLES = {"os", "distro", "distro_vers", "fb", "test"} - def parse_conditional_section_name(name, section_def): expr = name[len(section_def) + 1 :] - return parse_expr(expr, ALLOWED_VARIABLES) + return parse_expr(expr, ManifestContext.ALLOWED_VARIABLES) def validate_allowed_fields(file_name, section, config, allowed_fields): @@ -404,3 +402,59 @@ class ManifestParser(object): ) raise KeyError("project %s has no known builder" % (self.name)) + + +class ManifestContext(object): + """ ProjectContext contains a dictionary of values to use when evaluating boolean + expressions in a project manifest. + + This object should be passed as the `ctx` parameter in ManifestParser.get() calls. + """ + + ALLOWED_VARIABLES = {"os", "distro", "distro_vers", "fb", "test"} + + def __init__(self, ctx_dict): + assert set(ctx_dict.keys()) == self.ALLOWED_VARIABLES + self.ctx_dict = ctx_dict + + def get(self, key): + return self.ctx_dict[key] + + def set(self, key, value): + assert key in self.ALLOWED_VARIABLES + self.ctx_dict[key] = value + + def copy(self): + return ManifestContext(dict(self.ctx_dict)) + + def __str__(self): + s = ", ".join( + "%s=%s" % (key, value) for key, value in sorted(self.ctx_dict.items()) + ) + return "{" + s + "}" + + +class ContextGenerator(object): + """ ContextGenerator allows creating ManifestContext objects on a per-project basis. + This allows us to evaluate different projects with slightly different contexts. + + For instance, this can be used to only enable tests for some projects. """ + + def __init__(self, default_ctx): + self.default_ctx = ManifestContext(default_ctx) + self.ctx_by_project = {} + + def set_value_for_project(self, project_name, key, value): + project_ctx = self.ctx_by_project.get(project_name) + if project_ctx is None: + project_ctx = self.default_ctx.copy() + self.ctx_by_project[project_name] = project_ctx + project_ctx.set(key, value) + + def set_value_for_all_projects(self, key, value): + self.default_ctx.set(key, value) + for ctx in self.ctx_by_project.values(): + ctx.set(key, value) + + def get_context(self, project_name): + return self.ctx_by_project.get(project_name, self.default_ctx) diff --git a/build/fbcode_builder/getdeps/platform.py b/build/fbcode_builder/getdeps/platform.py index d3368c2c4..27270c7e6 100644 --- a/build/fbcode_builder/getdeps/platform.py +++ b/build/fbcode_builder/getdeps/platform.py @@ -96,22 +96,3 @@ class HostType(object): and self.distro == b.distro and self.distrovers == b.distrovers ) - - -def context_from_host_tuple(host_tuple=None, facebook_internal=False): - """ Given an optional host tuple, construct a context appropriate - for passing to the boolean expression evaluator so that conditional - sections in manifests can be resolved. """ - if host_tuple is None: - host_type = HostType() - elif isinstance(host_tuple, HostType): - host_type = host_tuple - else: - host_type = HostType.from_tuple_string(host_tuple) - - return { - "os": host_type.ostype, - "distro": host_type.distro, - "distro_vers": host_type.distrovers, - "fb": "on" if facebook_internal else "off", - }