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

Revert part of #5393 #5465

Merged
merged 1 commit into from
May 3, 2018
Merged

Revert part of #5393 #5465

merged 1 commit into from
May 3, 2018

Conversation

nrc
Copy link
Member

@nrc nrc commented May 3, 2018

Commit 0b530c3 (which this commit mostly reverts) did some refactoring around the target_dir function. However, it introduced a bug because it meant that where CARGO_TARGET_DIR was specified but --target-dir was not, the value from CARGO_TARGET_DIR was ignored.

r? @matklad

This bug is causing RLS tests to fail in the Rust repo (see rust-lang/rust#50379 (comment)) because on Linux, the src directory is write-protected. This bug causes Rust build's CARGO_TARGET_DIR to be ignored by the RLS tests so that they write files into the src directory (CWD) rather than the target directory.

Commit 0b530c3 (which this commit mostly reverts) did some refactoring around the `target_dir` function. However, it introduced a bug because it meant that where `CARGO_TARGET_DIR` was specified but `--target-dir` was not, the value from `CARGO_TARGET_DIR` was ignored.
@nrc nrc changed the title Revert part to #5393 Revert part of #5393 May 3, 2018
@matklad
Copy link
Member

matklad commented May 3, 2018

@bors r+

@bors
Copy link
Contributor

bors commented May 3, 2018

📌 Commit 695bc5e has been approved by matklad

@bors
Copy link
Contributor

bors commented May 3, 2018

⌛ Testing commit 695bc5e with merge 168f15274db3d9e906e432c4a211fda5b03746d7...

@matklad
Copy link
Member

matklad commented May 3, 2018

What actually happens here is that, in Cargo, we always call Config::configure early on, so the target_dir is setup properly, and the 0b530c3 is actually correct, if we look only at Cargo.

However, RLS does not always call configure, so the target_dir is not set for it.

I'll try to find a way to guarantee that Cargo and RLS behave similarly here....

@bors
Copy link
Contributor

bors commented May 3, 2018

💔 Test failed - status-travis

bors added a commit that referenced this pull request May 3, 2018
Fix target_dir handling for RLS

Supersedes  and closes #5465
@bors bors merged commit 695bc5e into rust-lang:master May 3, 2018
kennytm added a commit to kennytm/rust that referenced this pull request May 3, 2018
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.

3 participants