Skip to content

Commit

Permalink
Print server side messages, if sent by the RE
Browse files Browse the repository at this point in the history
  • Loading branch information
Vasko Mitanov authored and vaskomitanov committed Sep 13, 2024
1 parent b91a610 commit 2968c74
Show file tree
Hide file tree
Showing 9 changed files with 94 additions and 12 deletions.
50 changes: 47 additions & 3 deletions app/buck2_build_api/src/actions/calculation.rs
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ use buck2_build_signals::NodeDuration;
use buck2_common::events::HasEvents;
use buck2_core::base_deferred_key::BaseDeferredKey;
use buck2_core::target::configured_target_label::ConfiguredTargetLabel;
use buck2_data::ActionErrorDiagnostics;
use buck2_data::{action_error_diagnostics, ActionErrorDiagnostics};
use buck2_data::ActionSubErrors;
use buck2_data::ToProtoMessage;
use buck2_events::dispatch::async_record_root_spans;
Expand Down Expand Up @@ -57,6 +57,7 @@ use crate::actions::error_handler::ActionSubErrorResult;
use crate::actions::error_handler::StarlarkActionErrorContext;
use crate::actions::execute::action_executor::ActionOutputs;
use crate::actions::execute::action_executor::HasActionExecutor;
use crate::actions::execute::error::ExecuteError;
use crate::actions::RegisteredAction;
use crate::artifact_groups::calculation::ensure_artifact_group_staged;
use crate::deferred::calculation::lookup_deferred_holder;
Expand Down Expand Up @@ -255,7 +256,10 @@ async fn build_action_no_redirect(

let last_command = commands.last().cloned();

let error_diagnostics = try_run_error_handler(action.dupe(), last_command.as_ref());
let error_diagnostics = build_error_diagnostics(
&e,
try_run_error_handler(action.dupe(), last_command.as_ref())
);

let e = ActionError::new(
e,
Expand Down Expand Up @@ -357,10 +361,50 @@ async fn build_action_no_redirect(
},
spans,
})?;

action_execution_data.action_result
}

fn build_error_diagnostics(execute_error: &ExecuteError, error_diagnostics: Option<ActionErrorDiagnostics>) -> Option<ActionErrorDiagnostics> {
let ExecuteError::CommandExecutionError { error: Some(command_execute_error) } = execute_error else {
return error_diagnostics;
};
let action_sub_error = || buck2_data::ActionSubError {
category: "ServerMessage".to_string(),
message: Some(format!("{}", command_execute_error)),
locations: None,
};
if let Some(inner_error_diagnostics) = &error_diagnostics {
if let Some(error_diagnostics_data) = &inner_error_diagnostics.data {
match error_diagnostics_data {
action_error_diagnostics::Data::SubErrors(action_sub_errors) => {
let sub_errors = action_sub_errors
.sub_errors
.iter()
.chain([&action_sub_error()])
.cloned()
.collect();
Some(ActionErrorDiagnostics {
data: Some(action_error_diagnostics::Data::SubErrors(ActionSubErrors { sub_errors })),
})
},
action_error_diagnostics::Data::HandlerInvocationError(handler_error) => {
Some(ActionErrorDiagnostics {
data: Some(action_error_diagnostics::Data::HandlerInvocationError(format!("{}\n{}", handler_error, command_execute_error))),
})
},
}
} else {
Some(ActionErrorDiagnostics {
data: Some(action_error_diagnostics::Data::SubErrors(ActionSubErrors { sub_errors: vec![action_sub_error()] })),
})
}
} else {
Some(ActionErrorDiagnostics {
data: Some(action_error_diagnostics::Data::SubErrors(ActionSubErrors { sub_errors: vec![action_sub_error()] })),
})
}
}

// Attempt to run the error handler if one was specified. Returns either the error diagnostics, or
// an actual error if the handler failed to run successfully.
fn try_run_error_handler(
Expand Down
1 change: 0 additions & 1 deletion app/buck2_build_api/src/actions/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@
*/

use std::fmt;

use buck2_event_observer::display::display_action_error;
use buck2_event_observer::display::TargetDisplayOptions;

Expand Down
10 changes: 9 additions & 1 deletion app/buck2_build_api/src/actions/execute/action_executor.rs
Original file line number Diff line number Diff line change
Expand Up @@ -462,7 +462,15 @@ impl ActionExecutionCtx for BuckActionExecutionContext<'_> {
error: Some(error.clone()),
})
}
_ => Err(ExecuteError::CommandExecutionError { error: None }),
_ => {
#[derive(Display, Debug, thiserror::Error)]
struct ServerError {
message: String,
}
Err(ExecuteError::CommandExecutionError { error: result.server_message.map(|server_message| buck2_error::Error::new(ServerError{
message: server_message,
})) })
},
};
self.command_reports.extend(rejected_execution);
self.command_reports.push(report);
Expand Down
35 changes: 31 additions & 4 deletions app/buck2_client_ctx/src/subscribers/superconsole.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ use std::time::Duration;

use anyhow::Context as _;
use async_trait::async_trait;
use buck2_data::CommandExecutionDetails;
use buck2_data::{action_error_diagnostics, ActionError, ActionErrorDiagnostics, CommandExecutionDetails};
use buck2_event_observer::display;
use buck2_event_observer::display::display_file_watcher_end;
use buck2_event_observer::display::TargetDisplayOptions;
Expand Down Expand Up @@ -621,7 +621,7 @@ impl StatefulSuperConsoleImpl {
async fn handle_action_error(&mut self, error: &buck2_data::ActionError) -> anyhow::Result<()> {
let mut lines = vec![];
let display_platform = self.state.config.display_platform;

Self::write_diagnostics(&error, &mut lines);
let display::ActionErrorDisplay {
action_id,
reason,
Expand Down Expand Up @@ -650,12 +650,39 @@ impl StatefulSuperConsoleImpl {
if let Some(command) = command {
lines_for_command_details(&command, self.verbosity, &mut lines);
}

self.super_console.emit(Lines(lines));

Ok(())
}

fn write_diagnostics(error: &ActionError, lines: &mut Vec<Line>) {
if let Some(ActionErrorDiagnostics {data: Some(data)} ) = &error.error_diagnostics {
match data {
action_error_diagnostics::Data::SubErrors(action_sub_errors) => {
for sub_error in &action_sub_errors.sub_errors {
if let Some(message) = &sub_error.message {
lines.push(Line::from_iter([Span::new_styled_lossy(StyledContent::new(
ContentStyle {
foreground_color: Some(Color::Yellow),
..Default::default()
},
format!("{}: {:?}", sub_error.category, message),
))]));
}
}
},
action_error_diagnostics::Data::HandlerInvocationError(handler_error) => {
lines.push(Line::from_iter([Span::new_styled_lossy(StyledContent::new(
ContentStyle {
foreground_color: Some(Color::Yellow),
..Default::default()
},
format!("{:?}", handler_error),
))]));
}
}
}
}

async fn handle_test_result(&mut self, result: &buck2_data::TestResult) -> anyhow::Result<()> {
if let Some(msg) = display::format_test_result(result)? {
self.super_console.emit(msg);
Expand Down
2 changes: 2 additions & 0 deletions app/buck2_execute/src/execute/manager.rs
Original file line number Diff line number Diff line change
Expand Up @@ -143,6 +143,7 @@ impl CommandExecutionManagerLike for CommandExecutionManager {
eligible_for_full_hybrid: false,
dep_file_metadata: None,
action_result: None,
server_message: None,
}
}

Expand Down Expand Up @@ -223,6 +224,7 @@ impl CommandExecutionManagerLike for CommandExecutionManagerWithClaim {
eligible_for_full_hybrid: false,
dep_file_metadata: None,
action_result: None,
server_message: None,
}
}

Expand Down
1 change: 1 addition & 0 deletions app/buck2_execute/src/execute/result.rs
Original file line number Diff line number Diff line change
Expand Up @@ -196,6 +196,7 @@ pub struct CommandExecutionResult {
/// to be re-used when uploading the remote dep file.
#[derivative(Debug = "ignore")]
pub action_result: Option<TActionResult2>,
pub server_message: Option<String>,
}

impl CommandExecutionResult {
Expand Down
5 changes: 2 additions & 3 deletions app/buck2_execute_impl/src/executors/re.rs
Original file line number Diff line number Diff line change
Expand Up @@ -189,9 +189,9 @@ impl ReExecutor {
platform: platform.clone(),
remote_dep_file_key: None,
};

let execution_kind = response.execution_kind(remote_details);
let manager = manager.with_execution_kind(execution_kind.clone());

if response.status.code != TCode::OK {
let res = if let Some(out) = as_missing_outputs_error(&response.status) {
// TODO: Add a dedicated report variant for this.
Expand Down Expand Up @@ -236,7 +236,6 @@ impl ReExecutor {
error_type,
)
};

return ControlFlow::Break(res);
}

Expand Down Expand Up @@ -357,9 +356,9 @@ impl PreparedCommandExecutor for ReExecutor {
)
.boxed()
.await;

let DownloadResult::Result(mut res) = res;
res.action_result = Some(response.action_result);
res.server_message = if response.message.is_empty() { None } else { Some(response.message) };
res
}

Expand Down
1 change: 1 addition & 0 deletions remote_execution/oss/re_grpc/src/client.rs
Original file line number Diff line number Diff line change
Expand Up @@ -664,6 +664,7 @@ impl REClient {
},
cached_result: execute_response_grpc.cached_result,
action_digest: Default::default(), // Filled in below.
message: execute_response_grpc.message,
};

ExecuteWithProgressResponse {
Expand Down
1 change: 1 addition & 0 deletions remote_execution/oss/re_grpc/src/response.rs
Original file line number Diff line number Diff line change
Expand Up @@ -176,6 +176,7 @@ pub struct ExecuteResponse {
pub action_digest: TDigest,
pub action_result_digest: TDigest,
pub action_result_ttl: i64,
pub message: String,
}

#[derive(Clone, Default)]
Expand Down

0 comments on commit 2968c74

Please sign in to comment.