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

Detect git version before attempting to use --progress #71559

Merged
merged 1 commit into from
Apr 30, 2020

Conversation

dillona
Copy link
Contributor

@dillona dillona commented Apr 25, 2020

Otherwise each update is run twice and errors are printed

I've tested this with:
git version 2.8.2.windows.1 (Windows)
git version 2.26.2.266.ge870325ee8 (Linux built from source)
git version 2.17.1 (Linux)
git version 2.21.1 (Apple Git-122.3) (MacOS)

I've tested with Python 2.7 (Windows, Linux, MacOS), 3.6 (Linux), and 3.7 (MacOS)

@rust-highfive
Copy link
Collaborator

r? @Mark-Simulacrum

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 25, 2020
Otherwise each update is run twice and errors are printed
@dillona dillona force-pushed the detect_git_progress_version branch from d702bd8 to 7ac093f Compare April 25, 2020 20:31
@Mark-Simulacrum
Copy link
Member

I would prefer to avoid version detection -- can we set a (global, I guess) flag that we've been unable to run with --progress and if so don't try to do so for each submodule? That should reduce the amount of errors we see.

AFAICT, git version 2.11 went out of support around 2017-09-22 -- how are you getting it on Windows? Do we have some idea of usage of that platform?

@dillona
Copy link
Contributor Author

dillona commented Apr 26, 2020

We could certainly save a global variable indicating something like "We once failed when running with --progress then succeeded running without it". However I'd really like to find something that at least doesn't show the user any errors. It makes it look like your build is broken when it actually isn't. I'm not sure if there are any commands we could run to detect the presence of the --progress feature, though that would in some ways be the cleanest solution.

This came up because I happened to have Git 2.8.2 installed on my Windows computer, presumably from when that was current. I suspect this is actually somewhat common on Windows, as there's no built-in update mechanism.

@dillona
Copy link
Contributor Author

dillona commented Apr 26, 2020

We could run a git submodule --help and search for the "--progress" string

@Mark-Simulacrum
Copy link
Member

My thought is that, essentially, it seems like these old versions of git likely also have security vulnerabilities or so - I can't be certain, of course, but it seems like people would want to migrate off of them. I guess I'm not opposed to version detection in principle (we already pay the cost of the additional git run), but I feel odd explicitly supporting such old versions of git.

I'll think some more about this, but since we do encounter old versions in the wild legitimately, I'm leaning towards accepting this. I'll try to make a decision Monday.

@Mark-Simulacrum
Copy link
Member

I decided that since we're not running more commands now, this seems fine; if it was a perf regression for us then I'd be more concerned.

@bors r+

@bors
Copy link
Contributor

bors commented Apr 27, 2020

📌 Commit 7ac093f has been approved by Mark-Simulacrum

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 27, 2020
RalfJung added a commit to RalfJung/rust that referenced this pull request Apr 30, 2020
…, r=Mark-Simulacrum

Detect git version before attempting to use --progress

Otherwise each update is run twice and errors are printed

I've tested this with:
git version 2.8.2.windows.1 (Windows)
git version 2.26.2.266.ge870325ee8 (Linux built from source)
git version 2.17.1 (Linux)
git version 2.21.1 (Apple Git-122.3) (MacOS)

I've tested with Python 2.7 (Windows, Linux, MacOS), 3.6 (Linux), and 3.7 (MacOS)
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 30, 2020
Rollup of 5 pull requests

Successful merges:

 - rust-lang#70950 (extend NLL checker to understand `'empty` combined with universes)
 - rust-lang#71433 (Add help message for missing right operand in condition)
 - rust-lang#71449 (Move `{Free,}RegionRelations` and `FreeRegionMap` to `rustc_infer`)
 - rust-lang#71559 (Detect git version before attempting to use --progress)
 - rust-lang#71597 (Rename Unique::empty() -> Unique::dangling())

Failed merges:

r? @ghost
@bors bors merged commit 2770f82 into rust-lang:master Apr 30, 2020
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