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

Update RLS to set __CARGO_TEST_ROOT when running its tests. #56177

Closed
wants to merge 1 commit into from

Conversation

eddyb
Copy link
Member

@eddyb eddyb commented Nov 23, 2018

That is, update RLS to include rust-lang/rls#1138.

This fixes fallout from #53586.

r? @alexcrichton cc @Mark-Simulacrum @nrc @pietroalbini

@rust-highfive
Copy link
Collaborator

⚠️ Warning ⚠️

  • These commits modify submodules.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 23, 2018
@alexcrichton
Copy link
Member

This is an internal environment variable used by cargo and not intended for other users, is there another way to fix this or perhaps something that can be officially supported by cargo?

The double underscore was supposed to indicate that...

@eddyb
Copy link
Member Author

eddyb commented Nov 23, 2018

@alexcrichton I was told to use that variable in #53586 (comment) (and that PR already landed).

EDIT: I don't see how to do it short of a policy change in Cargo, to make "current package believes it's in a workspace when it's not" not an error (unless I misunderstood and it's just a warning?)

@alexcrichton
Copy link
Member

@Mark-Simulacrum how come you suggested that? That should (like some others related to it) be an internal Cargo environment variable that isn't intended for use elsewhere.

@Mark-Simulacrum
Copy link
Member

I did realize it was intended for Cargo-internal use but rustbuild is somewhat closely tied in anyway; I agree it's not too ideal of a state though. I mostly intended it as "this is one way" vs. "this is the right way" but perhaps I was misunderstood / that intent wasn't realized in "need to implement a more public option"

@eddyb
Copy link
Member Author

eddyb commented Nov 24, 2018

I'm experimenting with __CARGO_TEST_ROOT-less solutions in #56194.

@eddyb
Copy link
Member Author

eddyb commented Nov 24, 2018

@alexcrichton Given #56194 (comment), what options do we have?

@alexcrichton
Copy link
Member

@Mark-Simulacrum oh that's what the double underscores were supposed to imply 😢

There just wasn't a way to fix this when rustbuild was originally written, hence the usage of src and not the root of the repository. I would not personally want to solve that problem by reaching into the internals of Cargo.

AFAIK Cargo isn't actually that tied to rustbuild in the sense that rustbuild is only relying on public APIs of Cargo and other official ways to use it. There's been some bugs here and there over time but that's just because rustbuild exercises so much functionality.

@eddyb I don't know of solutions off the top of my head, which is why I never tried to move Cargo.toml out of src.

@eddyb
Copy link
Member Author

eddyb commented Nov 26, 2018

Given that #56194 worked, including unbreaking RLS' tests, I'll close this and revert rust-lang/rls#1138.

@eddyb eddyb closed this Nov 26, 2018
@eddyb eddyb deleted the top-hacks branch November 26, 2018 00:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants