Skip to content

Do not build tools if user do not want them #71346

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

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Apr 21, 2020

Conversation

mati865
Copy link
Contributor

@mati865 mati865 commented Apr 20, 2020

Fixes #71307

@rust-highfive
Copy link
Contributor

r? @Mark-Simulacrum

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 20, 2020
@Mark-Simulacrum
Copy link
Member

@bors r+

Thanks!

@bors
Copy link
Collaborator

bors commented Apr 20, 2020

📌 Commit 9296d3b has been approved by Mark-Simulacrum

@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 Apr 20, 2020
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 21, 2020
Rollup of 4 pull requests

Successful merges:

 - rust-lang#69362 (Stabilize most common subset of alloc_layout_extras)
 - rust-lang#71174 (Check that main/start is not async)
 - rust-lang#71285 (MIR: use HirId instead of NodeId to avoid cycles while inlining)
 - rust-lang#71346 (Do not build tools if user do not want them)

Failed merges:

r? @ghost
@bors bors merged commit 9a0e702 into rust-lang:master Apr 21, 2020
@mati865 mati865 deleted the rustbuild-tools branch April 21, 2020 07:08
@tshepang
Copy link
Member

tshepang commented Apr 22, 2020

I see cargo is getting built, even though I asked only for rustfmt

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

@tshepang
Copy link
Member

Ok, I suppose that was because cargo-fmt also needed to get built

@mati865
Copy link
Contributor Author

mati865 commented Apr 22, 2020

@tshepang I'm on my mobile right now but I think there was piece of code somewhere that always enables Cargo when extended = true.

@tshepang
Copy link
Member

tshepang commented Apr 22, 2020

@mati865
Copy link
Contributor Author

mati865 commented Apr 22, 2020

@tshepang exactly. I don't know whether it's fine to skip Cargo when bulidng other tools though.

@tshepang
Copy link
Member

I would rather have a warning, or at least the behavior documented (in config.toml.example), than be surprised. Perhaps I should prepare a PR?

@mati865
Copy link
Contributor Author

mati865 commented Apr 22, 2020

@tshepang sure but maybe this condition should be changed from

run.path("src/tools/cargo").default_condition(builder.config.extended)

to

run.path("src/tools/cargo").default_condition(
    builder.config.extended
    && builder.config.tools.as_ref().map_or(true, |t| t.contains("cargo")))

since Cargo won't be installed if it's not present in tools list?

@tshepang
Copy link
Member

That makes me wonder why cargo is not made part of that tools_extended macro... code looks similar?

@mati865
Copy link
Contributor Author

mati865 commented Apr 23, 2020

I have no idea, probably because of this line:

.expect("expected to build -- essential tool")

@tshepang
Copy link
Member

@Mark-Simulacrum why is cargo marked as a essential tool?

@Mark-Simulacrum
Copy link
Member

We can't not ship a cargo, since essentially that just means a broken nightly (or whatever toolchain). It might be that there's a better way of doing it in the code though, I'm happy to review PRs and such changing the terminology or specific implementation details so long as we keep the property that our dist builders all build cargo and error if they're not able to.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

failing to build individual tools via x.py
5 participants