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

Download pre-compiled bootstrap from CI #112598

Closed
wants to merge 0 commits into from

Conversation

dvtkrlbs
Copy link
Contributor

@dvtkrlbs dvtkrlbs commented Jun 13, 2023

This PR builds on the work done by #98483 and downloads the bootstrap from the artifacts server if there is not any changes on the bootstrap source code. If the download fails it fallbacks to local build.

fixes #99989
helps with #94829

@rustbot
Copy link
Collaborator

rustbot commented Jun 13, 2023

r? @ozkanonur

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

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) labels Jun 13, 2023
@dvtkrlbs
Copy link
Contributor Author

r? @jyn514

@rustbot rustbot assigned jyn514 and unassigned onur-ozkan Jun 13, 2023
@dvtkrlbs
Copy link
Contributor Author

dvtkrlbs commented Jun 13, 2023

i could not find the button to turn the pr into draft it right now works but it is not a clean solution. I am not sure how to clean this it has a few hacks that i am not happy with

@rust-log-analyzer

This comment has been minimized.

@dvtkrlbs dvtkrlbs marked this pull request as draft June 13, 2023 22:40
@dvtkrlbs dvtkrlbs changed the title Download precompiled bootstrap from CI Download pre-compiled bootstrap from CI Jun 13, 2023
@dvtkrlbs
Copy link
Contributor Author

RIght now the wip code still tries to download even if there is a bootstrap locally. I am not sure what is the best way to handle this case. I first thought about chekcing if the cache folder has a bootstrap binary and then skipping download but this does not consider the case when user updates their old copy of the source code and in this case x.py will still use old bootstrap binary. What do you think is the best way to handle this case ?

@jyn514
Copy link
Member

jyn514 commented Jun 15, 2023

i don't have time for reviews, sorry. onur is working on the shell script side, he's a good reviewer.

r? @ozkanonur

@rustbot rustbot assigned onur-ozkan and unassigned jyn514 Jun 15, 2023
@onur-ozkan
Copy link
Member

I can do the review by the coming weekend. Until then, you can squash your commits and undraft the PR once it's ready.

@rustbot author

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 15, 2023
@rustbot rustbot added the WG-trait-system-refactor The Rustc Trait System Refactor Initiative (-Znext-solver) label Jun 17, 2023
@dvtkrlbs
Copy link
Contributor Author

dvtkrlbs commented Jun 17, 2023

I think this is ready for initial review the redownload logic is not fully clean but I am open to suggestions for best way to detect changes. I also screwed up my first rebase and the bot added a label.

@dvtkrlbs dvtkrlbs marked this pull request as ready for review June 17, 2023 16:05
@jyn514 jyn514 removed the WG-trait-system-refactor The Rustc Trait System Refactor Initiative (-Znext-solver) label Jun 17, 2023
Copy link
Member

@onur-ozkan onur-ozkan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe it's time to update this log

https://github.com/rust-lang/rust/blob/1673e54f679ebf67fc7c889b4bcba6d93add0456/src/bootstrap/bootstrap.py#L1124

Something like Checking if bootstrap needs to be downloaded or built before processing. should fit better

src/bootstrap/bootstrap.py Outdated Show resolved Hide resolved
src/bootstrap/bootstrap.py Outdated Show resolved Hide resolved
@dvtkrlbs
Copy link
Contributor Author

The one problem that I did not find a clean solution for still is even if there is a built bootstrap binary in the build folder and there is no changes it still tries to download so I think the bootstrap_modified also needs to detect if there is an up to date bootstrap binary. I am not sure how to best solve this.

@rust-log-analyzer

This comment has been minimized.

@onur-ozkan
Copy link
Member

I think the bootstrap_modified also needs to detect if there is an up to date bootstrap binary. I am not sure how to best solve this.

Yes, this should be checked too. You can create a stamp file on the same path with bootstrap binary and check that for this particular check.

@dvtkrlbs dvtkrlbs requested a review from onur-ozkan June 18, 2023 13:05
@dvtkrlbs
Copy link
Contributor Author

dvtkrlbs commented Jun 18, 2023

I added the stamp check. I am not sure about the best way to check the logic without having a CI artifact for the latest commit

Comment on lines 607 to 644
return True



def download_bootstrap(self, commit_hash):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Too many spaces

Comment on lines 617 to 665
def last_bootstrap_commit(self):
top_level = output(["git", "rev-parse", "--show-toplevel"])

cmd = ["git", "rev-list", "--author={}".format(self.git_merge_commit_mail),
"-n1", "--first-parent", "HEAD"]
merge_base = output(cmd)

path = "{}/src/bootstrap".format(top_level)
cmd = ["git", "diff-index", "--quiet", "HEAD", "--", path]

ret = subprocess.Popen(cmd, cwd=self.rust_root)
code = ret.wait()
if code != 0:
return None
else:
return merge_base
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This command alone git log -1 --pretty=format:%H src/bootstrap does what you need

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if you need short version of the commit hash, simply lowercase the H in --pretty

Comment on lines 634 to 672
def bootstrap_out_of_date(self, commit):
stamp_path = pathlib.Path(self.bin_root()).joinpath("bootstrap/.bootstrap-stamp")
if not stamp_path.exists():
return True

return stamp_path.read_text != commit
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems wrong as it will always return True because we don't have the stamp file on fresh repository.

@onur-ozkan
Copy link
Member

I haven't looked into this in detail, but I think the following steps should get the job done easily:

  • Check if anything has been changed in the src/bootstrap folder. If there are any modifications, compile the bootstrap.

  • If there haven't been any changes, first check if we have a stamp file:

    • If we do, compare its value with the latest bootstrap commit. If they don't match, download the latest version from the CI system.
    • If we don't have a stamp file, download it from the CI system.

Regardless of whether we need to compile or download, we need to make sure the stamp file has been generated.

@dvtkrlbs
Copy link
Contributor Author

Wait do we distribute the stamp-file with the CI as well. I thought it gets created after a successful download to mark the artifact.

@onur-ozkan
Copy link
Member

Wait do we distribute the stamp-file with the CI as well.

No, this is not needed.

@bors
Copy link
Contributor

bors commented Jun 25, 2023

☔ The latest upstream changes (presumably #113037) made this pull request unmergeable. Please resolve the merge conflicts.

@dvtkrlbs
Copy link
Contributor Author

fmt check
tidy check
dependency exception `dunce` license has changed
    previously `CC0-1.0 OR MIT-0 OR Apache-2.0` now `CC0-1.0 OR MIT-0`
    update EXCEPTIONS for the new license
some tidy checks failed
Build completed unsuccessfully in 0:00:37
error: failed to push some refs to 'github.com:dvtkrlbs/rust'

trying to rebasae my branch and getting this error. and it is not helpful at all it first does not tell which crate has this dependency and second it does not tell where this exceptions file or sections is

@dvtkrlbs
Copy link
Contributor Author

oh found it it was in the tools tidy deps

@Kobzol
Copy link
Contributor

Kobzol commented Jun 29, 2023

("dunce", "CC0-1.0 OR MIT-0 OR Apache-2.0"),

@rustbot rustbot added the A-testsuite Area: The testsuite used to check the correctness of rustc label Jun 29, 2023
@rustbot
Copy link
Collaborator

rustbot commented Jun 29, 2023

Some changes occurred in src/tools/cargo

cc @ehuss

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Contributor

bors commented Jul 1, 2023

☔ The latest upstream changes (presumably #113229) made this pull request unmergeable. Please resolve the merge conflicts.

@Dylan-DPC
Copy link
Member

@dvtkrlbs any updates on resolving the conflicts?

@dvtkrlbs
Copy link
Contributor Author

I will update my branch in the following days. Was there an easy way to resolve submodule conflicts there was a way to force reset them to HEAD without going through the conflict resolving but I don't remember how.

@dvtkrlbs
Copy link
Contributor Author

dvtkrlbs commented Aug 21, 2023

i really despise git sometimes I thought the rebase was resolved did the push and forgot about it. somehow it reset my branch. Looking at the reflog now to restore it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-testsuite Area: The testsuite used to check the correctness of rustc S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Download a pre-compiled bootstrap from CI
8 participants