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

Override target crate-type for cargo rustc --crate-type #10388

Merged
merged 3 commits into from
Feb 27, 2022
Merged

Override target crate-type for cargo rustc --crate-type #10388

merged 3 commits into from
Feb 27, 2022

Conversation

weihanglo
Copy link
Member

@weihanglo weihanglo commented Feb 13, 2022 •
edited
Loading

What does this PR try to resolve?

Fixes #10356, fixes #10389

This might not be the best solution, but at least it avoids duplicating
overridden rules all over the compilation logic.

r? @ehuss since you're the reviewer of #10093

How should we test and review this PR?

Perform reproducible steps as #10356 described.

This PR includes a new test rustc::build_with_crate_type_for_foo_with_deps
to validate the behavior of overridden crate-types with dependencies.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 13, 2022
@weihanglo weihanglo mentioned this pull request Feb 14, 2022
Closed
@lqd
Copy link
Member

lqd commented Feb 24, 2022 •
edited
Loading

I've tested this PR both on the repro (but that was included in tests already) and on hyper where the issue was discovered, and of course both worked.

As part of the initiative to improve compile times I've also benchmarked it, since the workaround to #10356 is currently preventing cargo's pipelining from working with both hyper's dependencies and the crates depending on it (as one can see e.g. in this cargo timing example).

As a testament to the great work done on cargo's pipelining, my measurements show that building hyper itself with this PR (from -j2 to -j12) results in 7-8% improved build times in debug, and 18-29% in release (only -j2 was at 18%, the mean was 26%) which is super cool (and these wins translate to downstream crates, although of course as a lower percentage of their own schedule, but still sometimes in the same ballpark). So great job @weihanglo on fixing the issue, it will have a big impact on the entire hyper ecosystem, the contributors to the crate itself, as well as CI improvements all around.

@weihanglo @ehuss is there something more I can do to help this PR along ? I can try to build the 800 or so popular crates we've been looking at in our extended benchmarks for example ?

@ehuss
Copy link
Contributor

ehuss commented Feb 25, 2022

Thanks!

@bors r+

@bors
Copy link
Contributor

bors commented Feb 25, 2022

📌 Commit 568b34ebe6f14605ededf6d619fc6821bbdd6a46 has been approved by ehuss

@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 Feb 25, 2022
@bors
Copy link
Contributor

bors commented Feb 25, 2022

⌛ Testing commit 568b34ebe6f14605ededf6d619fc6821bbdd6a46 with merge 496ffaefab260eb540fef5ab20cbd1f463599ce3...

@bors
Copy link
Contributor

bors commented Feb 25, 2022

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Feb 25, 2022
@weihanglo
Copy link
Member Author

@bors retry

@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 Feb 26, 2022
@bors
Copy link
Contributor

bors commented Feb 26, 2022

⌛ Testing commit 568b34ebe6f14605ededf6d619fc6821bbdd6a46 with merge ff397ae463bcea45a5eda8687bf0ffe78ea14416...

@bors
Copy link
Contributor

bors commented Feb 26, 2022

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Feb 26, 2022
@weihanglo
Avoid assertion against default value in crate-type tests
229c48a 
`--crate-type` usually defaults to `lib`, so the original assertion is
somehow unuseful. Change to `cdylib` to make the test more robust.
@weihanglo
New test for cargo rustc --crate-type with dependency
497051a
lqd
lqd reviewed Feb 26, 2022
View reviewed changes
src/cargo/ops/cargo_compile.rs
unit.mode,
unit.features.clone(),
unit.is_std,
unit.dep_hash,
Copy link
Member

Choose a reason for hiding this comment

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

The intern API has recently been updated on master, so something similar to this looks to be needed now

Suggested change
unit.dep_hash,
unit.dep_hash,
unit.artifact,

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks! Just found that and rebased to master

@weihanglo
Override target crate-type for cargo rustc --crate-type
b833643 
Instead of writing override rules all over the compilation logic, this
commit simply override the unit created by `generate_targets`.
As a result, `cargo rustc --crate-type` behaves exactly as expected.
@ehuss
Copy link
Contributor

ehuss commented Feb 27, 2022

@bors r+

@bors
Copy link
Contributor

bors commented Feb 27, 2022

📌 Commit b833643 has been approved by ehuss

@bors bors removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 27, 2022
@bors bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Feb 27, 2022
@bors
Copy link
Contributor

bors commented Feb 27, 2022

⌛ Testing commit b833643 with merge 1e55490...

@bors
Copy link
Contributor

bors commented Feb 27, 2022

☀️ Test successful - checks-actions
Approved by: ehuss
Pushing 1e55490 to master...

@bors bors merged commit 1e55490 into rust-lang:master Feb 27, 2022
@weihanglo weihanglo deleted the issue-10356 branch February 27, 2022 19:21
@lqd
Copy link
Member

lqd commented Feb 27, 2022

Thanks @weihanglo. I have a branch ready to fix hyper and will open a PR once this hits nightly.
cc @seanmonstar

@Michael-F-Bryan Michael-F-Bryan mentioned this pull request Feb 28, 2022
Merged
@jonhoo jonhoo mentioned this pull request Feb 28, 2022
Merged
matthiaskrgr pushed a commit to matthiaskrgr/rust that referenced this pull request Mar 1, 2022
Update cargo
6b476fe 
11 changes in
d6cdde584a1f15ea086bae922e20fd27f7165431..3d6970d50e30e797b8e26b2b9b1bdf92dc381f34
2022-02-22 19:55:51 +0000 to 2022-02-28 19:29:07 +0000:

 - rust-lang/cargo#10395
 - rust-lang/cargo#10425
 - rust-lang/cargo#10428
 - rust-lang/cargo#10388
 - rust-lang/cargo#10167
 - rust-lang/cargo#10429
 - rust-lang/cargo#10426
 - rust-lang/cargo#10372
 - rust-lang/cargo#10420
 - rust-lang/cargo#10416
 - rust-lang/cargo#10417
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Mar 1, 2022
@matthiaskrgr
Rollup merge of rust-lang#94459 - jonhoo:bump-cargo, r=Dylan-DPC
6aab954 
Update cargo

11 changes in
d6cdde584a1f15ea086bae922e20fd27f7165431..3d6970d50e30e797b8e26b2b9b1bdf92dc381f34
2022-02-22 19:55:51 +0000 to 2022-02-28 19:29:07 +0000:

 - rust-lang/cargo#10395
 - rust-lang/cargo#10425
 - rust-lang/cargo#10428
 - rust-lang/cargo#10388
 - rust-lang/cargo#10167
 - rust-lang/cargo#10429
 - rust-lang/cargo#10426
 - rust-lang/cargo#10372
 - rust-lang/cargo#10420
 - rust-lang/cargo#10416
 - rust-lang/cargo#10417
@lqd lqd mentioned this pull request Mar 1, 2022
Merged
@ehuss ehuss added this to the 1.61.0 milestone Apr 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lqd lqd lqd left review comments

Assignees

@ehuss ehuss

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
1.61.0
5 participants
@weihanglo @lqd @ehuss @bors @rust-highfive