Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Print server side messages, if sent by the RE #775

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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
Loading