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

gitoxide auth tests sporadically failing #11821

Closed
ehuss opened this issue Mar 9, 2023 · 13 comments · Fixed by #13117
Closed

gitoxide auth tests sporadically failing #11821

ehuss opened this issue Mar 9, 2023 · 13 comments · Fixed by #13117
Labels
A-testing-cargo-itself Area: cargo's tests C-bug Category: bug S-accepted Status: Issue or feature is accepted, and has a team member available to help mentor or review Z-gitoxide Nightly: gitoxide integration

Comments

@ehuss
Copy link
Contributor

ehuss commented Mar 9, 2023

Some of the gitoxide tests seem to be randomly failing in CI:

The errors look like:

---- git_auth::http_auth_offered stdout ----
running `/Users/runner/work/cargo/cargo/target/debug/cargo build -v`
running `/Users/runner/work/cargo/cargo/target/debug/cargo check`
thread '<unnamed>' panicked at 'assertion failed: `(left == right)`
  left: `{}`,
 right: `{"GET /foo/bar/info/refs?service=git-upload-pack HTTP/1.1", "Authorization: Basic Zm9vOmJhcg==", "Accept: */*"}`', tests/testsuite/git_auth.rs:63:9
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
thread 'git_auth::http_auth_offered' panicked at '
test failed running `/Users/runner/work/cargo/cargo/target/debug/cargo check`
error: expected to find:
    Updating git repository `http://127.0.0.1:50629/foo/bar`
error: failed to get `bar` as a dependency of package `foo v0.0.1 [..]`

Caused by:
  failed to load source for dependency `bar`

Caused by:
  Unable to update http://127.0.0.1:50629/foo/bar

Caused by:
  failed to clone into: [..]

Caused by:
  failed to authenticate when downloading repository

  * attempted to find username/password via `credential.helper`, but [..]

  if the git CLI succeeds then `net.git-fetch-with-cli` may help here
  https://[..]

Caused by:


did not find in output:
    Updating git repository `http://127.0.0.1:50629/foo/bar`
error: failed to get `bar` as a dependency of package `foo v0.0.1 (/Users/runner/work/cargo/cargo/target/tmp/cit/testsuite/git_auth/http_auth_offered/foo)`

Caused by:
  failed to load source for dependency `bar`

Caused by:
  Unable to update http://127.0.0.1:50629/foo/bar

Caused by:
  failed to clone into: /Users/runner/work/cargo/cargo/target/tmp/cit/testsuite/git_auth/http_auth_offered/home/.cargo/git/db/bar-71575e16952ccc70

Caused by:
  failed to authenticate when downloading repository

  * attempted to find username/password via git's `credential.helper` support, but failed

  if the git CLI succeeds then `net.git-fetch-with-cli` may help here
  https://doc.rust-lang.org/cargo/reference/config.html#netgit-fetch-with-cli

Caused by:
  Failed to obtain credentials

Caused by:
  An IO error occurred while communicating to the credentials helper

Caused by:
  Broken pipe (os error 32)
', tests/testsuite/git_auth.rs:163:10

---- git_auth::instead_of_url_printed stdout ----
running `/Users/runner/work/cargo/cargo/target/debug/cargo build -v`
running `/Users/runner/work/cargo/cargo/target/debug/cargo check`
thread '<unnamed>' panicked at 'assertion failed: `(left == right)`
  left: `{}`,
 right: `{"Authorization: Basic Zm9vOmJhcg==", "Accept: */*", "GET /foo/bar/info/refs?service=git-upload-pack HTTP/1.1"}`', tests/testsuite/git_auth.rs:63:9
thread 'git_auth::instead_of_url_printed' panicked at '
test failed running `/Users/runner/work/cargo/cargo/target/debug/cargo check`
error: stderr did not match:
1   1         Updating git repository `https://foo.bar/foo/bar`
2   2     error: failed to get `bar` as a dependency of package `foo [..]`
3   3     
4   4     Caused by:
5   5       failed to load source for dependency `bar`
6   6     
7   7     Caused by:
8   8       Unable to update https://foo.bar/foo/bar
9   9     
10  10    Caused by:
11  11      failed to clone into: [..]
12  12    
13  13    Caused by:
14  14      failed to authenticate when downloading repository: http://127.0.0.1:50632/foo/bar
15  15    
16       -  * attempted to find username/password via `credential.helper`, but maybe the found credentials were incorrect
    16   +  * attempted to find username/password via git's `credential.helper` support, but failed
17  17    
18  18      if the git CLI succeeds then `net.git-fetch-with-cli` may help here
19  19      https://[..]
20  20    
21  21    Caused by:
22  22      [..]
23  23    
24  24    Caused by:
25  25      [..]
    26   +
    27   +Caused by:
    28   +  Broken pipe (os error 32)


---- git_auth::instead_of_url_printed stdout ----
running `/home/runner/work/cargo/cargo/target/debug/cargo build -v`
running `/home/runner/work/cargo/cargo/target/debug/cargo check`
thread '<unnamed>' panicked at 'assertion failed: `(left == right)`
  left: `{}`,
 right: `{"GET /foo/bar/info/refs?service=git-upload-pack HTTP/1.1", "Authorization: Basic Zm9vOmJhcg==", "Accept: */*"}`', tests/testsuite/git_auth.rs:63:9
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
thread 'git_auth::instead_of_url_printed' panicked at '
test failed running `/home/runner/work/cargo/cargo/target/debug/cargo check`
error: stderr did not match:
1   1         Updating git repository `https://foo.bar/foo/bar`
2   2     error: failed to get `bar` as a dependency of package `foo [..]`
3   3     
4   4     Caused by:
5   5       failed to load source for dependency `bar`
6   6     
7   7     Caused by:
8   8       Unable to update https://foo.bar/foo/bar
9   9     
10  10    Caused by:
11  11      failed to clone into: [..]
12  12    
13  13    Caused by:
14  14      failed to authenticate when downloading repository: http://127.0.0.1:43505/foo/bar
15  15    
16       -  * attempted to find username/password via `credential.helper`, but maybe the found credentials were incorrect
    16   +  * attempted to find username/password via git's `credential.helper` support, but failed
17  17    
18  18      if the git CLI succeeds then `net.git-fetch-with-cli` may help here
19  19      https://[..]
20  20    
21  21    Caused by:
22  22      [..]
23  23    
24  24    Caused by:
25  25      [..]
    26   +
    27   +Caused by:
    28   +  Broken pipe (os error 32)
@ehuss ehuss added C-bug Category: bug A-testing-cargo-itself Area: cargo's tests Z-gitoxide Nightly: gitoxide integration labels Mar 9, 2023
@ehuss
Copy link
Contributor Author

ehuss commented Mar 10, 2023

@Byron This seems to be happening fairly frequently. Would it be possible to ignore these tests or disable the gitoxide tests until this is fixed?

@Byron
Copy link
Member

Byron commented Mar 10, 2023

I think it should be possible to disable these tests only for gitoxide test runs and put fixing this on the list of things to do for an immediate remedy. I will submit a PR for that today.

For context, what happens here is that cargo configures a credential helper that it builds itself which merely echos credentials in a format expected by git. It should be impossible to not be able to read that in time and I don't know why the process can launch, but then somehow we fail to read its stdout.

What's even stranger is that git2 doesn't seem to have this problem, and all gitoxide does is to spawn a process and communicate through its stdin and out pipes. It's unclear to me how this program can fail at all. Maybe it didn't fail even, maybe it exited already and closed its stdin which we try to write, and gitoxide considers write failures whereas git2 does ignore write failures.

Let me also ignore write failures (just like git does) and submit a PR with a gix update if necessary.

Byron added a commit to GitoxideLabs/gitoxide that referenced this issue Mar 10, 2023
They might ignore all input and output hardcoded credentials for good reason,
so we don't validate writes to the helper similarly to how git does it.

Note that we also don't care if credentials are stored or erased for similar reasons.
I would happily make the case that we should be more adamant about error handling in that
case, but typically we check error codes and that's the only indicator for success or failure.

Related to rust-lang/cargo#11821
Byron added a commit to Byron/cargo that referenced this issue Mar 10, 2023
The proper fix is in https://github.com/Byron/gitoxide/releases/tag/gix-v0.41.0
which unfortunately can't be used as it also comes with the latest `tempfile` v3.4
which causes other issues when compiling on some platforms.

Thus we first disable the flaky tests, and re-enable them with the `gix` upgrade
which should be possible once `tempfile` doesn't hinder `cargo` on some platforms
anymore.

Related to rust-lang#11821
Byron added a commit to Byron/cargo that referenced this issue Mar 10, 2023
@Byron
Copy link
Member

Byron commented Mar 10, 2023

This is more complicated than it has any right to be, but here we are.

Are there better ways to do this? This is the only way I see unfortunately.

Byron added a commit to Byron/cargo that referenced this issue Mar 11, 2023
upgrade `gix` to v0.41 to fix flaky auth tests and revert commit c890c64.

The latest release (https://github.com/Byron/gitoxide/releases/tag/gix-v0.41.0)
makes writing to the credential helper non-fallible which is useful if the helper
program doesn't read from stdin and is in line with of `git` implements it as well.

This fix also undoes a pin `tempfile` to version 3.3 which at this time
causes build issues for some platforms if `tempfile` version 3.4 or newer would be used.

Thus this PR cannot be merged until `tempfile` will not cause build failures.
This also means that `cargo` can't upgrade to any newer version of `gitoxide` until `tempfile` isn't an
issue anymore, also blocking the shallow clone feature for example.

@weihanglo, maybe you can replace this line with references to issues to wait for.
bors added a commit that referenced this issue Mar 11, 2023
Disable flaky auth tests when `gitoxide` runs them

The proper fix is in https://github.com/Byron/gitoxide/releases/tag/gix-v0.41.0
which unfortunately can't be used as it also comes with the latest `tempfile` v3.4
which causes other issues when compiling on some platforms.

Thus we first disable the flaky tests, and re-enable them with the `gix` upgrade
which should be possible once `tempfile` doesn't hinder `cargo` on some platforms
anymore.

Related to #11821

This PR is supposed to be followed up by #11831 which re-enables the flaky tests and fixes them properly by upgrading `gix` which contains the fix.
Byron added a commit to Byron/cargo that referenced this issue Apr 1, 2023
upgrade `gix` to v0.41 to fix flaky auth tests and revert commit c890c64.

The latest release (https://github.com/Byron/gitoxide/releases/tag/gix-v0.41.0)
makes writing to the credential helper non-fallible which is useful if the helper
program doesn't read from stdin and is in line with of `git` implements it as well.

This fix also undoes a pin `tempfile` to version 3.3 which at this time
causes build issues for some platforms if `tempfile` version 3.4 or newer would be used.

Thus this PR cannot be merged until `tempfile` will not cause build failures.
This also means that `cargo` can't upgrade to any newer version of `gitoxide` until `tempfile` isn't an
issue anymore, also blocking the shallow clone feature for example.

@weihanglo, maybe you can replace this line with references to issues to wait for.
Byron added a commit to Byron/cargo that referenced this issue Apr 4, 2023
upgrade `gix` to v0.41 to fix flaky auth tests and revert commit c890c64.

The latest release (https://github.com/Byron/gitoxide/releases/tag/gix-v0.41.0)
makes writing to the credential helper non-fallible which is useful if the helper
program doesn't read from stdin and is in line with of `git` implements it as well.

This fix also undoes a pin `tempfile` to version 3.3 which at this time
causes build issues for some platforms if `tempfile` version 3.4 or newer would be used.

Thus this PR cannot be merged until `tempfile` will not cause build failures.
This also means that `cargo` can't upgrade to any newer version of `gitoxide` until `tempfile` isn't an
issue anymore, also blocking the shallow clone feature for example.

@weihanglo, maybe you can replace this line with references to issues to wait for.
@weihanglo
Copy link
Member

This was fixed by #11840 apparently, as it hasn't happened for a long while. Closing.

@ehuss
Copy link
Contributor Author

ehuss commented Dec 5, 2023

@weihanglo Doesn't #11830 need to be reverted?

@weihanglo
Copy link
Member

Hmm… we didn't?

@weihanglo weihanglo reopened this Dec 5, 2023
weihanglo added a commit to weihanglo/cargo that referenced this issue Dec 5, 2023
This reverts commit c890c64.

gix was updated to 0.41.0 (and later) a while back.
Let's see how it goes.
@weihanglo
Copy link
Member

The old failure should be gone but there is a new one on Windows. See #13116 (comment).

Feel free to take over it.

@weihanglo weihanglo added the S-accepted Status: Issue or feature is accepted, and has a team member available to help mentor or review label Dec 5, 2023
@Byron
Copy link
Member

Byron commented Dec 5, 2023

I was actually convinced that something was indeed reverted in the shallow-clones PR, but judging by this PR on Windows strange things are still happening.

To me it seems that it would already be an improvement to enable the two disabled tests on all platforms but windows for now, and I can work on a way to let library users control what happens to stderr of launched programs. Most of the time, it's set to inherit, which definitely isn't helpful for the tests. Maybe there is also a way to change the timeout which seems to cause these extra messages.

I will probably submit a PR for this tomorrow as the issue on windows seems fixable, so we can finally close this issue here.

@weihanglo
Copy link
Member

Sounds good to me for either disabling or fixing the test on Windows. Thank you for taking a look at it!

@Byron
Copy link
Member

Byron commented Dec 5, 2023

Great! I gave it a quick shot but wasn't lucky enough for my 'fix' to fully work. This means gitoxide needs an adjustment to accommodate, but I hope to make that happen tomorrow then. With it, that issue should be permanently fixed though, which probably is the most desirable kind of fix anyway :).

Byron added a commit to Byron/cargo that referenced this issue Dec 6, 2023
This release disables stderr for external programs by default, while
allowing to override these settings using configuration or environment
variables.
Byron added a commit to Byron/cargo that referenced this issue Dec 6, 2023
This release disables stderr for external programs by default, while
allowing to override these settings using configuration or environment
variables.
Byron added a commit to Byron/cargo that referenced this issue Dec 6, 2023
On Windows, `gix` will call the `git-credential-manager, but with
`stderr` set to `inherit` it makes any errors visible to the user,
just like `git` does.

```
1   1         Updating git repository `https://foo.bar/foo/bar`
    2    +warning: auto-detection of host provider took too long (>2000ms)
    3    +warning: see https://aka.ms/gcm/autodetect for more information.
    4    +fatal: A task was canceled.
    5    +warning: auto-detection of host provider took too long (>2000ms)
    6    +warning: see https://aka.ms/gcm/autodetect for more information.`
````

This, however, isn't what's desirable in tests sometimes, nor may
it be desirable in Cargo.

In the latest version of `gix`, it's possible to control `stderr`
which is now set on a per-test basis.

Also note that for `cargo` as a whole the default didn't change,
and stderr of spawned helper programs will remain visible in the
enclosing terminal.
@weihanglo weihanglo linked a pull request Dec 6, 2023 that will close this issue
3 tasks
bors added a commit that referenced this issue Dec 6, 2023
re-enable previously disabled tests with Windows-specific fix

Related to #11821 for which this is a fix.
However, it's probably not yet the optimal solution, depending on how `stderr` of subprocesses should be handled.

### Tasks

* [x] try to fix the issue with an env var.
    - Failure, as one warning remains that seems to originate from a C# HTTP client
* [x] figure out if `stderr` should be on or off by default - on by default like before, but now one can control it.
* [x] create a new `gix` release and use it here

### Review Notes

* Personally, I think `cargo` should keep `stderr` to be inherited so users can see potentially relevant warnings or errors provided by credential helpers. Thus this is still the default, but the tests that need it explicitly disabled `stderr` of credential helpers.

----

On Windows, `gix` will call the `git-credential-manager, but with
`stderr` set to `inherit` which makes any errors visible to the user,
just like `git` does.

```
1   1         Updating git repository `https://foo.bar/foo/bar`
    2    +warning: auto-detection of host provider took too long (>2000ms)
    3    +warning: see https://aka.ms/gcm/autodetect for more information.
    4    +fatal: A task was canceled.
    5    +warning: auto-detection of host provider took too long (>2000ms)
    6    +warning: see https://aka.ms/gcm/autodetect for more information.`
````

This, however, isn't what's desirable in tests sometimes, nor may
it be desirable in Cargo.

For now, it seems easiest to disable this particular feature that
issues the warning messages, even though a future `gix` update
should allow to control what to do with `stderr`.
@weihanglo
Copy link
Member

Reopen. See #13129

@Byron
Copy link
Member

Byron commented Dec 7, 2023

The quick-shot must have missed :/. Admittedly I was flying blind and didn't validate that it works locally. I will try again and see what the problem is (this time with validation), as right now is a good time to try to tackle it as I can still make a point release of whichever crates are affected in the fix.

Byron added a commit to Byron/cargo that referenced this issue Dec 7, 2023
)

With `gix-config` now being fixed, it will properly respect `GIT_CONFIG_NOSYSTEM`
both for system-wide configuration as well as for the git installation configuration.

That way, credential helpers provided by the git installation won't be called anymore,
which prevents them from 'somehow' emitting information to stderr.

The latter was previously disabled for credential helpers, and despite
everything^1 looking like it should work, it simply didn't.

[1]: rust-lang#13117 (comment)
bors added a commit that referenced this issue Dec 7, 2023
re-enable flaky tests thanks to update to `gix-config`. (#11821)

Related to #11821.

With `gix-config` now being fixed, it will properly respect `GIT_CONFIG_NOSYSTEM` both for system-wide configuration as well as for the git installation configuration.

That way, credential helpers provided by the git installation won't be called anymore, which prevents them from 'somehow' emitting information to stderr.

The latter was previously disabled for credential helpers, and despite [everything looking like it should work][1], it simply didn't.

[1]: #13117 (comment)
bors added a commit to rust-lang-ci/rust that referenced this issue Dec 9, 2023
Update cargo

12 commits in 9787229614b27854cf73d57ffae430d7c1e6caa4..6feabf94773286928b7d73d0d313c0c5562b66af
2023-12-06 02:29:23 +0000 to 2023-12-08 22:38:37 +0000
- fix: explicitly remap current dir by using `.` (rust-lang/cargo#13114)
- Don't rely on mtime to test changes (rust-lang/cargo#13143)
- refactor: Pull PackageIdSpec into schema (rust-lang/cargo#13128)
- fix: Print rustc messages colored on wincon (rust-lang/cargo#13140)
- Add a windows manifest file (rust-lang/cargo#13131)
- Avoid writing CACHEDIR.TAG if it already exists (rust-lang/cargo#13132)
- re-enable flaky tests thanks to update to `gix-config`. (rust-lang/cargo#11821) (rust-lang/cargo#13130)
- fix bash completion in directory with spaces (rust-lang/cargo#13126)
- test: re-ignore git auth tests for gitoxide (rust-lang/cargo#13129)
- fix(toml): Disallow inheriting of dependency public status (rust-lang/cargo#13125)
- re-enable previously disabled tests with Windows-specific fix (rust-lang/cargo#13117)
- refactor: Clarify PackageId constructor names (rust-lang/cargo#13123)

r? ghost
bors added a commit to rust-lang-ci/rust that referenced this issue Dec 9, 2023
Update cargo

13 commits in 9787229614b27854cf73d57ffae430d7c1e6caa4..66ad359b408ccdf867cceb30cc2dff1ed4afd82d
2023-12-06 02:29:23 +0000 to 2023-12-09 12:30:01 +0000
- chore: downgrade to openssl v1.1.1 (rust-lang/cargo#13144)
- fix: explicitly remap current dir by using `.` (rust-lang/cargo#13114)
- Don't rely on mtime to test changes (rust-lang/cargo#13143)
- refactor: Pull PackageIdSpec into schema (rust-lang/cargo#13128)
- fix: Print rustc messages colored on wincon (rust-lang/cargo#13140)
- Add a windows manifest file (rust-lang/cargo#13131)
- Avoid writing CACHEDIR.TAG if it already exists (rust-lang/cargo#13132)
- re-enable flaky tests thanks to update to `gix-config`. (rust-lang/cargo#11821) (rust-lang/cargo#13130)
- fix bash completion in directory with spaces (rust-lang/cargo#13126)
- test: re-ignore git auth tests for gitoxide (rust-lang/cargo#13129)
- fix(toml): Disallow inheriting of dependency public status (rust-lang/cargo#13125)
- re-enable previously disabled tests with Windows-specific fix (rust-lang/cargo#13117)
- refactor: Clarify PackageId constructor names (rust-lang/cargo#13123)
bors added a commit to rust-lang-ci/rust that referenced this issue Dec 12, 2023
Update cargo

20 commits in 9787229614b27854cf73d57ffae430d7c1e6caa4..1aa9df1a5be205cce621f0bc0ea6062a5e22a98c
2023-12-06 02:29:23 +0000 to 2023-12-12 14:52:31 +0000
- crates-io: Add support for other 2xx HTTP status codes (rust-lang/cargo#13158)
- Remove the deleted feature test_2018_feature from the test (rust-lang/cargo#13156)
- refactor(schema): Remove reliance on cargo types (rust-lang/cargo#13154)
- fix(toml)!: Disallow `[lints]` in virtual workspaces (rust-lang/cargo#13155)
- Limit exported-private-dependencies lints to libraries (rust-lang/cargo#13135)
- chore: update to gix-index@0.27.1 (rust-lang/cargo#13148)
- Update curl-sys to bring in curl 8.5.0 (rust-lang/cargo#13147)
- chore: downgrade to openssl v1.1.1 (rust-lang/cargo#13144)
- fix: explicitly remap current dir by using `.` (rust-lang/cargo#13114)
- Don't rely on mtime to test changes (rust-lang/cargo#13143)
- refactor: Pull PackageIdSpec into schema (rust-lang/cargo#13128)
- fix: Print rustc messages colored on wincon (rust-lang/cargo#13140)
- Add a windows manifest file (rust-lang/cargo#13131)
- Avoid writing CACHEDIR.TAG if it already exists (rust-lang/cargo#13132)
- re-enable flaky tests thanks to update to `gix-config`. (rust-lang/cargo#11821) (rust-lang/cargo#13130)
- fix bash completion in directory with spaces (rust-lang/cargo#13126)
- test: re-ignore git auth tests for gitoxide (rust-lang/cargo#13129)
- fix(toml): Disallow inheriting of dependency public status (rust-lang/cargo#13125)
- re-enable previously disabled tests with Windows-specific fix (rust-lang/cargo#13117)
- refactor: Clarify PackageId constructor names (rust-lang/cargo#13123)
@weihanglo
Copy link
Member

I feel confident to close this via #13130 this time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-testing-cargo-itself Area: cargo's tests C-bug Category: bug S-accepted Status: Issue or feature is accepted, and has a team member available to help mentor or review Z-gitoxide Nightly: gitoxide integration
Projects
None yet
3 participants