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.

Sign up for GitHub

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

Jump to bottom

bootstrap: use git merge-base for LLVM CI download logic #113588

Merged
merged 2 commits into from
Aug 1, 2023
Merged

bootstrap: use git merge-base for LLVM CI download logic #113588

merged 2 commits into from
Aug 1, 2023

Conversation

RalfJung
Copy link
Member

Fixes #101907
I tested this with a local branch that has extra merge commits due to Miri, and it worked fine there. But I am sure there are tons of other situations I did not think of...

r? @jyn514

@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 Jul 11, 2023
@rustbot
Copy link
Collaborator

rustbot commented Jul 11, 2023

This PR changes how LLVM is built. Consider updating src/bootstrap/download-ci-llvm-stamp.

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@RalfJung
Copy link
Member Author

Hm, this doesn't seem to work on CI already...

fatal: Not a valid object name origin/master

Does that checkout have the right remote under some other name? Or should we fall back to just master if we cannot find a suitable remote?

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@jyn514
Copy link
Member

jyn514 commented Jul 11, 2023

don't have time for reviews

r? bootstrap

@rustbot rustbot assigned albertlarsan68 and unassigned jyn514 Jul 11, 2023
@RalfJung
Copy link
Member Author

Looks like CI doesn't have any master branch of any sort, so the logic is bound to fail.

But I am confused that the logic even gets called there, doesn't CI need to always build LLVM itself anyway? The old logic would have returned the just-created fresh bors merge commit, and then it would have failed to find an LLVM build for that commit -- right?

@jyn514
Copy link
Member

jyn514 commented Jul 12, 2023

But I am confused that the logic even gets called there, doesn't CI need to always build LLVM itself anyway? The old logic would have returned the just-created fresh bors merge commit, and then it would have failed to find an LLVM build for that commit -- right?

see the logic in is_ci_llvm_modified, which handles that case. maybe there's some way to unify it with the logic you're modifying, not sure.

cc #113250

@albertlarsan68
Copy link
Member

@rustbot author
Flip back to me once CI fixed :)

@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 Jul 13, 2023
@bors
Copy link
Contributor

bors commented Jul 13, 2023

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

@albertlarsan68
Copy link
Member

CI only runs when there are no merge conflicts, because it runs on the merge result.

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@RalfJung
Copy link
Member Author

Interesting, the rev-list path filtering seems to be doing absolutely nothing on CI. I guess this is because of the shallow clone.

I see two options for what we could do here:

  • either we run something like git fetch origin master --depth 1 on CI so that we know the current master branch, and can use it to determine the merge-base
  • or we keep (some variant of) the old LLVM detection logic around as a fallback when origin/master cannot be found.

@albertlarsan68 any preferences?

@albertlarsan68
Copy link
Member

albertlarsan68 commented Jul 14, 2023 •
edited
Loading

If you are able to make option 1 work, the go ahead. Otherwise, I think the option 2 is a good compromise.

@RalfJung RalfJung force-pushed the llvm-merge-base branch 2 times, most recently from a4ca940 to a4c9ca4 Compare July 16, 2023 16:10
@RalfJung
Copy link
Member Author

I couldn't quite figure out the best way to do this, so I went with a different approach: we just use git merge-base to get the starting point for the doing the "search last bors commit that changed LLVM" as before. That should also keep intact the assumptions made by is_ci_llvm_modified.

@RalfJung
Copy link
Member Author

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jul 16, 2023
albertlarsan68
albertlarsan68 approved these changes Jul 17, 2023
View reviewed changes
Copy link
Member

@albertlarsan68 albertlarsan68 left a comment

Choose a reason for hiding this comment

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

LGTM, r=me with or without the change

src/tools/build_helper/src/git.rs Outdated Show resolved Hide resolved
@RalfJung @albertlarsan68
apply nit
314fe5d 
Co-authored-by: Albert Larsan <albertlarsan@unbon.cafe>
@RalfJung
Copy link
Member Author

@bors r=albertlarsan68

@bors
Copy link
Contributor

bors commented Jul 31, 2023

📌 Commit 314fe5d has been approved by albertlarsan68

It is now in the queue for this repository.

@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 Jul 31, 2023
@matthiaskrgr matthiaskrgr mentioned this pull request Aug 1, 2023
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 1, 2023
@bors
Auto merge of rust-lang#114318 - matthiaskrgr:rollup-c7gcw18, r=matth…
c435af0 
…iaskrgr

Rollup of 7 pull requests

Successful merges:

 - rust-lang#111081 (impl SliceIndex<str> for (Bound<usize>, Bound<usize>))
 - rust-lang#113394 (style-guide: Document style editions, start 2024 style edition)
 - rust-lang#113588 (bootstrap: use git merge-base for LLVM CI download logic)
 - rust-lang#113743 (Directly link more target docs)
 - rust-lang#114262 (Improve the rust style guide doc)
 - rust-lang#114309 (Update books)
 - rust-lang#114313 ([rustc_data_structures] Simplify SortedMap::insert.)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 849f4f8 into rust-lang:master Aug 1, 2023
@rustbot rustbot added this to the 1.73.0 milestone Aug 1, 2023
@RalfJung RalfJung deleted the llvm-merge-base branch August 1, 2023 16:15
lqd added a commit to lqd/rust that referenced this pull request Aug 1, 2023
@lqd
Revert "Rollup merge of rust-lang#113588 - RalfJung:llvm-merge-base, …
7c12b2e 
…r=albertlarsan68"

This reverts commit 849f4f8, reversing
changes made to 0242643.
@lqd lqd mentioned this pull request Aug 1, 2023
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 2, 2023
@bors
Auto merge of rust-lang#114345 - lqd:revert-113588, r=RalfJung
6e6d39a 
Revert rust-lang#113588 to fix bootstrap timings

This reverts rust-lang#113588 which seems to have broken perf's bootstrap timings via some git issue

rust-lang#114318 (comment) show a newly broken benchmark, the error at the time was

```
fatal: Path 'src/ci/channel' exists on disk, but not in 'e62323df22ecf9c163023132d17b7114f68b72e8'.
       thread 'main' panicked at 'command did not execute successfully: cd "/home/collector/rustc-perf/rust" && "git" "show" "e62323df22ecf9c163023132d17b7114f68b72e8:src/ci/channel"
       expected success, got: exit status: 128', config.rs:1786:27
```

If this lands, it will reopen rust-lang#101907 and annoy miri, but it could actually be an issue that would appear during the next bootstrap bump, not just rustc-perf today.

r? `@ghost`
@RalfJung RalfJung mentioned this pull request Aug 31, 2023
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 2, 2023
@bors
Auto merge of rust-lang#115409 - RalfJung:llvm-merge-reland, r=lqd
585bb5e 
bootstrap: use git merge-base for LLVM CI download logic

This re-lands rust-lang#113588, now that the perf issues are hopefully fixed by rust-lang/rustc-perf#1684.
r? `@lqd` `@Mark-Simulacrum`

Fixes rust-lang#101907
RalfJung pushed a commit to RalfJung/miri that referenced this pull request Sep 3, 2023
@bors
Auto merge of #115409 - RalfJung:llvm-merge-reland, r=lqd
b086219 
bootstrap: use git merge-base for LLVM CI download logic

This re-lands rust-lang/rust#113588, now that the perf issues are hopefully fixed by rust-lang/rustc-perf#1684.
r? `@lqd` `@Mark-Simulacrum`

Fixes rust-lang/rust#101907
@RalfJung RalfJung mentioned this pull request Jan 19, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@albertlarsan68 albertlarsan68 albertlarsan68 approved these changes

Assignees

@albertlarsan68 albertlarsan68

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)
Projects
None yet
Milestone
1.73.0
Development

Successfully merging this pull request may close these issues.

Merge commits break LLVM CI download
6 participants
@RalfJung @rustbot @rust-log-analyzer @jyn514 @albertlarsan68 @bors