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

ignore llvm::Lld if lld is not enabled #126701

Merged
merged 6 commits into from
Jun 28, 2024
Merged

ignore llvm::Lld if lld is not enabled #126701

merged 6 commits into from
Jun 28, 2024

Conversation

onur-ozkan
Copy link
Member

@onur-ozkan onur-ozkan commented Jun 19, 2024 •
edited
Loading

People are having trouble (ref. zulip thread) when they don't want to build lld for their custom distribution tarballs even with lld = false in their config.toml. This is because it is not controlled by lld_enabled flag. This change ensures that llvm:Lld is controlled by lld configuration.

Additionally, lld = true is set by default for dist profile, because we have been building it all along and this maintains that behavior.

try-job: x86_64-mingw

@onur-ozkan
ignore llvm::Lld step if lld is not enabled
f22775b 
People are having trouble when they don't want to build `lld` for their custom
distribution tarballs even with `lld = false` in their config.toml. This is because
it is not controlled by `lld_enabled` flag. This change ensures that `llvm:Lld`
is controlled by lld configuration.

Signed-off-by: onur-ozkan <work@onurozkan.dev>
@rustbot
Copy link
Collaborator

rustbot commented Jun 19, 2024

r? @clubby789

rustbot has assigned @clubby789.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@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 19, 2024
@rustbot
Copy link
Collaborator

rustbot commented Jun 19, 2024

This PR modifies src/bootstrap/defaults.

If appropriate, please update CONFIG_CHANGE_HISTORY in src/bootstrap/src/utils/change_tracker.rs.

@onur-ozkan
Copy link
Member Author

This PR modifies src/bootstrap/defaults.

If appropriate, please update CONFIG_CHANGE_HISTORY in src/bootstrap/src/utils/change_tracker.rs.

src/bootstrap/defaults/config.dist.toml was updated to avoid a breaking change, so there is nothing to add to the change tracker.

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rustbot rustbot added A-testsuite Area: The testsuite used to check the correctness of rustc T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue. labels Jun 19, 2024
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@Kobzol
Copy link
Contributor

Kobzol commented Jun 27, 2024

r? @Kobzol

@rustbot rustbot assigned Kobzol and unassigned clubby789 Jun 27, 2024
@Kobzol
Copy link
Contributor

Kobzol commented Jun 27, 2024

This is a good cleanup, it should be possible to turn packaging of lld off. Let's do a try build just to check that the lld component stays there.

@bors try

@bors
Copy link
Contributor

bors commented Jun 27, 2024

⌛ Trying commit bfca652 with merge b5e90df...

bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 27, 2024
@bors
Auto merge of rust-lang#126701 - onur-ozkan:build-lld-if-enabled, r=<…
b5e90df 
…try>

ignore `llvm::Lld` if lld is not enabled

People are having trouble ([ref. zulip thread](https://rust-lang.zulipchat.com/#narrow/stream/326414-t-infra.2Fbootstrap/topic/MSVC.20Runtime.20mismatch.20when.20building.20LLD)) when they don't want to build `lld` for their custom distribution tarballs even with `lld = false` in their config.toml. This is because it is not controlled by `lld_enabled` flag. This change ensures that `llvm:Lld` is controlled by lld configuration.

Additionally, `lld = true` is set by default for dist profile, because we have been building it all along and this maintains that behavior.
@bors
Copy link
Contributor

bors commented Jun 27, 2024

☀️ Try build successful - checks-actions
Build commit: b5e90df (b5e90df655dfa6cc794cb3eaa8452acc9f64fb25)

@Kobzol
Copy link
Contributor

Kobzol commented Jun 27, 2024

Good, lld seems to be there!

One more thing, maybe we should add an entry to config tracker? Maybe some people who do their own dist would be interested in the changed default (they might have overridden lld before or they don't use the dist default profile).

@Kobzol
Copy link
Contributor

Kobzol commented Jun 27, 2024

Thanks!

@bors r+

@bors
Copy link
Contributor

bors commented Jun 27, 2024

📌 Commit b1b473e has been approved by Kobzol

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 Jun 27, 2024
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jun 28, 2024
@matthiaskrgr
Rollup merge of rust-lang#126701 - onur-ozkan:build-lld-if-enabled, r…
a100d94 
…=Kobzol

ignore `llvm::Lld` if lld is not enabled

People are having trouble ([ref. zulip thread](https://rust-lang.zulipchat.com/#narrow/stream/326414-t-infra.2Fbootstrap/topic/MSVC.20Runtime.20mismatch.20when.20building.20LLD)) when they don't want to build `lld` for their custom distribution tarballs even with `lld = false` in their config.toml. This is because it is not controlled by `lld_enabled` flag. This change ensures that `llvm:Lld` is controlled by lld configuration.

Additionally, `lld = true` is set by default for dist profile, because we have been building it all along and this maintains that behavior.
@lqd
Copy link
Member

lqd commented Jun 28, 2024

how/why was it ignored ? .... it probably should be an msvc only test, you don't link on gnu by using the linker directly, the missing libs are passed by the cc driving it

@Kobzol
Copy link
Contributor

Kobzol commented Jun 28, 2024

It was ignored because of //@ needs-rust-lld. Probably it needs //@ only-msvc instead of //@ only-windows then?

@lqd
Copy link
Member

lqd commented Jun 28, 2024

Ah so likely yet another case of rust-lld was only enabled on some dist builders and no test builder.

Probably something like that directive yes, or maybe even more precise à la x86_64-pc-windows-msvc, as IIUC there are a bunch of other msvc targets that may not have rust-lld support (or where it's distributed though I guess needs-rust-lld will take care of that). But I know nothing about windows and this may need the eyes of e.g. Chris Denton.

That is, if the test actually still passes and hasn't regressed since it was written :/ Maybe you can try it on x64 msvc and it it fails ping Chris the expert?

@Kobzol
Copy link
Contributor

Kobzol commented Jun 28, 2024

The test has succeeded on x86_64-msvc in the previous merge attempt. So probably we just need to modify the compiletest flag.

@lqd
Copy link
Member

lqd commented Jun 28, 2024

great then //@ only-x86_64-pc-windows-msvc and we're good

@onur-ozkan
update run-make/windows-safeseh compiletest header
17b843b 
Signed-off-by: onur-ozkan <work@onurozkan.dev>
@onur-ozkan
Copy link
Member Author

Testing 17b843b

@bors try

@bors
Copy link
Contributor

bors commented Jun 28, 2024

⌛ Trying commit 17b843b with merge 08efb2e...

bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 28, 2024
@bors
Auto merge of rust-lang#126701 - onur-ozkan:build-lld-if-enabled, r=<…
08efb2e 
…try>

ignore `llvm::Lld` if lld is not enabled

People are having trouble ([ref. zulip thread](https://rust-lang.zulipchat.com/#narrow/stream/326414-t-infra.2Fbootstrap/topic/MSVC.20Runtime.20mismatch.20when.20building.20LLD)) when they don't want to build `lld` for their custom distribution tarballs even with `lld = false` in their config.toml. This is because it is not controlled by `lld_enabled` flag. This change ensures that `llvm:Lld` is controlled by lld configuration.

Additionally, `lld = true` is set by default for dist profile, because we have been building it all along and this maintains that behavior.

try-job: x86_64-mingw
@bors
Copy link
Contributor

bors commented Jun 28, 2024

☀️ Try build successful - checks-actions
Build commit: 08efb2e (08efb2e4d2982a6d0095dcad7e83f33675b32a86)

@Kobzol
Copy link
Contributor

Kobzol commented Jun 28, 2024

@bors r+ rollup=never

@bors
Copy link
Contributor

bors commented Jun 28, 2024

📌 Commit 17b843b has been approved by Kobzol

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

@bors r=Kobzol

@bors
Copy link
Contributor

bors commented Jun 28, 2024

💡 This pull request was already approved, no need to approve it again.

@bors
Copy link
Contributor

bors commented Jun 28, 2024

📌 Commit 17b843b has been approved by Kobzol

It is now in the queue for this repository.

@onur-ozkan
Copy link
Member Author

@bors r+ rollup=never

oops, didn't see that

@bors
Copy link
Contributor

bors commented Jun 28, 2024

⌛ Testing commit 17b843b with merge e9e6e2e...

@bors
Copy link
Contributor

bors commented Jun 28, 2024

☀️ Test successful - checks-actions
Approved by: Kobzol
Pushing e9e6e2e to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jun 28, 2024
@bors bors merged commit e9e6e2e into rust-lang:master Jun 28, 2024
1 check passed
@rustbot rustbot added this to the 1.81.0 milestone Jun 28, 2024
@bors bors mentioned this pull request Jun 28, 2024
Open
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (e9e6e2e): comparison URL.

Overall result: ✅ improvements - no action needed

@rustbot label: -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

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

Max RSS (memory usage)

Results (primary -3.1%, secondary -5.2%)

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)		 -3.1% [-3.8%, -2.4%] 2
Improvements ✅
(secondary)		 -5.2% [-6.4%, -3.7%] 3
All ❌✅ (primary) -3.1% [-3.8%, -2.4%] 2

Cycles

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

Binary size

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

Bootstrap: 696.756s -> 694.621s (-0.31%)
Artifact size: 326.69 MiB -> 324.60 MiB (-0.64%)

@onur-ozkan onur-ozkan deleted the build-lld-if-enabled branch June 29, 2024 05:48
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jun 29, 2024
@matthiaskrgr
Rollup merge of rust-lang#127081 - Kobzol:lld-test, r=onur-ozkan
bbd3a78 
Add a run-make test that LLD is not being used by default on the x64 beta/stable channel

rust-lang#126701 showed that the handling of `lld` in bootstrap is currently not ideal. While it would be nice to refactor it eventually, we should also make sure that we have a test that checks that `lld` is not used (yet!) by default on the x64 Linux stable channel.

CC `@lqd`

r? `@onur-ozkan`
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jun 29, 2024
@matthiaskrgr
Rollup merge of rust-lang#127081 - Kobzol:lld-test, r=onur-ozkan
fafb2ea 
Add a run-make test that LLD is not being used by default on the x64 beta/stable channel

rust-lang#126701 showed that the handling of `lld` in bootstrap is currently not ideal. While it would be nice to refactor it eventually, we should also make sure that we have a test that checks that `lld` is not used (yet!) by default on the x64 Linux stable channel.

CC ``@lqd``

r? ``@onur-ozkan``
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Jun 30, 2024
@rust-timer
Unrolled build for rust-lang#127081
2587663 
Rollup merge of rust-lang#127081 - Kobzol:lld-test, r=onur-ozkan

Add a run-make test that LLD is not being used by default on the x64 beta/stable channel

rust-lang#126701 showed that the handling of `lld` in bootstrap is currently not ideal. While it would be nice to refactor it eventually, we should also make sure that we have a test that checks that `lld` is not used (yet!) by default on the x64 Linux stable channel.

CC ``@lqd``

r? ``@onur-ozkan``
@SergioGasquez SergioGasquez mentioned this pull request Jul 5, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Kobzol Kobzol Kobzol left review comments

Assignees

@Kobzol Kobzol

Labels
A-run-make Area: port run-make Makefiles to rmake.rs A-testsuite Area: The testsuite used to check the correctness of rustc 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) T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue.
Projects
Oxidizing run-make tests
Status: Done
Milestone
1.81.0
Development

Successfully merging this pull request may close these issues.
10 participants
@onur-ozkan @rustbot @rust-log-analyzer @Kobzol @bors @tgross35 @matthiaskrgr @lqd @rust-timer @clubby789