Ensure `./x.py dist` adheres to `build.tools` #87282

pietroalbini · 2021-07-19T14:28:05Z

According to config.toml.example, the way to produce dist artifacts for both the compiler and a subset of tools would be to enable the extended build and manually specify the list of tools to build:

[build]
extended = true
tools = ["cargo", "rustfmt"]

This works as expected for ./x.py build and ./x.py install, but not for ./x.py dist. Before this PR ./x.py dist simply ignored the contents of build.tools, building just rustc/rustdoc if build.extended = false and all of the tools otherwise. This PR does two things:

Changes ./x.py dist extended to only build the tools defined in build.tools, if build.tools is not empty. The rest of the extended step was refactored to simplify the code.
Changes how dist jobs for tools are gated: instead of assert!(builder.config.extended) to prevent tools from being built with build.extended = false, tools are simply built by default depending on build.extended and build.tools. This also enables to explicitly dist tools even with build.extended = false.

This PR is best reviewed commit-by-commit.

Fixes #86436

paulmenzel · 2021-07-23T07:29:49Z

Do you know if there was a regression?

According to issue #86436 (Unable to build 1.53.0 from official tarball due to miri) this also does not work anymore with install.

pietroalbini · 2021-07-23T07:46:14Z

Hmm, this PR doesn't change ./x.py install, only ./x.py dist. From a quick glance at the source code ./x.py install should adhere to build.tools = ["..."], but I don't have much time to investigate that right now :(

src/bootstrap/dist.rs

Mark-Simulacrum · 2021-07-23T12:32:48Z

Overall looks good -- I think the refactor I suggested could be nice, but is also fine to leave out from this PR. I think over the last few months I've started thinking about a new design for the default/should_run system anyway, which would likely address my suggestion in a slightly better way. So unless you're feeling particularly enthusiastic about the possible cleanup, let's leave it out for the time being.

I would like the first comment about tools = [] addressed though, that should be easy.

pietroalbini · 2021-07-23T15:44:42Z

Pushed the refactoring as well, so you can review it. I haven't tested it behaves correctly in all the cases, so let's wait until I do so before merging (reviewing is fine though).

The default is then overridden by `should_run`.

pietroalbini · 2021-07-28T16:12:36Z

Ok fixed a bug I found while testing, the PR should now work as expected. r? @Mark-Simulacrum

Mark-Simulacrum · 2021-08-01T15:54:52Z

src/bootstrap/builder.rs

+
+        // Avoid running steps contained in --exclude
+        for pathset in &should_run.paths {
+            if desc.is_excluded(self, pathset) {


Hm -- this is going to end up printing a bunch of times that things are / are not excluded, which seems a little annoying (i.e., when this is invoked multiple times). Maybe we should make it a ensure-style query that gets its result cached...

Seems OK for this PR though, that printing is noisy anyway.

Mark-Simulacrum · 2021-08-01T15:55:16Z

@bors r+

bors · 2021-08-01T15:55:17Z

📌 Commit 69f712c has been approved by Mark-Simulacrum

Rollup of 13 pull requests Successful merges: - rust-lang#86183 (Change environment variable getters to error recoverably) - rust-lang#86439 (Remove `Ipv4Addr::is_ietf_protocol_assignment`) - rust-lang#86509 (Move `os_str_bytes` to `sys::unix`) - rust-lang#86593 (Partially stabilize `const_slice_first_last`) - rust-lang#86936 (Add documentation for `Ipv6MulticastScope`) - rust-lang#87282 (Ensure `./x.py dist` adheres to `build.tools`) - rust-lang#87468 (Update rustfmt) - rust-lang#87504 (Update mdbook.) - rust-lang#87608 (Remove unused field `Session.system_library_path`) - rust-lang#87629 (Consistent spelling of "adapter" in the standard library) - rust-lang#87633 (Update compiler_builtins to fix i128 shift/mul on thumbv6m) - rust-lang#87644 (Recommend `swap_remove` in `Vec::remove` docs) - rust-lang#87653 (mark a UB doctest as no_run) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup

pietroalbini added 6 commits July 19, 2021 16:14

refactor adding rustc and std into extended builds

8f3844f

refactor gating of dist docs

0c8baa7

change output of dist cargo and clippy to be consistent with other tools

7167f8b

refactor gating of tools

e60b348

refactor gating of demangler

82faa9c

refactor gating of mingw

f4c6d8f

rust-highfive assigned Mark-Simulacrum Jul 19, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 19, 2021

Mark-Simulacrum mentioned this pull request Jul 22, 2021

Unable to build 1.53.0 from official tarball due to miri #86436

Closed

Mark-Simulacrum reviewed Jul 23, 2021

View reviewed changes

src/bootstrap/dist.rs Outdated Show resolved Hide resolved

Mark-Simulacrum reviewed Jul 23, 2021

View reviewed changes

src/bootstrap/dist.rs Outdated Show resolved Hide resolved

Mark-Simulacrum 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 23, 2021

don't build extra tools if build.tools is explicitly an empty list

2cfac3f

This comment has been minimized.

Sign in to view

refactor extended tarball generaton to use the new ensure_if_default

c15e248

pietroalbini force-pushed the refactor-extended branch from f6202a5 to c15e248 Compare July 23, 2021 15:56

pietroalbini 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 23, 2021

set all of the optional tools as DEFAULT = true

69f712c

The default is then overridden by `should_run`.

Mark-Simulacrum reviewed Aug 1, 2021

View reviewed changes

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 Aug 1, 2021

JohnTitor mentioned this pull request Aug 2, 2021

Rollup of 13 pull requests #87689

Merged

bors merged commit 46f01ca into rust-lang:master Aug 2, 2021

rustbot added this to the 1.56.0 milestone Aug 2, 2021

pietroalbini deleted the refactor-extended branch August 12, 2021 06:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ensure `./x.py dist` adheres to `build.tools` #87282

Ensure `./x.py dist` adheres to `build.tools` #87282

pietroalbini commented Jul 19, 2021 •

edited by Mark-Simulacrum

Loading

paulmenzel commented Jul 23, 2021

pietroalbini commented Jul 23, 2021

Mark-Simulacrum commented Jul 23, 2021

pietroalbini commented Jul 23, 2021

This comment has been minimized.

pietroalbini commented Jul 28, 2021

Mark-Simulacrum Aug 1, 2021

Mark-Simulacrum commented Aug 1, 2021

bors commented Aug 1, 2021

Ensure ./x.py dist adheres to build.tools #87282

Ensure ./x.py dist adheres to build.tools #87282

Conversation

pietroalbini commented Jul 19, 2021 • edited by Mark-Simulacrum Loading

paulmenzel commented Jul 23, 2021

pietroalbini commented Jul 23, 2021

Mark-Simulacrum commented Jul 23, 2021

pietroalbini commented Jul 23, 2021

This comment has been minimized.

pietroalbini commented Jul 28, 2021

Mark-Simulacrum Aug 1, 2021

Choose a reason for hiding this comment

Mark-Simulacrum commented Aug 1, 2021

bors commented Aug 1, 2021

Ensure `./x.py dist` adheres to `build.tools` #87282

Ensure `./x.py dist` adheres to `build.tools` #87282

pietroalbini commented Jul 19, 2021 •

edited by Mark-Simulacrum

Loading