rust-analyzer icon indicating copy to clipboard operation
rust-analyzer copied to clipboard

Remove RUSTC_WRAPPER hack

Open Veykril opened this issue 3 years ago • 15 comments

Closes https://github.com/rust-lang/rust-analyzer/issues/12973

Veykril avatar Aug 11 '22 09:08 Veykril

It feels like a PR/commit which could include a fair amount of context in the body :P

Absolutely not every commit should have a great commit message, but this probably shold

matklad avatar Aug 11 '22 09:08 matklad

Will add more context in a bit (doing some more timings testing right now)

Veykril avatar Aug 11 '22 09:08 Veykril

So I've just disabled rust-analyzer.cargo.buildScripts.useRustcWrapper to work around the anyhow issues (which apparently are still there on 1.0.61?), and that makes even more build scripts fail, or at least that's what the popup says.

When I look at the language server output, it seems like it's complaining about the crate I'm working on right now, which of course isn't always in a buildable state, but this is a binary crate without a build.rs so I'm quite puzzled. In any case, just wanted to let this be known, I'll try to see whether this reproduces in other workspaces / what could be wrong here.

jplatte avatar Aug 11 '22 09:08 jplatte

I've seen some weird build errors without the proxy which got fixed after a cargo clean, but you might be right.

lnicola avatar Aug 11 '22 09:08 lnicola

Ye, so I tested this on a random crate I had lying around https://github.com/djeedai/bevy_hanabi, current master takes ~20s to fully load metadata with build script set ups. With this PR its ~30s. And as jplatte mentioned, the problem is if a crate in the workspace is not in a buildable state, we will pop up an error even if build scripts work fine...

So I guess we have to stick to the wrapper still until cargo gives us an alternative

Veykril avatar Aug 11 '22 09:08 Veykril

Yeah, not using the rustc wrapper is a pretty bad experience right now. Just opened up another workspace with currently-non-buildable proc-macro code and the same error popped up, which I'm pretty sure was not the case before.

edit: No, actually this always happens when opening that workspace, regardless of the setting :(

jplatte avatar Aug 11 '22 09:08 jplatte

@jplatte you should probably test on a nightly which includes https://github.com/rust-lang/rust-analyzer/pull/12984/. That might help a little, but probably not as much as the wrapper.

lnicola avatar Aug 11 '22 09:08 lnicola

Yeah I'm gonna use the wrapper again. It only breaks anyhow, and only on non-nightly versions.

jplatte avatar Aug 11 '22 09:08 jplatte

current master takes ~20s to fully load metadata with build script set ups. With this PR its ~30s.

I'd say that 30% slowdown here is an acceptable cost to pay here to avoid questionable hacks.

For error reporting, I think we could add some filtering and just ignore errors from everything which is not compiled for $host?

matklad avatar Aug 11 '22 09:08 matklad

It not always 30%, if you disable check on save, it can take arbitrarily longer depending on how large the project is, right?

lnicola avatar Aug 11 '22 09:08 lnicola

It not always 30%, if you disable check on save, it can take arbitrarily longer depending on how large the project is, right?

Right: I think 2x slowdown would be OK, 10x would be too much. For a typical project, I would probably expect something closer to 2x.

And in theory rust-analyzer should work OK while proc-macros are not loaded, it's not like you are completely blocked.

:thinking: I wonder, can we maybe detect, by reading cargo messages, when all proc-macros have already compiled and kill the cargo thence? Probably not.

matklad avatar Aug 11 '22 09:08 matklad

We could just disable it by default, instead of immediately removing it?

flodiebold avatar Aug 11 '22 10:08 flodiebold

We could, but the only crate which doesn't build correctly under the wrapper happens to be quite popular, so most users would have to keep it disabled.

lnicola avatar Aug 11 '22 10:08 lnicola

It not always 30%, if you disable check on save, it can take arbitrarily longer depending on how large the project is, right?

Note I tested this with checkOnSave disabled on a project that had no build artifacts yet, but yes the slowdown will scale with project size, though bevy as a dependency is already pretty large i think.

Veykril avatar Aug 11 '22 10:08 Veykril

which apparently are still there on 1.0.61?

I can't reproduce the issue with anyhow 1.0.61 on either stable or nightly.

veber-alex avatar Aug 12 '22 13:08 veber-alex

:umbrella: The latest upstream changes (presumably #13010) made this pull request unmergeable. Please resolve the merge conflicts.

bors avatar Aug 13 '22 14:08 bors

I think this hasn't been a problem lately, and ideally we'll get a caego option (not that there's been much interest on the cargo side). So it makes sense to close this.

lnicola avatar Jan 10 '23 17:01 lnicola

Ye let's close this, so far no one of the cargo team has replied to the RFC unfortunately so thsi will probably be a while until we can remove this.

Veykril avatar Jan 10 '23 18:01 Veykril