-
Notifications
You must be signed in to change notification settings - Fork 13k
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
bootstrap tries to download the wrong link from ci-artifacts.rust-lang.org if the local repository is shallow #87890
Comments
Hm, can I do this? @rustbot label +A-rustbuild |
Mentoring instructions: find what this command outputs when the repository is shallow: rust/src/bootstrap/bootstrap.py Lines 669 to 672 in ae90dcf
If it's an error, see if you can check for a shallow repository somehow and give a hard error instead of continuing onward just to fail. |
Note that this shouldn't need to the full history of the repo, just the past few days, since PRs are constantly being merged. |
We should only need one (bors merge) commit, I believe - each bors merge has the full set of artifacts. |
The
The I tried to fetch more commits, and then
But |
cc @tlyu I would have expected the git log and git rev-list to be 'identical', in some sense, so perhaps that's something worth filing upstream as well. But regardless, I am inclined to revert #87532 to fix this issue; it was primarily a 'hopefully more robust' solution and to my knowledge wasn't actually fixing any known bugs, so since it caused a bug seems good to revert it for now... |
I think it's possible that the shallow clone is causing the
The git documentation implies that shallow clones use something akin to the I haven't had a chance to poke at a shallow clone in detail yet. |
This comment has been minimized.
This comment has been minimized.
Would love to see movement on this. I don't want to allocate ~20GB of laptop space to a deep clone of the repository in order to simply be able to build the compiler. When i was messing around, removing "--merges" as @tlyu suggested did the trick. |
@BGR360 feel free to make a PR with that change :) |
@jyn514 I wasn't sure if there was some reason not to do that:
I don't really grok most of what @tlyu said there, but if you think that's a safe change, then sure thing! |
As long as we keep first parent, dropping merges should be ok, any commit in the first parent chain should have artifacts (if it's recent enough). |
I would appreciate knowing details of what behavior you're seeing with git worktrees, if you're willing to provide them. |
This comment has been minimized.
This comment has been minimized.
@tlyu I don't think we need to exhaustively catalogue the issues - even if removing --merges doesn't fix all the issues it will fix some of them, and we can make more improvements in follow-ups. |
@rustbot claim |
Someone should probably file a bug about |
bootstrap: don't use `--merges` to look for commit hashes for downloading artifacts Shallow clones (and possibly worktrees, though I can't seem to reproduce the problem there) can cause `git rev-list --merges` to falsely return no results, even if a merge commit is present. Stop using the `--merges` option when looking for commit hashes that have build artifacts. `--first-parent` and `[email protected]` should be sufficient. Also exit with an error if the configuration asks for artifacts to be downloaded and we can't determine an appropriate commit hash to use to download artifacts. Fixes rust-lang#87890. r? `@jyn514` `@rustbot` label +A-rustbuild +A-contributor-roadblock
Steps to reproduce
git clone --depth 1 https://github.com/rust-lang/rust.git cd rust ./x.py setup compiler ./x.py
Expected output
An error message indicating that a full (or at least longer) git history is required.
Actual output (of final
x.py
invocation)The correct link would include a commit hash inbetween the
//
. But the required commit hash is not available because there is no full history.The text was updated successfully, but these errors were encountered: