Skip to content

fix download-llvm logic for subtree sync branches #137593

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Merged
merged 1 commit into from
Mar 24, 2025

Conversation

RalfJung
Copy link
Member

@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 Feb 25, 2025
@Mark-Simulacrum
Copy link
Member

@bors r+

@bors
Copy link
Collaborator

bors commented Feb 25, 2025

📌 Commit 88f14a8 has been approved by Mark-Simulacrum

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 Feb 25, 2025
@Kobzol Kobzol mentioned this pull request Feb 26, 2025
@klensy
Copy link
Contributor

klensy commented Feb 26, 2025

This change partially merged in #137594, weird.

@onur-ozkan
Copy link
Member

@bors r-

It's already merged, and regressed LLVM commit finding logic (see #137661 (comment)).

@bors bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Feb 26, 2025
@RalfJung RalfJung force-pushed the subtree-sync-download-llvm branch from 88f14a8 to 598755d Compare March 17, 2025 14:49
@RalfJung
Copy link
Member Author

RalfJung commented Mar 17, 2025

I have updated the PR based on what I found here. I still think we shouldn't even try to look at the history when we know that basically none of the history has even been fetched, but that's a much larger change I will not undertake. I hope (and confirmed to the extent that I can, by playing with git log in a sparse checkout) this change now both fixes the issue I am running into and keeps CI working.

@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 Mar 17, 2025
@RalfJung RalfJung force-pushed the subtree-sync-download-llvm branch from 598755d to 0ee9456 Compare March 17, 2025 14:52
@Mark-Simulacrum
Copy link
Member

r=me if we didn't need to revert this somewhere (I seem to vaguely recall that happening somewhere)

@RalfJung
Copy link
Member Author

We had to revert an earlier version of this. So I went for a different approach now, which should fix that issue.

@bors r=Mark-Simulacrum

@bors
Copy link
Collaborator

bors commented Mar 23, 2025

📌 Commit 0ee9456 has been approved by Mark-Simulacrum

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 Mar 23, 2025
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 24, 2025
Rollup of 10 pull requests

Successful merges:

 - rust-lang#137593 (fix download-llvm logic for subtree sync branches)
 - rust-lang#137736 (Don't attempt to export compiler-builtins symbols from rust dylibs)
 - rust-lang#138135 (Simplify `PartialOrd` on tuples containing primitives)
 - rust-lang#138321 ([bootstrap] Distribute split debuginfo if present)
 - rust-lang#138574 (rustdoc: be more strict about "Methods from Deref")
 - rust-lang#138606 (Fix missing rustfmt in msi installer - cont)
 - rust-lang#138671 (Fix `FileType` `PartialEq` implementation on Windows)
 - rust-lang#138728 (Update `compiler-builtins` to 0.1.152)
 - rust-lang#138783 (Cache current_dll_path output)
 - rust-lang#138846 (Tweaks to writeback and `Obligation -> Goal` conversion)

Failed merges:

 - rust-lang#138755 ([rustdoc] Remove duplicated loop when computing doc cfgs)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 95994f9 into rust-lang:master Mar 24, 2025
6 checks passed
@rustbot rustbot added this to the 1.87.0 milestone Mar 24, 2025
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Mar 24, 2025
Rollup merge of rust-lang#137593 - RalfJung:subtree-sync-download-llvm, r=Mark-Simulacrum

fix download-llvm logic for subtree sync branches

Fixes rust-lang#101907

Cc `@onur-ozkan`
r? `@Mark-Simulacrum`
@jieyouxu
Copy link
Member

jieyouxu commented Mar 24, 2025

Hm, this might've somehow broken perf, but I'm not 100% (see #138873 (comment))

@jieyouxu
Copy link
Member

It's like the rev-list args is somehow wrong because the rustc-perf log output has

usage: git rev-list [OPTION] <commit-id>... [ -- paths... ]
         limiting output:
           --max-count=<n>
           --max-age=<epoch>
           --min-age=<epoch>
           --sparse
           --no-merges
           --min-parents=<n>
           --no-min-parents
           --max-parents=<n>
           --no-max-parents
           --remove-empty
           --all
           --branches
           --tags
           --remotes
           --stdin
           --quiet
         ordering output:
           --topo-order
           --date-order
           --reverse
         formatting output:
           --parents
           --children
           --objects | --objects-edge
           --unpacked
           --header | --pretty
           --abbrev=<n> | --no-abbrev
           --abbrev-commit
           --left-right
           --count
         special purpose:
           --bisect
           --bisect-vars
           --bisect-all

Comment on lines -157 to +165
"--first-parent",
"--diff-merges=first-parent",
Copy link
Member

Choose a reason for hiding this comment

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

I think --diff-merges is a git show-only flag, not git rev-list.

Copy link
Member Author

Choose a reason for hiding this comment

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

I tried it locally and it works for git rev-list as well.

$ git --version
git version 2.47.2

jieyouxu added a commit to jieyouxu/rust that referenced this pull request Mar 24, 2025
…load-llvm, r=Mark-Simulacrum"

Looks like unfortunately the `--diff-merges` flag is a `git show`-only
command, not `git rev-list`.

This reverts commit 95994f9, reversing
changes made to 7290b04.
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 24, 2025
Revert "fix download-llvm logic for subtree sync branches rust-lang#137593"

Looks like unfortunately the `--diff-merges` flag is a `git show`-only command, not `git rev-list`.

See https://git-scm.com/docs/git-rev-list#Documentation/git-rev-list.txt---first-parent which has `--first-parent`, versus https://git-scm.com/docs/git-show#Documentation/git-show.txt---diff-mergesltformatgt which has `--diff-merges=first-parent`.

This reverts commit 95994f9, reversing changes made to 7290b04.

This will unfortunately re-open rust-lang#101907 but that isn't fixed anyway since the git invocation is broken.

r? `@onur-ozkan` (or bootstrap or infra or anyone really)
@jieyouxu
Copy link
Member

I posted a revert #138878 to fix perf, because I know nothing about the distinction between these two flags, so a fix-forward seems also risky.

bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 24, 2025
Revert "fix download-llvm logic for subtree sync branches rust-lang#137593"

Reverts rust-lang#137593.

Looks like unfortunately the `--diff-merges=first-parent` flag is a `git show`-only flag, not `git rev-list` which only accepts `--first-parent`.

See https://git-scm.com/docs/git-rev-list#Documentation/git-rev-list.txt---first-parent which has `--first-parent`, versus https://git-scm.com/docs/git-show#Documentation/git-show.txt---diff-mergesltformatgt which has `--diff-merges=first-parent`.

This reverts commit 95994f9, reversing changes made to 7290b04.

This will unfortunately re-open rust-lang#101907 but that isn't fixed anyway since the git invocation is broken.

cc `@RalfJung` `@Mark-Simulacrum` for FYI (but I would've written the same incorrect flag 💀)
r? `@onur-ozkan` (or bootstrap or infra or anyone really)
@RalfJung RalfJung deleted the subtree-sync-download-llvm branch March 25, 2025 07:24
# for free to join this conversation on GitHub. Already have an account? # to comment
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
Development

Successfully merging this pull request may close these issues.

Merge commits break LLVM CI download
7 participants