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

Set cargo's output directory to <repo root>/target/ #7625

Closed
wants to merge 1 commit into from
Closed

Set cargo's output directory to <repo root>/target/ #7625

wants to merge 1 commit into from

Conversation

kneasle
Copy link
Contributor

@kneasle kneasle commented Sep 2, 2021

Makes the tests pass immediately for people (like me) who use a unified target/ directory by default.

@rust-highfive
Copy link

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @flip1995 (or someone else) soon.

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Sep 2, 2021
@kneasle kneasle changed the title Fix cargo's output directory to target/ Set cargo's output directory to <repo root>/target/ Sep 2, 2021
@camsteffen
Copy link
Contributor

Thanks for the PR @kneasle. Can you provide more detail as to what problem this solves?

@kneasle
Copy link
Contributor Author

kneasle commented Sep 3, 2021

Thanks for the PR @kneasle. Can you provide more detail as to what problem this solves?

The commit message had some more info - when building Clippy, the UI tests runner expects the binary to be in a hard-coded location in the <clippy repo>/target directory. However, Cargo lets you customise this output directory, which is (not uncommonly) used to put all the build artifacts in one folder like ~/.build/rust/. In this scenario, Clippy's UI tests would still look for the built binary in the <clippy repo>/target directory, making the test runner fail without a terribly obvious error message. It's not a disaster - I would expect anyone who's customised their target directory to know how to undo it - but it's a pain to have to manually enforce this, and it'd be nice if it just worked out of the box.

So this PR overrides Cargo's output directory option in Clippy's own .cargo/config file so that Clippy always puts its build files into <clippy repo>/target (and the change will only apply inside clippy's repo).

@camsteffen
Copy link
Contributor

I wonder if we could actually fix the problem at the source by making the tests work with a different target. But if not, I guess this is fine. We should just make sure it doesn't break the rust repo tests/CI.

@kneasle
Copy link
Contributor Author

kneasle commented Sep 5, 2021

If this comment is anything to go by, it seems you're not the first to think this (and if git blame is anything to go by, that person was @tesuji). But it seems like this requires way more knowledge of compiler internals than I have 😅. I figured this PR would at least get things working for new contributors out of the box (getting everything working was otherwise a very smooth experience).

@flip1995
Copy link
Member

flip1995 commented Sep 6, 2021

I'll have to test if this breaks something in the Rust repo. But since the sync situation between those two repos is currently difficult this will take me about a week.

@kneasle
Copy link
Contributor Author

kneasle commented Sep 6, 2021

I'll have to test if this breaks something in the Rust repo. But since the sync situation between those two repos is currently difficult this will take me about a week.

That's fine, no stress 😃.

@bors
Copy link
Collaborator

bors commented Sep 8, 2021

☔ The latest upstream changes (presumably #7631) made this pull request unmergeable. Please resolve the merge conflicts.

The UI tests only look in `target/` for the built binaries.  However,
Cargo allows users to override the target directory - usually to make a
single unified target directory.  In this case, the unit tests would
fail unless the target directory was manually overridden (as is done in
this commit).
@camsteffen
Copy link
Contributor

camsteffen commented Sep 8, 2021

You should be able to use a custom target directory with #7646.

@kneasle
Copy link
Contributor Author

kneasle commented Sep 8, 2021

You should be able to use a custom target directory with #7646.

Oh excellent, indeed it does - cheers!

@kneasle kneasle closed this Sep 8, 2021
@camsteffen
Copy link
Contributor

Actually I just found out that I had to add in the cargo config like you have here. But it should be safe to override it.

@kneasle kneasle reopened this Sep 8, 2021
@kneasle
Copy link
Contributor Author

kneasle commented Sep 8, 2021

Actually I just found out that I had to add in the cargo config like you have here. But it should be safe to override it.

Same here, so I've re-opened this PR. For #7640, I had to set it locally and avoid checking the change into git.

@camsteffen
Copy link
Contributor

I mean, the change in this PR is also needed in #7646, so it's already included there.

@kneasle
Copy link
Contributor Author

kneasle commented Sep 8, 2021

I mean, the change in this PR is also needed in #7646, so it's already included there.

Oh I seeeeee! Too many similar PR numbers 😅.

@kneasle kneasle closed this Sep 8, 2021
bors added a commit that referenced this pull request Sep 13, 2021
Target directory cleanup

changelog: none

* .cargo/config now has `target-dir` specified so that it is inherited by child projects. The target directory needs to be shared with clippy_dev, but not necessarily at the project root. (cc #7625)
* Uses `std::env::current_exe` (and its parent directories) whenever possible
* `CLIPPY_DRIVER_PATH` and `TARGET_LIBS` are no longer required from rustc bootstrap (but `HOST_LIBS` still is). These can be removed from the rustc side after merging.
* `CLIPPY_DOGFOOD` and the separate target directory are removed. This was originally added to mitigate #7343.

r? `@flip1995`
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.

5 participants