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

Test http_auth_offered fails on Windows #4591

Closed
ehuss opened this issue Oct 7, 2017 · 9 comments
Closed

Test http_auth_offered fails on Windows #4591

ehuss opened this issue Oct 7, 2017 · 9 comments
Labels
A-cargo-api Area: cargo-the-library API and internal code issues A-testing-cargo-itself Area: cargo's tests C-bug Category: bug O-windows OS: Windows

Comments

@ehuss
Copy link
Contributor

ehuss commented Oct 7, 2017

The test http_auth_offered is failing for me on Windows 10 with the following output:

---- http_auth_offered stdout ----
        running `H:\Proj\rust\cargo\target\debug\cargo.exe build -v`
running `H:\Proj\rust\cargo\target\debug\cargo.exe build`
thread 'http_auth_offered' panicked at '
Expected: execs
    but: differences:
 11 - |attempted to find username/password via `credential.helper`, but [..]|
    + |attempted to find username/password via git's `credential.helper` support, but failed|

The reason this happens is because this code in the git2 package assumes you have a Bourne shell in your path. Even if you have a Bourne shell (such as Git Bash or Cygwin), they often do not support Windows-style paths (in this case, the gitconfig is configured with a path like helper = H:\\Proj\\rust\\cargo\\target\\cit\\t1\\script\\target\\debug\\script.exe).

Is there something about my environment that should be different? Or does this problem happen for other people? I'm not sure what an appropriate fix would be. The official git client seems to avoid using a shell if there are no shell characters in the command (see prepare_shell_cmd), but even that assumes a backslash implies escaping, so the paths may need to be created in a unix-style.

@alexcrichton
Copy link
Member

Ah we probably just need to relax the assertion here in the test itself. Should be ok to have a different environment!

@ehuss
Copy link
Contributor Author

ehuss commented Oct 10, 2017

I did some more investigation. It seems that it works correctly if you have sh from MSYS/MinGW in your PATH (it handles Windows paths better than Cygwin). I'm not sure what the best solution here is. I'd be happy if it was just documented somewhere that you need Git Bash (or MSYS) in your PATH to run the tests (maybe CONTRIBUTING.md?). What do you think?

See also similar (but unrelated) #4601 and #4602.

@alexcrichton
Copy link
Member

Hm in theory you shouldn't need git in PATH, although maybe the error is just different if you don't have git in PATH? If so we should just update the test.

@ehuss
Copy link
Contributor Author

ehuss commented Oct 10, 2017

I mean you need sh from MSYS in your path, which just happens to be bundled with Git (C:\Program Files\Git\usr\bin specifically). There are a few other tests which do require git to be in your PATH.

The error from cargo if it fails to run the credential helper is:
attempted to find username/password via git's `credential.helper` support, but failed

The "success" case (which this test is asserting) is when the credentials are successfully retrieved, but the little pseudo-server listening on localhost returns a 401 Unauthorized, which displays the message:
attempted to find username/password via `credential.helper`, but maybe the found credentials were incorrect

You can't just test for the other error message. Not only would that invalidate the test, the test would hang forever because the little pseudo server never receives a connection.

I think it's fine for the tests to assume your environment has certain things installed, I just think they need to be documented. I'd be happy to try to document the requirements for running the tests.

@alexcrichton
Copy link
Member

I think in this case we just need to change the assertion to attempted to find username/password [..], right?

@ehuss
Copy link
Contributor Author

ehuss commented Oct 10, 2017

No, if you change it to that, it will hang forever, because the server thread never receives a connection.

@alexcrichton
Copy link
Member

Ah ok, well I'm fine with whatever fix works!

@carols10cents carols10cents added A-cargo-api Area: cargo-the-library API and internal code issues A-testing-cargo-itself Area: cargo's tests C-bug Category: bug O-windows OS: Windows labels Oct 16, 2017
@ehuss
Copy link
Contributor Author

ehuss commented Jan 19, 2018

See also upstream issue alexcrichton/git2-rs#274.

@alexcrichton
Copy link
Member

I think this has since been fixed, so closing!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-cargo-api Area: cargo-the-library API and internal code issues A-testing-cargo-itself Area: cargo's tests C-bug Category: bug O-windows OS: Windows
Projects
None yet
Development

No branches or pull requests

3 participants