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

feat: Only flycheck workspace that belongs to saved file #12808

Merged
merged 5 commits into from
Aug 4, 2022

Conversation

Veykril
Copy link
Member

@Veykril Veykril commented Jul 18, 2022

Supercedes #11038

There is still the problem that all the diagnostics are cleared, only clearing diagnostics of the relevant workspace isn't easily doable though I think, will have to dig into that

@Veykril
Copy link
Member Author

Veykril commented Jul 20, 2022

Okay so I got a fix for the remaining problem, but somehow cargo is refusing to send diagnostics at some point. It works once and then never again for any but the last workspace...

@Veykril
Copy link
Member Author

Veykril commented Jul 20, 2022

Ye so cargo check just finishes without errors for all but the last workspace when repeatedly invoked even though those workspaces contain errors 😕

@bjorn3
Copy link
Member

bjorn3 commented Jul 20, 2022

What if one workspace depends on a crate from the other workspace? For example if I edit cranelift-codegen it would be nice if cg_clif gets recompiled. I can't put cranelift-codegen in cg_clif's workspace as it is already in a different workspace and as it is used as [patch.crates-io] replacement rather than a regular path = "..." dependency.

@Veykril
Copy link
Member Author

Veykril commented Jul 20, 2022

Okay, so I think I am hitting a cargo bug now? I basically have three workspaces all of which have errors in them. When r-a starts up it starts a flycheck process for each and we get diagnostics for all three. But due to how r-a works we redo this a few times at the start (like 2 or 3 times due to VFS change reloads), now here is where it gets weird, all following flychecks report successfull builds on the first two workspaces while the last one still errors, and this keeps happening unless the workspaces are flychecked on their own. So it seems cargo gets confused when multiple checks are running at the same time?

@lnicola
Copy link
Member

lnicola commented Jul 20, 2022

Why, yes. I actually complained about this, thinking it's a RA bug:

image

@Veykril
Copy link
Member Author

Veykril commented Jul 20, 2022

Lovely, so I guess we can't do this until that gets fixed. I assume there is no issue on the cargo issue tracker regarding this yet?

@lnicola
Copy link
Member

lnicola commented Jul 20, 2022

The thing is, I can't reproduce this (outside of RA) with cargo check --all-targets; cargo check.

@lnicola
Copy link
Member

lnicola commented Jul 20, 2022

We also have the RUSTC_WRAPPER thing, but I can't disable it ("failed to run build scripts").

@Veykril Veykril marked this pull request as ready for review August 4, 2022 11:05
@Veykril
Copy link
Member Author

Veykril commented Aug 4, 2022

Turns out triggering the cargo bug doesnt always occur and I changed some things around so we don't start a bunch of flycheck in the middle of fetching the workspace which should surpress the bug from occuring mostly. I'll merge this for now, we'll see if this causes trouble though from my testing it should work fine.
@bors r+

@bors
Copy link
Contributor

bors commented Aug 4, 2022

📌 Commit df7f755 has been approved by Veykril

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Aug 4, 2022

⌛ Testing commit df7f755 with merge 0fe3bcf...

@bors
Copy link
Contributor

bors commented Aug 4, 2022

☀️ Test successful - checks-actions
Approved by: Veykril
Pushing 0fe3bcf to master...

@bors bors merged commit 0fe3bcf into rust-lang:master Aug 4, 2022
@bors bors mentioned this pull request Aug 4, 2022
@Veykril Veykril deleted the check-workspace branch August 4, 2022 13:30
@@ -192,6 +192,7 @@ impl GlobalState {
if let Some(path) = vfs.file_path(file.file_id).as_path() {
let path = path.to_path_buf();
if reload::should_refresh_for_change(&path, file.change_kind) {
tracing::warn!("fetch-fiel_change");
Copy link
Member

Choose a reason for hiding this comment

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

Should be removed or changed to info!?

Copy link
Member Author

Choose a reason for hiding this comment

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

was already removed in a follow up

Copy link
Member

@lnicola lnicola Aug 8, 2022

Choose a reason for hiding this comment

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

Yeah, sorry, I actually missed it in #12947

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants