Add some notes about running Cargo's tests on Windows. #4609
Conversation
|
(rust_highfive has picked a reviewer for you, use r? to override) |
| and optional Unix tools from the Windows Command Prompt` to use the shell | ||
| that is bundled with Git, or place the path `C:\Program Files\Git\usr\bin` | ||
| towards the top of your PATH list. | ||
| * Avoid using a `~/.cargo/config` file. Some tests run in a temp directory |
There was a problem hiding this comment.
This seems like a bug I think we should be able to fix, right?
There was a problem hiding this comment.
If you have any ideas how to fix it, I could try to make a PR. I don't know of a way to prevent cargo from recursing up the directory structure. And I don't know of a way to get a Temp directory in Windows that is not under %USERPROFILE%. (See #4601)
There was a problem hiding this comment.
We can just override the env var, right?
There was a problem hiding this comment.
We could override TMP, I just don't know of a safe path to use. It needs be to outside of the cargo repo, and outside of the user's home directory. We could create a random directory in the root of a drive (I think), but that doesn't sound good to me.
There was a problem hiding this comment.
Cargo creates a home/tmp folder for each test, I think that can be used?
There was a problem hiding this comment.
It is using tempdir, that's the problem. tempdir uses the temp directory on Windows which is ~/AppData/Local/Temp/. Unfortunately since that is under the user's home directory, cargo will read ~/.cargo/config, and if you have [cargo-new] entries, the test will fail since those take priority. The only fixes I know of are:
- Create a temp directory somewhere safe. But where?
- Somehow prevent cargo from recursing up the directory structure.
There was a problem hiding this comment.
Sounds like we should add a way for Cargo to avoid traversing upwards in tests?
There was a problem hiding this comment.
Hm, I just discovered #3869 which added __CARGO_TEST_ROOT as a hack to prevent this very issue. I will investigate if this can be leveraged to make this work, it should be simple.
There was a problem hiding this comment.
OK, opened #4630 with a proposed fix.
| verify with your environment. Besides having a normal Rust development | ||
| environment (with CMake installed), check the following: | ||
|
|
||
| * `sh` from MSYS should be in your PATH (ahead of any other `sh` installations |
There was a problem hiding this comment.
Something about this isn't correct I think because AppVeyor doesn't have this in PATH, yet tests pass on AppVeyor?
There was a problem hiding this comment.
Yea, I actually ran a test in AppVeyor that printed the environment because I was confused about that too. They include C:\Program Files\Git\usr\bin in PATH. Here is the list.
There was a problem hiding this comment.
Hm, can you gist again the error that comes about from not having sh in PATH?
There was a problem hiding this comment.
The error is at the top of #4591.
There was a problem hiding this comment.
One workaround for this would be to change git2 to only use sh if the helper contains shell-like characters. I'd be happy to try a PR for that, it's just a bigger change.
There was a problem hiding this comment.
The test can test something else then which doesn't fail when sh isn't available?
There was a problem hiding this comment.
I don't think there is anything else the test can do. As it is, credential helpers need sh or they just don't work.
There was a problem hiding this comment.
So... prefix the test with?
if Command::new("sh").arg("--version").output().is_err() {
return
}There was a problem hiding this comment.
That wouldn't be my preference (I don't like tests that are silently ignored). If it were to do something like that, I was thinking it would probably need to run sh -c path/to/script. I don't think --version wouldn't be completely safe (for example, I don't think BSD sh handles that). Also, the path would need to use forward slashes on windows to not mysteriously fail with Cygwin. This is a pretty obscure issue, I was just trying to make it a little less frustrating for future Windows developers to get going. I'm a little concerned adding all this may make it more brittle.
There was a problem hiding this comment.
Note that we're not looking for a successful execution of sh, just if the program exists.
|
How much of this is still relevant? |
|
I believe one was fixed through a PR and the other was fixed through an update of libgit2, so closing! |
|
Sorry, I forgot to close this. However, I don't think libgit2 was ever updated (see alexcrichton/git2-rs#274). |
Fixes #4591 and #4601.