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

Make cargo a workspace #109133

Merged
merged 6 commits into from
Apr 16, 2023
Merged

Make cargo a workspace #109133

merged 6 commits into from
Apr 16, 2023

Conversation

weihanglo
Copy link
Member

@weihanglo weihanglo commented Mar 14, 2023

8 commits in 7bf43f028ba5eb1f4d70d271c2546c38512c9875..39116ccc9b420a883a98a960f0597f9cf87414b8
2023-04-10 16:01:41 +0000 to 2023-04-15 20:24:15 +0000


What does this PR try to resolve?

Making cargo a workspace.

Why doing this?

  • rustc-workspace-hack is primarily for sharing dependencies between rls and cargo, as rls previously depends on cargo. After rls retired, it is no longer the case sharing dependencies.
  • It's q bit painful that cargo needs to deal with some dependency and licensing complexities. For example, Update cargo #108665 failed because of the interaction bewteen windows-sys and raw-dylib. It currenctly blocks cargo's feature -Zgitxodie from moving forward.
  • See Make cargo a workspace cargo#11851

Benchmark result

I've done a simple benchmark on both keeping or removing entire rustc-workspace-hack. It had no significant difference. Both took ~2m30s to finish ./x.py build -j8 src/tools/cargo src/tools/rls src/tools/clippy src/tools/miri src/tools/rustfmt. Environment info:

host: aarch64-apple-darwin
os: Mac OS 13.2.1 [64-bit]

A sophisticated benchmark may be needed.

Additional information

This depends on prior works from @Muscraft and @ehuss. Credits to them!

@rustbot rustbot added A-testsuite Area: The testsuite used to check the correctness of rustc 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 Mar 14, 2023
@albertlarsan68
Copy link
Member

Does this means that cargo will have its own Cargo.lock?

.gitmodules Outdated Show resolved Hide resolved
@weihanglo
Copy link
Member Author

@bors try

@bors
Copy link
Contributor

bors commented Mar 14, 2023

⌛ Trying commit d4724e2fc45d91b6c6ccd6ce4c8ab6a4b05212bb with merge ffd9aff2c26946dbc701cdc41e080c17cc40789b...

@weihanglo
Copy link
Member Author

Does this means that cargo will have its own Cargo.lock?

Exactly!

@bors
Copy link
Contributor

bors commented Mar 14, 2023

💔 Test failed - checks-actions

@bors 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-review Status: Awaiting review from the assignee but also interested parties. labels Mar 14, 2023
@weihanglo weihanglo force-pushed the make-cargo-a-workspace branch from bd24787 to 5c7b56b Compare March 15, 2023 00:06
@weihanglo
Copy link
Member Author

Failed on openssl build. I guess it's rustc-workspace-hack including some curl-sys features causes the issue.

curl-sys = { version = "0.4.13", features = ["http2", "libnghttp2-sys"], optional = true }

I've made a change in cargo to include curl/http2 in all-static feature.

Let's @bors try again.

@bors
Copy link
Contributor

bors commented Mar 15, 2023

⌛ Trying commit 5c7b56bde2f25a99b03bdc1d525b0d0c55afd5dc with merge 9fa7021716eaa8f3cfccf9eb2c8dfaa209f8ee8e...

@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Contributor

bors commented Mar 15, 2023

💔 Test failed - checks-actions

@rust-log-analyzer

This comment has been minimized.

@weihanglo weihanglo force-pushed the make-cargo-a-workspace branch from 5c7b56b to 9a7e8a1 Compare March 15, 2023 13:56
@weihanglo
Copy link
Member Author

@bors try

@bors
Copy link
Contributor

bors commented Mar 15, 2023

⌛ Trying commit 9a7e8a1 with merge 8ea09f172814926c4c234f649b199f6aa9205307...

@bors
Copy link
Contributor

bors commented Mar 15, 2023

💔 Test failed - checks-actions

@weihanglo

This comment was marked as outdated.

@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Contributor

bors commented Mar 15, 2023

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

@weihanglo weihanglo force-pushed the make-cargo-a-workspace branch 2 times, most recently from b34783c to a6c6912 Compare March 17, 2023 13:10
It is unnecessary to stream cargo JSON output.
@weihanglo weihanglo force-pushed the make-cargo-a-workspace branch from 4081b34 to 103ed0e Compare April 16, 2023 18:32
@weihanglo
Copy link
Member Author

openssl dependency now becomes a platform specific dependency of Cargo. Let's try again.

@bors r=ehuss rollup=never p=1

@bors
Copy link
Contributor

bors commented Apr 16, 2023

📌 Commit 103ed0e has been approved by ehuss

It is now in the queue for this repository.

@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 16, 2023
@bors
Copy link
Contributor

bors commented Apr 16, 2023

⌛ Testing commit 103ed0e with merge d0f204e...

@bors
Copy link
Contributor

bors commented Apr 16, 2023

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

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (d0f204e): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
2.0% [2.0%, 2.0%] 1
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-2.0% [-2.5%, -1.2%] 3
All ❌✅ (primary) - - 0

Cycles

This benchmark run did not return any relevant results for this metric.

bors added a commit to rust-lang/rust-clippy that referenced this pull request May 30, 2023
deps: drop serde feature from url, drop rustc-workspace-hack

Cargo now have it's own workspace and rustc dropped [`rustc-workspace-hack`](rust-lang/rust#109133), so no need to unify features here; drop rustc-workspace-hack.

changelog: none
@RalfJung
Copy link
Member

RalfJung commented Jun 4, 2023

So Cargo now is a separate workspace from the main rustc workspace? It was already in a workspace before, after all (so I am confused by the PR description). Doesn't that mean https://github.com/rust-lang/rust/blob/master/src/etc/rust_analyzer_settings.json needs updating? Cc @jyn514

bors added a commit to rust-lang/miri that referenced this pull request Jun 4, 2023
Remove rustc-workspace-hack

The `rustc-workspace-hack` dependency was removed in rust-lang/rust#109133 and should no longer be needed.
@albertlarsan68
Copy link
Member

I do not think that this is needed, since not many people will do cargo development in the Rustc repo, and the needed dependencies (if any) should already be set as path deps.

@ehuss
Copy link
Contributor

ehuss commented Jun 4, 2023

Sorry, I'm not familiar with rust-analyzer settings, so I don't know.

@jyn514
Copy link
Member

jyn514 commented Jun 4, 2023

@RalfJung #111998

@jyn514
Copy link
Member

jyn514 commented Jun 4, 2023

ah, looks like i missed cargo there - happy to take a PR adding it

RalfJung pushed a commit to RalfJung/rust that referenced this pull request Jun 29, 2023
Remove rustc-workspace-hack

The `rustc-workspace-hack` dependency was removed in rust-lang#109133 and should no longer be needed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-testsuite Area: The testsuite used to check the correctness of rustc merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants