Skip to content

Commit

Permalink
Append cert failures on build errors
Browse files Browse the repository at this point in the history
Summary:
We want to check for certs validity on build failure and present it to users so action can be taken. It'd be best to add this at a centralized location to prevent the possibility of the check being called multiple times, so the proposition to be appended to build errors so it will be visible to users and won't overwrite the actual error.

Users actually serious about fixing issues will definitely see the error and our tracking purposes, logview will just use the last error of the list

Reviewed By: JakobDegen

Differential Revision: D61342082

fbshipit-source-id: e4cf5307f3be32cff3d51927a1c58373feb52a18
  • Loading branch information
Will-MingLun-Li authored and facebook-github-bot committed Aug 18, 2024
1 parent 599aa08 commit a46ebb6
Show file tree
Hide file tree
Showing 9 changed files with 32 additions and 3 deletions.
10 changes: 10 additions & 0 deletions app/buck2_server/src/ctx.rs
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ use buck2_build_api::materialize::HasMaterializationQueueTracker;
use buck2_build_api::spawner::BuckSpawner;
use buck2_build_signals::CriticalPathBackendName;
use buck2_build_signals::HasCriticalPathBackend;
use buck2_certs::validate::CertState;
use buck2_cli_proto::client_context::HostArchOverride;
use buck2_cli_proto::client_context::HostPlatformOverride;
use buck2_cli_proto::client_context::PreemptibleWhen;
Expand Down Expand Up @@ -195,6 +196,9 @@ pub struct ServerCommandContext<'a> {
/// dropped.
heartbeat_guard_handle: Option<HeartbeatGuard>,

/// The current state of the certificate. This is used to detect errors due to invalid certs.
cert_state: CertState,

/// Daemon uuid passed in from the client side to detect nested invocation.
pub(crate) daemon_uuid_from_client: Option<String>,

Expand All @@ -217,6 +221,7 @@ impl<'a> ServerCommandContext<'a> {
starlark_profiler_instrumentation_override: StarlarkProfilerConfiguration,
build_options: Option<&CommonBuildOptions>,
paths: &InvocationPaths,
cert_state: CertState,
snapshot_collector: SnapshotCollector,
cancellations: &'a ExplicitCancellationContext,
) -> anyhow::Result<Self> {
Expand Down Expand Up @@ -303,6 +308,7 @@ impl<'a> ServerCommandContext<'a> {
oncall,
client_id_from_client_metadata,
_re_connection_handle: re_connection_handle,
cert_state,
starlark_profiler_instrumentation_override,
buck_out_dir: paths.buck_out_dir(),
isolation_prefix: paths.isolation.clone(),
Expand Down Expand Up @@ -768,6 +774,10 @@ impl<'a> ServerCommandContextTrait for ServerCommandContext<'a> {
&self.isolation_prefix
}

fn cert_state(&self) -> CertState {
self.cert_state.dupe()
}

fn project_root(&self) -> &ProjectRoot {
&self.base_context.project_root
}
Expand Down
1 change: 1 addition & 0 deletions app/buck2_server/src/daemon/server.rs
Original file line number Diff line number Diff line change
Expand Up @@ -443,6 +443,7 @@ impl BuckdServer {
opts.starlark_profiler_instrumentation_override(&req)?,
req.build_options(),
&daemon_state.paths,
cert_state.dupe(),
snapshot_collector,
cancellations,
)?;
Expand Down
1 change: 1 addition & 0 deletions app/buck2_server_commands/BUCK
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ rust_library(
"fbsource//third-party/rust:zstd",
"//buck2/app/buck2_artifact:buck2_artifact",
"//buck2/app/buck2_build_api:buck2_build_api",
"//buck2/app/buck2_certs:buck2_certs",
"//buck2/app/buck2_cli_proto:buck2_cli_proto",
"//buck2/app/buck2_common:buck2_common",
"//buck2/app/buck2_core:buck2_core",
Expand Down
1 change: 1 addition & 0 deletions app/buck2_server_commands/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ starlark_map = { workspace = true }

buck2_artifact = { workspace = true }
buck2_build_api = { workspace = true }
buck2_certs = { workspace = true }
buck2_cli_proto = { workspace = true }
buck2_common = { workspace = true }
buck2_core = { workspace = true }
Expand Down
4 changes: 3 additions & 1 deletion app/buck2_server_commands/src/commands/build.rs
Original file line number Diff line number Diff line change
Expand Up @@ -290,12 +290,14 @@ async fn process_build_result(

let result_reports = ResultReporter::convert(
&artifact_fs,
server_ctx.cert_state(),
ResultReporterOptions {
return_outputs: response_options.return_outputs,
return_default_other_outputs: response_options.return_default_other_outputs,
},
&build_result,
);
)
.await;

let serialized_build_report = if build_opts.unstable_print_build_report {
let build_report_opts = build_report_opts(&mut ctx, &cell_resolver, build_opts).await?;
Expand Down
13 changes: 11 additions & 2 deletions app/buck2_server_commands/src/commands/build/result_report.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@ use buck2_build_api::build::BuildProviderType;
use buck2_build_api::build::BuildTargetResult;
use buck2_build_api::build::ConfiguredBuildTargetResult;
use buck2_build_api::build::ProviderArtifacts;
use buck2_certs::validate::check_cert_state;
use buck2_certs::validate::CertState;
use buck2_core::configuration::compatibility::MaybeCompatible;
use buck2_core::fs::artifact_path_resolver::ArtifactFs;
use buck2_core::provider::label::ConfiguredProvidersLabel;
Expand Down Expand Up @@ -51,8 +53,9 @@ pub(crate) struct BuildTargetsAndErrors {
}

impl<'a> ResultReporter<'a> {
pub(crate) fn convert(
pub(crate) async fn convert(
artifact_fs: &'a ArtifactFs,
cert_state: CertState,
options: ResultReporterOptions,
build_result: &BuildTargetResult,
) -> BuildTargetsAndErrors {
Expand All @@ -75,7 +78,7 @@ impl<'a> ResultReporter<'a> {
out.collect_result(k, v);
}

let error_list = if let Some(e) = non_action_errors.pop() {
let mut error_list = if let Some(e) = non_action_errors.pop() {
// FIXME(JakobDegen): We'd like to return more than one error here, but we have
// to get better at error deduplication first
vec![e]
Expand All @@ -84,6 +87,12 @@ impl<'a> ResultReporter<'a> {
action_errors
};

if !error_list.is_empty() {
if let Some(e) = check_cert_state(cert_state).await {
error_list.push(e.into());
}
}

BuildTargetsAndErrors {
build_targets: out.results,
build_errors: BuildErrors { errors: error_list },
Expand Down
1 change: 1 addition & 0 deletions app/buck2_server_ctx/BUCK
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ rust_library(
"fbsource//third-party/rust:tracing",
"//buck2/allocative/allocative:allocative",
"//buck2/app/buck2_build_signals:buck2_build_signals",
"//buck2/app/buck2_certs:buck2_certs",
"//buck2/app/buck2_cli_proto:buck2_cli_proto",
"//buck2/app/buck2_common:buck2_common",
"//buck2/app/buck2_core:buck2_core",
Expand Down
1 change: 1 addition & 0 deletions app/buck2_server_ctx/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ gazebo = { workspace = true }

# Please do not add dependency on `buck2_build_api`.
buck2_build_signals = { workspace = true }
buck2_certs = { workspace = true }
buck2_cli_proto = { workspace = true }
buck2_common = { workspace = true }
buck2_core = { workspace = true }
Expand Down
3 changes: 3 additions & 0 deletions app/buck2_server_ctx/src/ctx.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ use async_trait::async_trait;
use buck2_build_signals::BuildSignalsContext;
use buck2_build_signals::DeferredBuildSignals;
use buck2_build_signals::HasCriticalPathBackend;
use buck2_certs::validate::CertState;
use buck2_cli_proto::client_context::PreemptibleWhen;
use buck2_core::fs::paths::file_name::FileName;
use buck2_core::fs::project::ProjectRoot;
Expand Down Expand Up @@ -49,6 +50,8 @@ pub trait ServerCommandContextTrait: Send + Sync {

fn project_root(&self) -> &ProjectRoot;

fn cert_state(&self) -> CertState;

fn materializer(&self) -> Arc<dyn Materializer>;

/// exposes the dice for scoped access, but isn't intended to be callable by anyone
Expand Down

0 comments on commit a46ebb6

Please sign in to comment.