From 88f96e63cb1c424de4f83bf1c6f757fb504184a9 Mon Sep 17 00:00:00 2001 From: Alex Hornby Date: Fri, 7 Jan 2022 01:31:45 -0800 Subject: [PATCH] extract get_dependencies method Summary: A number of places were extracting dependencies from manifests, but only one was adding in the implicit dependencies for build tools. Extract the logic to one place and use so that a change in a tool like cmake will now correctly affect all tools using cmake, as it will be taken into account as a dependency hash when the manifest's hash is computed. Tests for this change revealed that install_dirs needed to be populated in reverse order from the manifest topo-sort, so have also addressed that Reviewed By: wittgenst Differential Revision: D32730717 fbshipit-source-id: 1b2a25e460de6085d274c99acfd391b3bd259264 --- build/fbcode_builder/getdeps.py | 7 +++++-- build/fbcode_builder/getdeps/builder.py | 6 +++--- build/fbcode_builder/getdeps/load.py | 16 ++------------ build/fbcode_builder/getdeps/manifest.py | 21 +++++++++++++++++-- .../getdeps/test/manifest_test.py | 2 +- build/fbcode_builder/manifests/fizz | 2 +- 6 files changed, 31 insertions(+), 23 deletions(-) diff --git a/build/fbcode_builder/getdeps.py b/build/fbcode_builder/getdeps.py index 6d4b047ca..2d4d9fb40 100755 --- a/build/fbcode_builder/getdeps.py +++ b/build/fbcode_builder/getdeps.py @@ -581,7 +581,10 @@ class BuildCmd(ProjectCmdBase): elif args.verbose: print("found good %s" % built_marker) - install_dirs.append(inst_dir) + # Paths are resolved from front. We prepend rather than append as + # the last project in topo order is the project itself, which + # should be first in the path, then its deps and so on. + install_dirs.insert(0, inst_dir) def compute_dep_change_status(self, m, built_marker, loader): reconfigure = False @@ -589,7 +592,7 @@ class BuildCmd(ProjectCmdBase): st = os.lstat(built_marker) ctx = loader.ctx_gen.get_context(m.name) - dep_list = sorted(m.get_section_as_dict("dependencies", ctx).keys()) + dep_list = m.get_dependencies(ctx) for dep in dep_list: if reconfigure and sources_changed: break diff --git a/build/fbcode_builder/getdeps/builder.py b/build/fbcode_builder/getdeps/builder.py index 73d1701a0..fd55779f1 100644 --- a/build/fbcode_builder/getdeps/builder.py +++ b/build/fbcode_builder/getdeps/builder.py @@ -1359,12 +1359,12 @@ incremental = false is also cargo-builded and if yes then extract it's git configs and install dir """ - dependencies = self.manifest.get_section_as_dict("dependencies", ctx=self.ctx) + dependencies = self.manifest.get_dependencies(self.ctx) if not dependencies: return [] dep_to_git = {} - for dep in dependencies.keys(): + for dep in dependencies: dep_manifest = self.loader.load_manifest(dep) dep_builder = dep_manifest.get("build", "builder", ctx=self.ctx) if dep_builder not in ["cargo", "nop"] or dep == "rust": @@ -1374,7 +1374,7 @@ incremental = false # toolchain. continue - git_conf = dep_manifest.get_section_as_dict("git", ctx=self.ctx) + git_conf = dep_manifest.get_section_as_dict("git", self.ctx) if "repo_url" not in git_conf: raise Exception( "A cargo dependency requires git.repo_url to be defined." diff --git a/build/fbcode_builder/getdeps/load.py b/build/fbcode_builder/getdeps/load.py index 2e716c4ed..e25803379 100644 --- a/build/fbcode_builder/getdeps/load.py +++ b/build/fbcode_builder/getdeps/load.py @@ -192,19 +192,7 @@ class ManifestLoader(object): # to produce the same order regardless of how they are listed # in the project manifest files. ctx = self.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 in ("cmake", "python-wheel"): - dep_list.append("cmake") - elif builder == "autoconf" and m.name not in ( - "autoconf", - "libtool", - "automake", - ): - # they need libtool and its deps (automake, autoconf) so add - # those as deps (but obviously not if we're building those - # projects themselves) - dep_list.append("libtool") + dep_list = m.get_dependencies(ctx) dep_count = 0 for dep_name in dep_list: @@ -303,7 +291,7 @@ class ManifestLoader(object): manifest.update_hash(hasher, ctx) - dep_list = sorted(manifest.get_section_as_dict("dependencies", ctx).keys()) + dep_list = manifest.get_dependencies(ctx) for dep in dep_list: dep_manifest = self.load_manifest(dep) dep_hash = self.get_project_hash(dep_manifest) diff --git a/build/fbcode_builder/getdeps/manifest.py b/build/fbcode_builder/getdeps/manifest.py index eff2af630..21c1502e8 100644 --- a/build/fbcode_builder/getdeps/manifest.py +++ b/build/fbcode_builder/getdeps/manifest.py @@ -246,6 +246,24 @@ class ManifestParser(object): return defval + def get_dependencies(self, ctx): + dep_list = list(self.get_section_as_dict("dependencies", ctx).keys()) + dep_list.sort() + builder = self.get("build", "builder", ctx=ctx) + if builder in ("cmake", "python-wheel"): + dep_list.insert(0, "cmake") + elif builder == "autoconf" and self.name not in ( + "autoconf", + "libtool", + "automake", + ): + # they need libtool and its deps (automake, autoconf) so add + # those as deps (but obviously not if we're building those + # projects themselves) + dep_list.insert(0, "libtool") + + return dep_list + def get_section_as_args(self, section, ctx=None): """Intended for use with the make.[build_args/install_args] and autoconf.args sections, this method collects the entries and returns an @@ -290,9 +308,8 @@ class ManifestParser(object): res.append((key, value)) return res - def get_section_as_dict(self, section, ctx=None): + def get_section_as_dict(self, section, ctx): d = {} - ctx = ctx or {} for s in self._config.sections(): if s != section: diff --git a/build/fbcode_builder/getdeps/test/manifest_test.py b/build/fbcode_builder/getdeps/test/manifest_test.py index c5cd5bf34..7aa08d7f7 100644 --- a/build/fbcode_builder/getdeps/test/manifest_test.py +++ b/build/fbcode_builder/getdeps/test/manifest_test.py @@ -179,7 +179,7 @@ foo = bar foo = baz """, ) - self.assertEqual(p.get_section_as_dict("cmake.defines"), {"foo": "bar"}) + self.assertEqual(p.get_section_as_dict("cmake.defines", {}), {"foo": "bar"}) self.assertEqual( p.get_section_as_dict("cmake.defines", {"test": "on"}), {"foo": "baz"} ) diff --git a/build/fbcode_builder/manifests/fizz b/build/fbcode_builder/manifests/fizz index 1c0b15b56..15e14ec60 100644 --- a/build/fbcode_builder/manifests/fizz +++ b/build/fbcode_builder/manifests/fizz @@ -30,7 +30,7 @@ zlib zstd [dependencies.all(test=on, not(os=windows))] -googletest_1_8 +googletest [shipit.pathmap] fbcode/fizz/public_tld = .