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

Hint git-fetch-with-cli on git errors #8166

Merged
merged 1 commit into from
May 4, 2020
Merged

Conversation

kornelski
Copy link
Contributor

Our team has struggled with making Cargo git dependencies work in CI, until learning about this setting.

Cargo doesn't have a dedicated system for error hints like rustc, so I've stuffed the hint into the error message.

@alexcrichton
Copy link
Member

I'm a bit hesitant to blindly recommend for any and all errors that git-fetch-with-cli is used instead. Would it be possible to catch specifically authentication errors and make that recommendation instead? Additionally could this link to online documentation about this configuration option?

@kornelski
Copy link
Contributor Author

@alexcrichton I understand your concern. I've tried to narrow it down to specific libgit errors, but in the end it didn't feel like an improvement.

The first problem is that it's not limited to just auth errors. In my case libgit not using our VPN-aware proxy looks like a regular network errors, e.g. DNS error and connection timeouts.

Git errors generally fall into network errors, auth errors, invalid repo state, invalid command. The first two categories would benefit from the hint. The other two are unlikely to happen, because Cargo manages the repo state and commands itself.

Another problem with this is that just getting to the relevant libgit error codes via anyhow's chain takes more code than the rest of the checkout function. I don't know if you'd be OK with such relatively complicated thing for a small check.

@alexcrichton
Copy link
Member

Hm so extraction of error codes should be pretty simple, I think it's just calling downcast_ref? (note the outer .chain() isn't necessary there)

I'd personally still prefer to limit this somehow. I think it's ok to be a bit broad, but if there's something like a corrupt db or if a remote branch doesn't exist then you shouldn't get a recommendation to switch git implementations.

@kornelski
Copy link
Contributor Author

I've expanded it to check libgit's error class

@kornelski
Copy link
Contributor Author

Tests for my change pass. The failure comes from another test, which seems unrelated:

thread 'rustc_info_cache::rustc_info_cache' panicked at '
Expected: execs
    but: expected not to find:
[..] rustc info cache miss[..]

@alexcrichton
Copy link
Member

@bors: r+

Looks great to me, thanks for this! Those test failures should be fixed on master now.

@bors
Copy link
Contributor

bors commented May 4, 2020

📌 Commit 4fd4835 has been approved by alexcrichton

@bors bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label May 4, 2020
@bors
Copy link
Contributor

bors commented May 4, 2020

⌛ Testing commit 4fd4835 with merge 893db8c...

@bors
Copy link
Contributor

bors commented May 4, 2020

☀️ Test successful - checks-azure
Approved by: alexcrichton
Pushing 893db8c to master...

@bors bors merged commit 893db8c into rust-lang:master May 4, 2020
@kornelski kornelski deleted the net-cli branch May 4, 2020 21:57
@ehuss
Copy link
Contributor

ehuss commented May 6, 2020

This broke on rust-lang/rust CI:

---- git_auth::net_err_suggests_fetch_with_cli stdout ----
running `/checkout/obj/build/x86_64-unknown-linux-gnu/stage2-tools/x86_64-unknown-linux-gnu/release/cargo build -v`
running `/checkout/obj/build/x86_64-unknown-linux-gnu/stage2-tools/x86_64-unknown-linux-gnu/release/cargo build -v`
thread 'git_auth::net_err_suggests_fetch_with_cli' panicked at '
Expected: execs
    but: differences:
 16 - |ssh: Could not resolve hostname[..]|
    + |error: cannot run ssh: No such file or directory|

 19 - |Please make sure you have the correct access rights|
    +

 20 - |and the repository exists.|
    +

 21 - |[..]|
    +

I'm a little confused by the error. It almost looks like it is trying to run ssh, but I thought libgit used libssh. This is on an ubuntu 16 docker image. Can you please take a look?

@kornelski
Copy link
Contributor Author

kornelski commented May 6, 2020

Ouch. This is a limitation of Cargo's test comparison. The output contains strings that are specific to git and ssh, but the test can't account for varying number of lines :(

I've cut the test down to minimum.

kornelski added a commit to kornelski/cargo that referenced this pull request May 6, 2020
bors added a commit that referenced this pull request May 6, 2020
Avoid testing git-specific error messages

#8166
bors added a commit to rust-lang-ci/rust that referenced this pull request May 7, 2020
Update cargo

7 commits in 258c89644c4587273a3ed3ee9522d2640facba43..f534844c25cacc5e004404cea835ac85e35ca3fd
2020-04-30 21:48:21 +0000 to 2020-05-06 14:39:10 +0000
- Avoid testing git-specific error messages (rust-lang/cargo#8212)
- features: allow activated_features_unverified to communicate not-present (rust-lang/cargo#8194)
- Don't force rustc to do codegen for LTO builds (rust-lang/cargo#8192)
- Hint git-fetch-with-cli on git errors (rust-lang/cargo#8166)
- ¬∃x. ¬y => ∀x. y (rust-lang/cargo#8205)
- clippy fixes (rust-lang/cargo#8189)
- Rename bitcode-in-rlib flag to embed-bitcode (rust-lang/cargo#8204)
@ehuss ehuss added this to the 1.45.0 milestone Feb 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants