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.

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

parallel_compiler: hide dependencies behind feature #93787

Merged
merged 2 commits into from
Mar 28, 2022

Conversation

klensy
Copy link
Contributor

@klensy klensy commented Feb 8, 2022

Separate dependencies for parallel_compiler feature, so they will not be compiled if feature not selected, reducing number of compiled crates from 238 to 224.

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Feb 8, 2022
@rust-highfive
Copy link
Collaborator

r? @wesleywiser

(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 Feb 8, 2022
})
} else {
(FxHashMap::default(), Duration::new(0, 0))
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you make two closures? This would allow to avoid the cfg-if dependency:

#[cfg(parallel_compiler)]
let pre_compile_cgus = |cgu_reuse: &[CguReuse]| { /* ... */ };

#[cfg(not(parallel_compiler))]
let pre_compile_cgus = |_: &[CguReuse]| (FxHashMap::default(), Duration::new(0, 0));

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@klensy
Copy link
Contributor Author

klensy commented Feb 8, 2022

Only thing that depends on rustc-rayon* crates for non parallel_compiler is indexmap. Before #90842 it wasn't used rustc-rayon feature. Should this feature for indexmap be gated for parallel_compiler too?

@klensy
Copy link
Contributor Author

klensy commented Feb 9, 2022

Err, looks like that cfg in cargo don't work?

I'm expecting, that this definition will work, Cargo.toml:

...
[target.'cfg(parallel_compiler)'.dependencies]
rayon = { version = "0.3.2", package = "rustc-rayon" }
rayon-core = { version = "0.3.2", package = "rustc-rayon-core" }
indexmap = { version = "1.8.0", features = ["rustc-rayon"] }

[target.'cfg(not(parallel_compiler))'.dependencies]
indexmap = { version = "1.8.0" }

and compile indexmap with different features, depending on cfg, but it always builded with indexmap = { version = "1.8.0", features = ["rustc-rayon"] }

@cuviper
Copy link
Member

cuviper commented Feb 9, 2022

I don't think you can use cfg that way, since that's not a builtin from rustc --print cfg:
https://doc.rust-lang.org/cargo/reference/specifying-dependencies.html#platform-specific-dependencies

It could be a Cargo feature, something like:

[features]
parallel_compiler = ["indexmap/rustc-rayon", "rayon", "rayon-core"]

[dependencies]
indexmap = "1.8.0"
rayon = { version = "0.3.2", package = "rustc-rayon", optional = true }
rayon-core = { version = "0.3.2", package = "rustc-rayon-core", optional = true }

Plumbing that feature from rustbuild through the crate tree might be challenging, or at least tedious.

@klensy
Copy link
Contributor Author

klensy commented Feb 11, 2022

Looks like that should work, but propagating feature through rustc crates that way not fun at all.

@klensy klensy changed the title parallel_compiler: hide dependencies behind cfg parallel_compiler: hide dependencies behind feature Feb 17, 2022
@bors
Copy link
Contributor

bors commented Feb 25, 2022

☔ The latest upstream changes (presumably #94350) made this pull request unmergeable. Please resolve the merge conflicts.

@JohnCSimon
Copy link
Member

Triage:
@klensy can you please rebase this PR?

Copy link
Member

@wesleywiser wesleywiser left a comment

Choose a reason for hiding this comment

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

Tested locally both with and without parallel-compiler = true and builds succeed.

@wesleywiser
Copy link
Member

Thanks @klensy!

@bors r+

@bors
Copy link
Contributor

bors commented Mar 28, 2022

📌 Commit 008fc79 has been approved by wesleywiser

@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 Mar 28, 2022
@bors
Copy link
Contributor

bors commented Mar 28, 2022

⌛ Testing commit 008fc79 with merge 6809057bc8259f310e1eb6d51d81018dab5010fc...

@Dylan-DPC
Copy link
Member

@bors retry (yield to rollup containing this pr )

bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 28, 2022
Rollup of 5 pull requests

Successful merges:

 - rust-lang#93787 (parallel_compiler: hide dependencies behind feature)
 - rust-lang#95318 (diagnostics: correct generic bounds with doubled colon)
 - rust-lang#95328 (Fix yet another Box<T, A> ICE)
 - rust-lang#95397 (Link to std::io's platform-specific behavior disclaimer)
 - rust-lang#95407 (Inline u8::is_utf8_char_boundary)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@rust-log-analyzer
Copy link
Collaborator

A job failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

@bors
Copy link
Contributor

bors commented Mar 28, 2022

⌛ Testing commit 008fc79 with merge ee915c3...

@bors bors merged commit 72770ef into rust-lang:master Mar 28, 2022
@rustbot rustbot added this to the 1.61.0 milestone Mar 28, 2022
@klensy klensy deleted the really-not-a-features branch March 29, 2022 11:32
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. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants