-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
re-enable previously disabled tests with Windows-specific fix #13117
Conversation
r? @weihanglo (rustbot has picked a reviewer for you, use r? to override) |
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.
@weihanglo This PR is ready for review. Pleas also take a look at the Review Notes, as this issue can definitely be solved in various ways, even though I kept everything as is, while fixing the two tests specifically. Thanks for taking a look. |
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.
It's unbelievably fast! Thanks for the new release :)
@bors r+ |
☀️ Test successful - checks-actions |
Looks like we didn't fix the issue 😭. I'll disable it again. We can postpone the fix until we're going to stabilize maybe. |
test: re-ignore git auth tests for gitoxide Happened again in <#13126 (comment)> even after #13117. Let's ignore them again and hope someday we can fix it :)
Thanks for letting me know. Experiment: use the new environment variables to see if they workI did the validation that I probably should have had before, and here is the outcome. Stderr of external filter programs can now be swallowed as expected: # .git/config
[filter "noop"]
clean = "echo clean 1>&2; cat"
smudge = "echo smudge 1>&2; cat" # .gitattributes
*.filter filter=noop
But what matters really is the credential helper: # .git/config
[credential]
helper = "!echo error 1>&2"
[remote "origin"]
url = https://github.com/user/private
fetch = +refs/heads/*:refs/remotes/origin/*
The The wiring actually works, and I double-checked that As the test-suite captures But I validated that by tracing, and any [src/cargo/sources/git/oxide.rs:271] repo.config_snapshot() = [credential]
<tab>helper = /Users/byron/dev/github.com/rust-lang/cargo/target/tmp/cit/t0/script/target/debug/script
[core]
<tab>bare = true
<tab>repositoryformatversion = 0
<tab>filemode = true
<tab>ignorecase = true
<tab>precomposeunicode = true
[gitoxide "http"]
<tab>connectTimeout = 30000
[http]
<tab>lowSpeedLimit = 10
<tab>lowSpeedTime = 30
<tab>userAgent = cargo 1.76.0 (930d6536e 2023-12-06)
[core]
<tab>logAllRefUpdates = false
[gitoxide "credentials"]
<tab>helperStderr = 0 # from GIX_CREDENTIALS_HELPER_STDERR I even validated that the credential-helper value is false:
So from all I can tell, it's working as it's supposed to, yet the system-wide credential helper still manages to write to stderr. Maybe, it writes to the terminal directly, and somehow that ends up being picked up by the test-suite. ConclusionSo I looked further and decided that the system-wide credential helper shouldn't be picked up in the first place. This is a feature, but the test-suite should be much more isolated. There are some ways to achieve this, and I think the most practical one is to assure that It turns out that |
) 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 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)
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
stderr
should be on or off by default - on by default like before, but now one can control it.gix
release and use it hereReview Notes
cargo
should keepstderr
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 disabledstderr
of credential helpers.On Windows,
gix
will call thegit-credential-manager, but with
stderrset to
inheritwhich makes any errors visible to the user, just like
git` does.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
updateshould allow to control what to do with
stderr
.