Conditionally build `wasm-component-ld` by alexcrichton · Pull Request #127866 · rust-lang/rust (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation13 Commits2 Checks6 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

This commit updates the support for the wasm-component-ld tool from #126967 to conditionally build rather than unconditionally building it when LLD is enabled. This support is disabled by default and can be enabled by one of two means:

the extended field in config.toml which dist builders use to build a complete set of tools for each host platform.
a "wasm-component-ld" entry in the tools section of config.toml.

Neither of these are enabled by default meaning that most local builds will likely not have this new tool built. Dist builders should still, however, build the tool.

r? @onur-ozkan

rustbot has assigned @onur-ozkan.
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

This PR modifies config.example.toml.

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

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 17, 2024

This comment has been minimized.

alexcrichton changed the title~~Conditionally build wasm-component-ld when lld is enabled~~ Conditionally build wasm-component-ld

Jul 17, 2024

Choose a reason for hiding this comment

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

LGTM other than a missing change record.

This PR modifies config.example.toml.

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

Please add it in change_tracker module similar to

ChangeInfo {
change_id: 124129,
severity: ChangeSeverity::Warning,
summary: "`rust.lld` has a new default value of `true` on `x86_64-unknown-linux-gnu`. Starting at stage1, `rust-lld` will thus be this target's default linker. No config changes should be necessary.",
},

Sure thing, added. I figured this was a pretty niche and small change and it looks like all users get warnings/prints/etc when updating existing checkouts so I didn't want to spam messages, but if it's desired to have an exhaustive changelog I'm happy to add the entry.

Sure thing, added. I figured this was a pretty niche and small change and it looks like all users get warnings/prints/etc when updating existing checkouts so I didn't want to spam messages, but if it's desired to have an exhaustive changelog I'm happy to add the entry.

Thanks, no worries about the spam; records are printed only once (unless the build directory is removed).

@bors r+

📌 Commit d0777ca has been approved by onur-ozkan

It is now in the queue for this repository.

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

Jul 18, 2024

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

Jul 19, 2024

This commit updates the support for the wasm-component-ld tool from rust-lang#126967 to conditionally build it rather than unconditionally building it when LLD is enabled. This support is disabled by default and can be enabled by one of two means:

the extended field in config.toml which dist builders use to build a complete set of tools for each host platform.
a "wasm-component-ld" entry in the tools section of config.toml.

Neither of these are enabled by default meaning that most local builds will likely not have this new tool built. Dist builders should still, however, build the tool.

📌 Commit f0a2b5b has been approved by onur-ozkan

It is now in the queue for this repository.

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

Jul 19, 2024

tgross35 added a commit to tgross35/rust that referenced this pull request

Jul 19, 2024

…t-ld-by-default, r=onur-ozkan

Conditionally build wasm-component-ld

This commit updates the support for the wasm-component-ld tool from rust-lang#126967 to conditionally build rather than unconditionally building it when LLD is enabled. This support is disabled by default and can be enabled by one of two means:

the extended field in config.toml which dist builders use to build a complete set of tools for each host platform.
a "wasm-component-ld" entry in the tools section of config.toml.

Neither of these are enabled by default meaning that most local builds will likely not have this new tool built. Dist builders should still, however, build the tool.

bors added a commit to rust-lang-ci/rust that referenced this pull request

Jul 20, 2024

Rollup of 8 pull requests

Successful merges:

rust-lang#123196 (Add Process support for UEFI)
rust-lang#127523 (Migrate dump-ice-to-disk and panic-abort-eh_frame run-make tests to rmake)
rust-lang#127556 (Replace a long inline "autoref" comment with method docs)
rust-lang#127693 (Migrate crate-hash-rustc-version to rmake)
rust-lang#127866 (Conditionally build wasm-component-ld )
rust-lang#127918 (Safely enforce thread name requirements)
rust-lang#127948 (fixes panic error index out of bounds in conflicting error)
rust-lang#127980 (Avoid ref when using format! in compiler)

r? @ghost @rustbot modify labels: rollup

bors added a commit to rust-lang-ci/rust that referenced this pull request

Jul 20, 2024

bors added a commit to rust-lang-ci/rust that referenced this pull request

Jul 20, 2024

rust-timer added a commit to rust-lang-ci/rust that referenced this pull request

Jul 20, 2024

Rollup merge of rust-lang#127866 - alexcrichton:disable-wasm-component-ld-by-default, r=onur-ozkan

Conditionally build wasm-component-ld

the extended field in config.toml which dist builders use to build a complete set of tools for each host platform.
a "wasm-component-ld" entry in the tools section of config.toml.

Neither of these are enabled by default meaning that most local builds will likely not have this new tool built. Dist builders should still, however, build the tool.

Hmm the condition to build wasm-component-ld seems inconsistent with other tools (cargo etc.). Those tools require both extended=true and tools containing the name to be built, but here wasm-component-ld requires either extended=true or tools containing its name.

IMO we should change it to match other tools (or there's just no way to build another tool without building wasm-component-ld).

alexcrichton added a commit to alexcrichton/rust that referenced this pull request

Sep 6, 2024

It was [pointed out recently][comment] that enabling wasm-component-ld as a host tool is different from other host tools. This commit refactors the logic to match by deduplicating selection of when to build other tools and then using the same logic for wasm-component-ld.

[comment]: rust-lang#127866 (comment)

Good catch, and my bad on the mistake! I've tried to fix this in #130034

workingjubilee added a commit to workingjubilee/rustc that referenced this pull request

Sep 9, 2024