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

fix llvm_out to use the correct LLVM root #120721

Merged
merged 2 commits into from
Feb 10, 2024
Merged

fix llvm_out to use the correct LLVM root #120721

merged 2 commits into from
Feb 10, 2024

Conversation

onur-ozkan
Copy link
Member

@onur-ozkan onur-ozkan commented Feb 6, 2024 •
edited

When download-ci-llvm is enabled, llvm_out ends up with the
error below due to an incorrect path on cross-compilations. This change fixes that.

failed to execute command: "/rust/build/x86_64-unknown-linux-gnu/llvm/build/bin/llvm-config" "--version"
ERROR: No such file or directory (os error 2)

@rustbot
Copy link
Collaborator

rustbot commented Feb 6, 2024

r? @albertlarsan68

(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 Feb 6, 2024
@onur-ozkan onur-ozkan changed the title fix llvm_out to find the correct LLVM root. fix llvm_out to find the correct LLVM root Feb 6, 2024
@onur-ozkan onur-ozkan force-pushed the incorrect-llvm-path-on-cross-target branch 2 times, most recently from d780721 to d3848dc Compare February 6, 2024 21:55
@rustbot
Copy link
Collaborator

rustbot commented Feb 6, 2024

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

@onur-ozkan onur-ozkan changed the title fix llvm_out to find the correct LLVM root fix prebuilt_llvm_config to use the correct LLVM root Feb 6, 2024
@onur-ozkan
fix llvm_out to use the correct LLVM root
7fb4512 
When `download-ci-llvm` is enabled, `llvm_out` ends up with the
error below due to an incorrect path on cross-compilations. This change fixes that.

```
failed to execute command: "/rust/build/x86_64-unknown-linux-gnu/llvm/build/bin/llvm-config" "--version"
ERROR: No such file or directory (os error 2)
```

Signed-off-by: onur-ozkan <work@onurozkan.dev>
@onur-ozkan onur-ozkan force-pushed the incorrect-llvm-path-on-cross-target branch from d3848dc to 7fb4512 Compare February 6, 2024 22:56
@onur-ozkan onur-ozkan changed the title fix prebuilt_llvm_config to use the correct LLVM root fix llvm_out to use the correct LLVM root Feb 6, 2024
@albertlarsan68
Copy link
Member

Thanks for the PR!
@bors r+

@bors
Copy link
Contributor

bors commented Feb 7, 2024

📌 Commit 7fb4512 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 Feb 7, 2024
@onur-ozkan
test llvm_out behaviour
63cc3c7 
Signed-off-by: onur-ozkan <work@onurozkan.dev>
@onur-ozkan
Copy link
Member Author

onur-ozkan commented Feb 7, 2024 •
edited

I just added a unit test for testing the behaviour of llvm_out (just in case we don't break anything now and in the future).

@bors r-

@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 7, 2024
@onur-ozkan onur-ozkan 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 Feb 7, 2024
@albertlarsan68
Copy link
Member

Thanks for the PR!
r=me when CI is green

@onur-ozkan
Copy link
Member Author

@bors r=albertlarsan68 rollup

@bors
Copy link
Contributor

bors commented Feb 7, 2024

📌 Commit 63cc3c7 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 Feb 7, 2024
Nadrieril added a commit to Nadrieril/rust that referenced this pull request Feb 7, 2024
@Nadrieril
Rollup merge of rust-lang#120721 - onur-ozkan:incorrect-llvm-path-on-…
0d3ff16 
…cross-target, r=albertlarsan68

fix `llvm_out` to use the correct LLVM root

When `download-ci-llvm` is enabled, `llvm_out` ends up with the
error below due to an incorrect path on cross-compilations. This change fixes that.

```sh
failed to execute command: "/rust/build/x86_64-unknown-linux-gnu/llvm/build/bin/llvm-config" "--version"
ERROR: No such file or directory (os error 2)
```
@Nadrieril Nadrieril mentioned this pull request Feb 7, 2024
Closed
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 7, 2024
@bors
Auto merge of rust-lang#120743 - Nadrieril:rollup-h2mxjc2, r=Nadrieril
e8ec9f4 
Rollup of 13 pull requests

Successful merges:

 - rust-lang#110482 (Add armv8r-none-eabihf target for the Cortex-R52.)
 - rust-lang#119162 (Add unstable `-Z direct-access-external-data` cmdline flag for `rustc`)
 - rust-lang#120302 (various const interning cleanups)
 - rust-lang#120455 ( Add FileCheck annotations to MIR-opt SROA tests)
 - rust-lang#120470 (Mark "unused binding" suggestion as maybe incorrect)
 - rust-lang#120479 (Suggest turning `if let` into irrefutable `let` if appropriate)
 - rust-lang#120564 (coverage: Split out counter increment sites from BCB node/edge counters)
 - rust-lang#120633 (pattern_analysis: gather up place-relevant info)
 - rust-lang#120664 (Add parallel rustc ui tests)
 - rust-lang#120721 (fix `llvm_out` to use the correct LLVM root)
 - rust-lang#120726 (Don't use bashism in checktools.sh)
 - rust-lang#120733 (MirPass: make name more const)
 - rust-lang#120735 (Remove some `unchecked_claim_error_was_emitted` calls)

Failed merges:

 - rust-lang#120727 (exhaustiveness: Prefer "`0..MAX` not covered" to "`_` not covered")

r? `@ghost`
`@rustbot` modify labels: rollup
@Nadrieril
Copy link
Member

I got this linker-related rollout failure, could this be the culprit?

@onur-ozkan
Copy link
Member Author

I am not so sure.

@bors rollup=iffy

@Nadrieril
Copy link
Member

FWIW the rest of the rollup worked

@Nadrieril
Copy link
Member

Nadrieril commented Feb 8, 2024 •
edited

Two interesting things in the error output:

  • -iphoneos_version_min has been renamed to -ios_version_min
  • ld: unknown options: -ios_simulator_version_min

Googling around, it looks like Xcode 15 beta 3 has the same two things. Apparently because of the ld version they use. I'd guess that the ci-downloaded llvm has a different ld and your change revealed that.

@Nadrieril
Copy link
Member

Nadrieril commented Feb 8, 2024 •
edited

This doesn't look like a spurious failure at least. Maybe @rust-lang/bootstrap would know more? For the bootstrap team: this PR makes more use of the CI LLVM, which as far as I can tell triggered a problem with ld flags on x86_64-apple-darwin (error here).

@bors r-

From googling, suggested workaround is to use the -ld64 flag to use the old linker. Or use the newer -platform_version flag instead.

@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 8, 2024
@onur-ozkan
Copy link
Member Author

onur-ozkan commented Feb 8, 2024 •
edited

I don't understand how this is relevant. Currently, llvm_out is returning the compiled LLVM path. This PR modifies it to return the ci-llvm path if download-ci-llvm is enabled. However, download-ci-llvm is disabled in the CI pipeline (failed one) anyway . Therefore, this PR shouldn't have any effect there.

@Nadrieril
Copy link
Member

Nadrieril commented Feb 8, 2024 •
edited

Oh. removing this PR from the rollup fixed the error though, do you think it was a fluke?

@onur-ozkan
Copy link
Member Author

do you think it was a fluke?

I think it was

@Nadrieril
Copy link
Member

Alright then, I can't claim to understand much in this topic

@bors r=albertlarsan68 rollup=iffy

@bors
Copy link
Contributor

bors commented Feb 8, 2024

📌 Commit 63cc3c7 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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 8, 2024
@bors
Copy link
Contributor

bors commented Feb 10, 2024

⌛ Testing commit 63cc3c7 with merge 68125c7...

@bors
Copy link
Contributor

bors commented Feb 10, 2024

☀️ Test successful - checks-actions
Approved by: albertlarsan68
Pushing 68125c7 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Feb 10, 2024
@bors bors merged commit 68125c7 into rust-lang:master Feb 10, 2024
12 checks passed
@rustbot rustbot added this to the 1.78.0 milestone Feb 10, 2024
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (68125c7): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

This benchmark run did not return any relevant results for this metric.

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)		 - - 0
Regressions ❌
(secondary)		 - - 0
Improvements ✅
(primary)		 - - 0
Improvements ✅
(secondary)		 -3.8% [-3.8%, -3.8%] 1
All ❌✅ (primary) - - 0

Binary size

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)		 - - 0
Regressions ❌
(secondary)		 0.1% [0.1%, 0.1%] 1
Improvements ✅
(primary)		 - - 0
Improvements ✅
(secondary)		 - - 0
All ❌✅ (primary) - - 0

Bootstrap: 666.849s -> 667.844s (0.15%)
Artifact size: 308.02 MiB -> 308.06 MiB (0.01%)

@Nadrieril
Copy link
Member

I stand corrected, sorry for making a fuss 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@albertlarsan68 albertlarsan68

Labels
merged-by-bors This PR was explicitly merged by bors. 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.78.0
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@onur-ozkan @rustbot @albertlarsan68 @bors @Nadrieril @rust-timer