[build] Delete api.build.with_spec()
Now that the vast majority of arguments to `api.build.with_options()`
are passed directly into fint via the fint params file,
`api.build.from_spec()` is just a tiny wrapper around
`api.build.with_options()` that only uses 3 fields of the build spec.
Having a separate `from_spec()` just makes it more likely that people
will be tempted to put new logic in `from_spec()` instead of
`with_options()`, leading to unnecessary divergence between fuchsia
builders and other builders; the incremental build cache setup logic
recently added to `from_spec()` is an excellent example of logic that's
not actually specific to the fuchsia recipes, but was put in
`from_spec()` anyway.
Bug: 67861
Change-Id: Ibf73de0de809388dbd56792fa4ab5bf47bf9c86a
Reviewed-on: https://fuchsia-review.googlesource.com/c/infra/recipes/+/513933
Commit-Queue: Oliver Newman <olivernewman@google.com>
Fuchsia-Auto-Submit: Oliver Newman <olivernewman@google.com>
Reviewed-by: Ina Huh <ihuh@google.com>
diff --git a/recipe_modules/build/api.py b/recipe_modules/build/api.py
index 0476d46..c7a357c 100644
--- a/recipe_modules/build/api.py
+++ b/recipe_modules/build/api.py
@@ -513,46 +513,6 @@
)
return ""
- def from_spec(
- self,
- build_spec,
- checkout,
- sdk_id=None,
- gcs_bucket=None,
- buildstats_upload_namespace=None,
- incremental=False,
- collect_coverage=False,
- allow_dirty=False,
- ):
- """Builds Fuchsia from a Jiri checkout.
-
- Args:
- build_spec (fuchsia_pb2.Fuchsia.Build): The input build spec.
- checkout (CheckoutApi.CheckoutResults): The Fuchsia checkout.
- sdk_id (str): If specified, set sdk_id in GN.
- gcs_bucket (str or None): The GCS bucket to upload results to, if set.
- buildstats_upload_namespace (str|None): The namespace to upload build
- stats to.
- incremental (bool): Whether or not to build incrementally.
- collect_coverage (bool): Whether to build for collecting coverage.
- allow_dirty (bool): Skip the ninja no op check.
-
- Returns:
- A FuchsiaBuildResults, representing the build.
- """
- with self.m.cache.guard("incremental") if incremental else self.m.nullcontext():
- return self.with_options(
- checkout=checkout,
- sdk_id=sdk_id,
- gcs_bucket=gcs_bucket,
- build_stats_gcs_bucket=build_spec.stats_gcs_bucket,
- upload_namespace=buildstats_upload_namespace,
- fint_params_path=build_spec.fint_params_path,
- incremental=incremental,
- collect_coverage=collect_coverage,
- allow_dirty=allow_dirty,
- )
-
def with_options(
self,
checkout,
@@ -582,7 +542,12 @@
"""
bb_input = self.m.buildbucket.build.input
- with self.m.step.nest("build") as presentation, self.m.macos_sdk():
+ if incremental:
+ cache_ctx = lambda: self.m.cache.guard("incremental")
+ else:
+ cache_ctx = self.m.nullcontext
+
+ with self.m.step.nest("build") as presentation, cache_ctx(), self.m.macos_sdk():
fint_params = self.m.file.read_text(
"read fint params",
checkout.root_dir.join(fint_params_path),
@@ -714,7 +679,7 @@
gn_results,
allow_dirty=False,
gcs_bucket=None,
- build_stats_gcs_bucket=None,
+ stats_gcs_bucket=None,
upload_namespace=None,
):
"""Runs `fint build`.
@@ -728,8 +693,7 @@
allow_dirty (bool): Skip the ninja no op check.
gcs_bucket (str or None): A GCS bucket to upload crash reports
to.
- build_stats_gcs_bucket (str): GCS bucket name to upload build
- stats to.
+ stats_gcs_bucket (str): GCS bucket name to upload build stats to.
upload_namespace (str): The namespace within the build stats GCS
bucket to upload to.
@@ -741,17 +705,14 @@
if gn_results.fint_set_artifacts.use_goma:
goma_context = self.m.goma.build_with_goma
else:
-
- @contextlib.contextmanager
- def goma_context():
- yield
+ goma_context = self.m.nullcontext
with goma_context():
try:
fint_build_artifacts = self._fint_build(
gn_results=gn_results,
allow_dirty=allow_dirty,
- build_stats_gcs_bucket=build_stats_gcs_bucket,
+ stats_gcs_bucket=stats_gcs_bucket,
upload_namespace=upload_namespace,
)
except self.m.step.StepFailure:
@@ -852,9 +813,7 @@
self.m.step("bootstrap fint", [bootstrap_path, "-o", fint_path])
return fint_path
- def _fint_build(
- self, gn_results, build_stats_gcs_bucket, upload_namespace, allow_dirty
- ):
+ def _fint_build(self, gn_results, stats_gcs_bucket, upload_namespace, allow_dirty):
context = copy.deepcopy(gn_results._fint_context)
context.skip_ninja_noop_check = allow_dirty
# TODO(olivernewman): Simplify the step naming. The nested
@@ -897,10 +856,10 @@
] = artifacts.ninja_duration_seconds
self._emitted_ninja_duration = True
- if build_stats_gcs_bucket and upload_namespace:
+ if stats_gcs_bucket and upload_namespace:
try:
self._upload_buildstats_output(
- gn_results, build_stats_gcs_bucket, upload_namespace, artifacts
+ gn_results, stats_gcs_bucket, upload_namespace, artifacts
)
except Exception as e:
self.m.step("upload buildstats failure", None).presentation.logs[
diff --git a/recipe_modules/build/examples/full.expected/clang_crash_in_fuchsia_and_buildstats_failure.json b/recipe_modules/build/examples/full.expected/clang_crash_in_fuchsia_and_buildstats_failure.json
index 34083a1..b6e6a7a 100644
--- a/recipe_modules/build/examples/full.expected/clang_crash_in_fuchsia_and_buildstats_failure.json
+++ b/recipe_modules/build/examples/full.expected/clang_crash_in_fuchsia_and_buildstats_failure.json
@@ -366,7 +366,7 @@
"name": "build.upload buildstats failure",
"~followup_annotations": [
"@@@STEP_NEST_LEVEL@1@@@",
- "@@@STEP_LOG_LINE@exception@Infra Failure: Step(u'build.upload fuchsia-buildstats.json to ###fuchsia-build###') (retcode: 1)@@@",
+ "@@@STEP_LOG_LINE@exception@Infra Failure: Step('build.upload fuchsia-buildstats.json to ###fuchsia-build###') (retcode: 1)@@@",
"@@@STEP_LOG_END@exception@@@"
]
},
diff --git a/recipe_modules/build/examples/full.expected/incremental_build.json b/recipe_modules/build/examples/full.expected/incremental_build.json
index 7b92a80..a8236a0 100644
--- a/recipe_modules/build/examples/full.expected/incremental_build.json
+++ b/recipe_modules/build/examples/full.expected/incremental_build.json
@@ -1,5 +1,14 @@
[
{
+ "cmd": [],
+ "name": "build",
+ "~followup_annotations": [
+ "@@@STEP_LOG_LINE@fint_params@field: \"value\"@@@",
+ "@@@STEP_LOG_END@fint_params@@@",
+ "@@@SET_BUILD_PROPERTY@fint_params@\"field: \\\"value\\\"\"@@@"
+ ]
+ },
+ {
"cmd": [
"vpython",
"-u",
@@ -11,21 +20,13 @@
"[CACHE]/incremental/.GUARD_FILE"
],
"infra_step": true,
- "name": "write guard file",
+ "name": "build.write guard file",
"~followup_annotations": [
+ "@@@STEP_NEST_LEVEL@1@@@",
"@@@STEP_LOG_END@.GUARD_FILE@@@"
]
},
{
- "cmd": [],
- "name": "build",
- "~followup_annotations": [
- "@@@STEP_LOG_LINE@fint_params@field: \"value\"@@@",
- "@@@STEP_LOG_END@fint_params@@@",
- "@@@SET_BUILD_PROPERTY@fint_params@\"field: \\\"value\\\"\"@@@"
- ]
- },
- {
"cmd": [
"vpython",
"-u",
@@ -610,7 +611,10 @@
"[CACHE]/incremental/.GUARD_FILE"
],
"infra_step": true,
- "name": "remove guard file"
+ "name": "build.remove guard file",
+ "~followup_annotations": [
+ "@@@STEP_NEST_LEVEL@1@@@"
+ ]
},
{
"cmd": [
diff --git a/recipe_modules/build/examples/full.expected/upload_buildstats_failure.json b/recipe_modules/build/examples/full.expected/upload_buildstats_failure.json
index 078d90a..3fe408c 100644
--- a/recipe_modules/build/examples/full.expected/upload_buildstats_failure.json
+++ b/recipe_modules/build/examples/full.expected/upload_buildstats_failure.json
@@ -364,7 +364,7 @@
"name": "build.upload buildstats failure",
"~followup_annotations": [
"@@@STEP_NEST_LEVEL@1@@@",
- "@@@STEP_LOG_LINE@exception@Infra Failure: Step(u'build.upload fuchsia-buildstats.json to ###fuchsia-build###') (retcode: 1)@@@",
+ "@@@STEP_LOG_LINE@exception@Infra Failure: Step('build.upload fuchsia-buildstats.json to ###fuchsia-build###') (retcode: 1)@@@",
"@@@STEP_LOG_END@exception@@@"
]
},
diff --git a/recipe_modules/build/examples/full.py b/recipe_modules/build/examples/full.py
index 37fe99e..74cc51d 100644
--- a/recipe_modules/build/examples/full.py
+++ b/recipe_modules/build/examples/full.py
@@ -6,8 +6,6 @@
from recipe_engine.config import List
from recipe_engine.recipe_api import Property
-from PB.infra.fuchsia import Fuchsia
-
DEPS = [
"fuchsia/artifacts",
"fuchsia/build",
@@ -73,18 +71,14 @@
source_info={},
)
- build_spec = Fuchsia.Build(
- upload_results=bool(gcs_bucket),
- fint_params_path="infra/specs/fint-params.textproto",
- stats_gcs_bucket=gcs_bucket,
- )
- build_results = api.build.from_spec(
- build_spec,
+ build_results = api.build.with_options(
checkout,
- sdk_id=sdk_id,
+ fint_params_path="infra/specs/fint-params.textproto",
gcs_bucket=gcs_bucket,
- buildstats_upload_namespace="namespace",
incremental=incremental,
+ sdk_id=sdk_id,
+ stats_gcs_bucket=gcs_bucket,
+ upload_namespace="namespace",
)
if not build_results:
assert not api.recipe_testing.enabled
diff --git a/recipes/fuchsia/build.expected/incremental_build.json b/recipes/fuchsia/build.expected/incremental_build.json
index 1d7c020..1d32955 100644
--- a/recipes/fuchsia/build.expected/incremental_build.json
+++ b/recipes/fuchsia/build.expected/incremental_build.json
@@ -786,6 +786,15 @@
]
},
{
+ "cmd": [],
+ "name": "build",
+ "~followup_annotations": [
+ "@@@STEP_LOG_LINE@fint_params@field: \"value\"@@@",
+ "@@@STEP_LOG_END@fint_params@@@",
+ "@@@SET_BUILD_PROPERTY@fint_params@\"field: \\\"value\\\"\"@@@"
+ ]
+ },
+ {
"cmd": [
"vpython",
"-u",
@@ -797,21 +806,13 @@
"[CACHE]/incremental/.GUARD_FILE"
],
"infra_step": true,
- "name": "write guard file",
+ "name": "build.write guard file",
"~followup_annotations": [
+ "@@@STEP_NEST_LEVEL@1@@@",
"@@@STEP_LOG_END@.GUARD_FILE@@@"
]
},
{
- "cmd": [],
- "name": "build",
- "~followup_annotations": [
- "@@@STEP_LOG_LINE@fint_params@field: \"value\"@@@",
- "@@@STEP_LOG_END@fint_params@@@",
- "@@@SET_BUILD_PROPERTY@fint_params@\"field: \\\"value\\\"\"@@@"
- ]
- },
- {
"cmd": [
"vpython",
"-u",
@@ -1334,7 +1335,10 @@
"[CACHE]/incremental/.GUARD_FILE"
],
"infra_step": true,
- "name": "remove guard file"
+ "name": "build.remove guard file",
+ "~followup_annotations": [
+ "@@@STEP_NEST_LEVEL@1@@@"
+ ]
},
{
"cmd": [],
diff --git a/recipes/fuchsia/build.py b/recipes/fuchsia/build.py
index d858388..40dabcb 100644
--- a/recipes/fuchsia/build.py
+++ b/recipes/fuchsia/build.py
@@ -225,15 +225,16 @@
# If SDK subbuild, set SDK ID to parent ID.
sdk_id = parent_id if spec.build.sdk_subbuild else None
- build_results = api.build.from_spec(
- spec.build,
+ build_results = api.build.with_options(
checkout,
- sdk_id=sdk_id,
- gcs_bucket=spec.gcs_bucket,
- buildstats_upload_namespace=upload_namespace,
- incremental=api.experimental.incremental_build,
- collect_coverage=collect_coverage,
+ spec.build.fint_params_path,
allow_dirty=without_cl,
+ collect_coverage=collect_coverage,
+ gcs_bucket=spec.gcs_bucket,
+ incremental=api.experimental.incremental_build,
+ sdk_id=sdk_id,
+ stats_gcs_bucket=spec.build.stats_gcs_bucket,
+ upload_namespace=upload_namespace,
)
postprocess_build(
diff --git a/recipes/fuchsia/determinism_check.py b/recipes/fuchsia/determinism_check.py
index 7a38d20..d3bc1a6 100644
--- a/recipes/fuchsia/determinism_check.py
+++ b/recipes/fuchsia/determinism_check.py
@@ -48,7 +48,9 @@
def run_build(api, spec):
"""Performs a single build of fuchsia and retrieves blobs.json."""
checkout = api.checkout.from_spec(spec.checkout, api.path.mkdtemp())
- build_results = api.build.from_spec(spec.build, checkout)
+ build_results = api.build.with_options(
+ checkout, fint_params_path=spec.build.fint_params_path
+ )
blobs_path = api.path.join(build_results.build_dir, "blobs.json")
return api.file.read_json("reading blobs", blobs_path)
