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

work around RAs RUSTC_WRAPPER hack #251

Closed
wants to merge 1 commit into from

Conversation

RalfJung
Copy link
Contributor

@RalfJung RalfJung commented Aug 8, 2022

Works around rust-lang/rust-analyzer#12973 by un-doing parts of what #157 intended to do, but only #249 actually did: honoring RUSTC_WRAPPER.

@jonhoo are you aware of any cases where ignoring RUSTC_WRAPPER actually causes problems? Seeing as autocfg ignores RUSTC_WRAPPER, it seems unlikely to me.

@RalfJung
Copy link
Contributor Author

RalfJung commented Aug 8, 2022

Note that this matches the behavior before #249, because anyhow used to check the wrong env var for this. 1.0.60 is the only version of anyhow that ever respected RUSTC_WRAPPER.

@jonhoo
Copy link
Sponsor Contributor

jonhoo commented Aug 9, 2022

I don't actually know of a use of RUSTC_WRAPPER, so can't really add much info. I've heard of it being used to always append certain rustc flags no matter which source is used for the rustc flags that Cargo decides to use, but don't have a concrete example I can link to.

Copy link
Owner

@dtolnay dtolnay left a comment

Choose a reason for hiding this comment

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

I would prefer to ignore only rust-analyzer's buggy RUSTC_WRAPPER specifically. Does it have some kind of recognizable name we could look for?

@RalfJung
Copy link
Contributor Author

RalfJung commented Aug 9, 2022

I would prefer to ignore only rust-analyzer's buggy RUSTC_WRAPPER specifically. Does it have some kind of recognizable name we could look for?

I'll have to forward that question to @lnicola

@lnicola
Copy link

lnicola commented Aug 9, 2022

Does it have some kind of recognizable name we could look for?

RA sets itself as the wrapper, so it should be called rust-analyzer or rust-analyzer.exe. However,

  • a tool based on RA might use the same approach
  • as mentioned in the issue, IntelliJ Rust takes the same approach (which is where we stole it from from)

@RalfJung
Copy link
Contributor Author

RalfJung commented Aug 9, 2022

This should help for RA, not sure if we want to start carrying a list of all the broken wrappers out there...

@jonhoo
Copy link
Sponsor Contributor

jonhoo commented Aug 9, 2022

I've found one use of RUSTC_WRAPPER in an internal code-base, and it's used to append to $PATH before delegating to the real rustc. And I'm not even sure that particular use is needed any more. That said, it does feel unfortunate to ignore RUSTC_WRAPPER entirely — even though I don't need it, chances are that if someone does set it it makes a difference to their use-case.

@RalfJung
Copy link
Contributor Author

RalfJung commented Aug 9, 2022

OTOH, given that autocfg is widely used and cuviper/autocfg#26 has seen very little activity, it doesn't seem to be terribly common for RUSTC_WRAPPER to be used in a way that just ignoring it causes a lot of trouble.

Copy link
Owner

@dtolnay dtolnay left a comment

Choose a reason for hiding this comment

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

I put up a different workaround in #252 since I think I would prefer not to sweep this under the rug yet as done by this PR.

I would be open to the workaround from this PR only if rust-lang/rust-analyzer#12973 is accepted as a rust-analyzer defect and we see progress toward resolving it, or if someone makes a compelling case that rust-lang/rust-analyzer#12973 is not a rust-analyzer defect and rust-analyzer will therefore not change behavior. The thing I don't want is for this workaround to land and then no follow-up ever occurs about the root cause.

@RalfJung
Copy link
Contributor Author

Sure, that also works. I locally disabled the rustc-wrapper hack anyway, just posted this because I felt bad for my previous PR triggering that problem.

@RalfJung RalfJung closed this Aug 11, 2022
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.

None yet

4 participants