Skip to content

fix: Fix reporting of build script errors #10024

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Aug 25, 2021
Merged

fix: Fix reporting of build script errors #10024

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
095df7b
Don't fetch build script output twice
jonas-schievink Aug 25, 2021
0ff2c81
Fix reporting of build script errors
jonas-schievink Aug 25, 2021
276f6c6
Reduce visibility of `WorkspaceBuildScripts::run`
jonas-schievink Aug 25, 2021
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
6 changes: 5 additions & 1 deletion crates/project_model/src/build_scripts.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ pub(crate) struct BuildScriptOutput {
}

impl WorkspaceBuildScripts {
pub fn run(
pub(crate) fn run(
config: &CargoConfig,
workspace: &CargoWorkspace,
progress: &dyn Fn(String),
Expand Down Expand Up @@ -196,6 +196,10 @@ impl WorkspaceBuildScripts {

Ok(res)
}

pub fn error(&self) -> Option<&str> {
self.error.as_deref()
}
}

// FIXME: File a better way to know if it is a dylib.
Expand Down
32 changes: 17 additions & 15 deletions crates/rust-analyzer/src/reload.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -232,14 +232,6 @@ impl GlobalState {
let mut res = Vec::new();
for ws in workspaces.iter() {
res.push(ws.run_build_scripts(&config, &progress));
let ws = match ws {
ProjectWorkspace::Cargo { cargo, .. } => cargo,
ProjectWorkspace::DetachedFiles { .. } | ProjectWorkspace::Json { .. } => {
res.push(Ok(WorkspaceBuildScripts::default()));
continue;
}
};
res.push(WorkspaceBuildScripts::run(&config, ws, &progress))
}
sender.send(Task::FetchBuildData(BuildDataProgress::End((workspaces, res)))).unwrap();
});
Expand Down Expand Up @@ -453,19 +445,29 @@ impl GlobalState {
}

fn fetch_build_data_error(&self) -> Option<String> {
let mut buf = String::new();
let mut buf = "rust-analyzer failed to run build scripts:\n".to_string();
let mut has_errors = false;

for ws in &self.fetch_build_data_queue.last_op_result().1 {
if let Err(err) = ws {
stdx::format_to!(buf, "rust-analyzer failed to run custom build: {:#}\n", err);
match ws {
Ok(data) => {
if let Some(err) = data.error() {
has_errors = true;
stdx::format_to!(buf, "{:#}\n", err);
}
Comment on lines +454 to +457
Copy link
Contributor

@matklad matklad Aug 25, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was semi-intentional -- if cargo check fails because you have a type error in non-build.rs code, we should consider that as a working workspace. I think this is a non-issue given that we use RUSTC_WRAPPER trick, but it makes sense to re-check that you don't get partially loaded workspace notification

image

just becaues borrow checked doesn't favor you today.

Copy link
Contributor

@matklad matklad Aug 25, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This -- the absense of errore reporting for the cases wheer cargo check run succesfully, but returned a non-zero status

Copy link
Contributor Author

@jonas-schievink jonas-schievink Aug 25, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, this still seems to work correctly

Copy link
Contributor Author

@jonas-schievink jonas-schievink Aug 25, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Although rust-analyzer does not seem to recover when there is a build error in a build script, even after clicking on that item after fixing the issue

}
Err(err) => {
has_errors = true;
stdx::format_to!(buf, "{:#}\n", err);
}
}
}

if buf.is_empty() {
return None;
if has_errors {
Some(buf)
} else {
None
}

Some(buf)
}

fn reload_flycheck(&mut self) {
Expand Down