gitoxide auth tests sporadically failing #11821

ehuss · 2023-03-09T20:18:47Z

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)

The text was updated successfully, but these errors were encountered:

ehuss · 2023-03-10T15:02:50Z

@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 · 2023-03-10T16:40:30Z

~~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.

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

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

This reverts commit c890c64.

Byron · 2023-03-10T19:04:10Z

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

Disable flaky auth tests when gitoxide runs them #11830 - ignore flaky tests for now without any side-effects.
[BLOCKED] upgrade gix to v0.41 to fix flaky auth tests. #11831 - can only be merged once tempfile 3.4 doesn't break builds anymore.

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

@weihanglo

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.

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.

@weihanglo

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

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 · 2023-12-05T04:40:49Z

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

ehuss · 2023-12-05T13:42:46Z

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

weihanglo · 2023-12-05T14:38:41Z

Hmm… we didn't?

This reverts commit c890c64. gix was updated to 0.41.0 (and later) a while back. Let's see how it goes.

weihanglo · 2023-12-05T17:02:17Z

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

Feel free to take over it.

Byron · 2023-12-05T18:24:12Z

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 · 2023-12-05T19:04:47Z

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

Byron · 2023-12-05T20:48:02Z

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 :).

This release disables stderr for external programs by default, while allowing to override these settings using configuration or environment variables.

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.

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 · 2023-12-07T01:46:41Z

Reopen. See #13129

Byron · 2023-12-07T06:29:06Z

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.

) 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)

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)

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

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)

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 [email protected] (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 · 2023-12-20T04:04:34Z

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

ehuss added C-bug Category: bug A-testing-cargo-itself Area: cargo's tests Z-gitoxide Nightly: gitoxide integration labels Mar 9, 2023

ehuss mentioned this issue Mar 9, 2023

Bump crates-io to 0.36.0 #11820

Merged

Byron mentioned this issue Mar 10, 2023

fix credential helper communication Byron/gitoxide#775

Merged

Byron mentioned this issue Mar 10, 2023

Tracking Issue for -Zgitoxide #11813

Open

15 tasks

Byron mentioned this issue Mar 10, 2023

Disable flaky auth tests when gitoxide runs them #11830

Merged

Byron added a commit to Byron/cargo that referenced this issue Mar 10, 2023

Revert "Disable flaky tests for now (rust-lang#11821)"

51ff275

This reverts commit c890c64.

Byron mentioned this issue Mar 10, 2023

[BLOCKED] upgrade gix to v0.41 to fix flaky auth tests. #11831

Closed

weihanglo closed this as completed Dec 5, 2023

weihanglo reopened this Dec 5, 2023

weihanglo added a commit to weihanglo/cargo that referenced this issue Dec 5, 2023

Revert "Disable flaky tests for now (rust-lang#11821)"

cb81a3f

This reverts commit c890c64. gix was updated to 0.41.0 (and later) a while back. Let's see how it goes.

weihanglo mentioned this issue Dec 5, 2023

Revert "Disable flaky tests for now (#11821)" #13116

Closed

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 mentioned this issue Dec 5, 2023

re-enable previously disabled tests with Windows-specific fix #13117

Merged

3 tasks

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

upgrade gitoxide to v0.56 (rust-lang#11821)

930d653

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

upgrade gitoxide to v0.56 (rust-lang#11821)

50c0005

This release disables stderr for external programs by default, while allowing to override these settings using configuration or environment variables.

weihanglo linked a pull request Dec 6, 2023 that will close this issue

re-enable previously disabled tests with Windows-specific fix #13117

Merged

3 tasks

bors closed this as completed in #13117 Dec 6, 2023

weihanglo reopened this Dec 7, 2023

Byron mentioned this issue Dec 7, 2023

make stderr-suppression actually work Byron/gitoxide#1159

Merged

Byron mentioned this issue Dec 7, 2023

re-enable flaky tests thanks to update to gix-config. (#11821) #13130

Merged

weihanglo mentioned this issue Dec 9, 2023

Update cargo rust-lang/rust#118765

Merged

weihanglo closed this as completed Dec 20, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

gitoxide auth tests sporadically failing #11821

gitoxide auth tests sporadically failing #11821

ehuss commented Mar 9, 2023

ehuss commented Mar 10, 2023

Byron commented Mar 10, 2023

Byron commented Mar 10, 2023

weihanglo commented Dec 5, 2023

ehuss commented Dec 5, 2023

weihanglo commented Dec 5, 2023

weihanglo commented Dec 5, 2023

Byron commented Dec 5, 2023

weihanglo commented Dec 5, 2023

Byron commented Dec 5, 2023

weihanglo commented Dec 7, 2023

Byron commented Dec 7, 2023

weihanglo commented Dec 20, 2023

gitoxide auth tests sporadically failing #11821

gitoxide auth tests sporadically failing #11821

Comments

ehuss commented Mar 9, 2023

ehuss commented Mar 10, 2023

Byron commented Mar 10, 2023

Byron commented Mar 10, 2023

weihanglo commented Dec 5, 2023

ehuss commented Dec 5, 2023

weihanglo commented Dec 5, 2023

weihanglo commented Dec 5, 2023

Byron commented Dec 5, 2023

weihanglo commented Dec 5, 2023

Byron commented Dec 5, 2023

weihanglo commented Dec 7, 2023

Byron commented Dec 7, 2023

weihanglo commented Dec 20, 2023