-
Notifications
You must be signed in to change notification settings - Fork 60
Normalize $SRC_DIR #244
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
Normalize $SRC_DIR #244
Conversation
This seems okay but cc @flip1995 and @camsteffen for a double check |
I think this could technically be wrong if running cargo test with CARGO_MANIFEST_DIR. Don't know if that's much of a concern in practice. Would a config value be better? |
Ok, I did try using CARGO_MANIFEST_DIR, but it was returning the path to the I also considered adding a configuration parameter, but that would deviate from the behavior of the official tool. |
I'm not sure it makes sense to re-use cargo's CARGO_MANIFEST_DIR in compiletest.
The official tool has the rustc project structure hard-coded in, so I wouldn't worry about deviating from that. But we can also consider making a change upstream. I think rustc would be amenable to small refactors that decouple compiletest from rustc. |
What is the issue with using |
I think it would be a little unfortunate to set CARGO_MANIFEST_DIR just for the purpose of configuring compiletest. There could be a case where SRC_DIR and CARGO_MANIFEST_DIR are wanted to be different. That said, I guess using CARGO_MANIFEST_DIR is still sensible and better than nothing. |
Ah, I see. Just to be clear, CARGO_MANIFEST_DIR is automatically set by Cargo to be the crate manifest path (or workspace manifest if in a workspace), so I believe this would do the right thing under most circumstances, without the user having to set anything. Im not familiar with what situations call for overriding this variable, but I could potentially see the need for specifying an alternate SRC_DIR when doing so. That could be done with a new config parameter like you suggest. |
Oh good point. I was thinking it is set by the user. That does make it better. |
@flip1995 @Manishearth @camsteffen will it be possible to review this soon? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Using the CARGO_MANIFEST_DIR
sounds reasonable to me.
Fixing the test failures in #245 |
This change normalizes absolute paths for the crate manifest directory to
$SRC_DIR
.This closes #198.
In that issue, it was discussed that it is difficult to determine the correct manifest directory. I'm not sure exactly the semantics of the working directory when
normalize_output
is executed, but it was reliably the manifest directory in all of my testing (running Cargo from many different directories in a large Cargo workspace).