Handle dev-dependency cycles #14475

Veykril · 2023-04-03T16:25:07Z

This should fix cycles errors mostly (it fixes the one on rome/tools at least, but not on rustc. Though there it might just be because of rustc workspace being rustc workspace). Unfortunately this will effectively duplicate all crates currently, since if we want to be completely correct we'd need to set the test cfg for all dev dependencies and the transitive dependencies of those, something I worry we should try to avoid.

davidbarsky · 2023-04-04T18:01:00Z

crates/project-model/src/workspace.rs

+ if cfg!(test) {
+ panic!("{}", err);
+ }
+ tracing::error!("{}", err);


i've seen a bunch of usage of format strings, but you can reduce this to:

Suggested change

tracing::error!("{}", err);

tracing::error!(?err);

davidbarsky · 2023-04-04T18:15:19Z

Unfortunately this will effectively duplicate all crates currently, since if we want to be completely correct we'd need to set the test cfg for all dev dependencies and the transitive dependencies of those, something I worry we should try to avoid.

Naive question: is this for only crates that are in the immediate workspace? naively, that feels like a reasonable and fine approach.

Veykril · 2023-04-04T18:42:48Z

The way its implemented right now is for all dev-dependencies (declared by workspace members, as dev-dependencies of libraries are irrelevant to the workspace) we will duplicate all their normal (non-build, non-dev) dependencies with the test flags. This would be completely correct as a dev depedency could re-export something from their normal dependencies that depends on the test cfg being set. but this also means as soon as you have a cycle (i.e a dev-dependency on a workspace member) you basically duplicate (almost) the entire crate graph.

An alternative heuristic way would be to only duplicate dependencies in dev-dependency that point to workspace members and use the "non dev-version" for the rest. This will most likely cover the majority of cases.

bors · 2023-04-05T14:31:19Z

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

davidbarsky · 2023-04-05T14:40:31Z

The way its implemented right now is for all dev-dependencies (declared by workspace members, as dev-dependencies of libraries are irrelevant to the workspace) we will duplicate all their normal (non-build, non-dev) dependencies with the test flags. This would be completely correct as a dev depedency could re-export something from their normal dependencies that depends on the test cfg being set. but this also means as soon as you have a cycle (i.e a dev-dependency on a workspace member) you basically duplicate (almost) the entire crate graph.

Gotcha, thanks for explaining :)

winksaville · 2023-04-08T02:48:50Z

@Veykril pointed me at this issue from a post I made at users.rust-lang. I've built RA from source and have it running on vscode and I'd like to see if this change fixes the problem I'm seeing. I'll give it a try if someone would kindly fix the merge conflicts :)

Veykril · 2023-04-13T14:12:12Z

Okay, I've changed it to now only duplicate workspace members. This cuts down the cyclic deps errors in the rust-lang/rust workspace from 50 to 23 (I believe the 23 come from the dependency on the rustc workspace itself which is already handled a bit incorrectly).

Veykril · 2023-04-13T14:37:07Z

Unfortunately this manifests a problem now, given we have two crates for some files we will basically duplicate some things in IDE features ... (symbol search etc), I am not sure how to best deal with that

Veykril · 2023-04-13T14:50:34Z

Okay, actually we fortunately ignore anything but the first module we find for a file so most things should work if we prefer the duplicated crates. (symbol search and so on will still dupe, but I guess that is the price people will have to pay for now for adding a dev-dependency like that)

Let's merge this next monday

winksaville · 2023-04-14T22:07:35Z

FYI, I tested my workspace and there are no longer any errors. Txs!!!!

bors · 2023-04-20T22:41:07Z

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

chenyukang · 2023-04-21T16:54:58Z

I verified it in my local, it works!!

Veykril · 2023-04-25T08:58:19Z

@bors r+

bors · 2023-04-25T08:58:21Z

📌 Commit b4345e9 has been approved by Veykril

It is now in the queue for this repository.

bors · 2023-04-25T09:00:49Z

⌛ Testing commit b4345e9 with merge 2cbed3d...

Handle dev-dependency cycles cc #14167 This should fix cycles errors mostly (it fixes the one on rome/tools at least, but not on rustc. Though there it might just be because of rustc workspace being rustc workspace). Unfortunately this will effectively duplicate all crates currently, since if we want to be completely correct we'd need to set the test cfg for all dev dependencies and the transitive dependencies of those, something I worry we should try to avoid.

bors · 2023-04-25T09:09:28Z

💔 Test failed - checks-actions

Veykril · 2023-04-25T10:07:19Z

@bors r+

bors · 2023-04-25T10:07:21Z

📌 Commit e205af2 has been approved by Veykril

It is now in the queue for this repository.

bors · 2023-04-25T10:07:27Z

⌛ Testing commit e205af2 with merge a96bb45...

bors · 2023-04-25T10:24:43Z

☀️ Test successful - checks-actions
Approved by: Veykril
Pushing a96bb45 to master...

Veykril · 2023-04-25T11:48:18Z

Something went terribly wrong with the rebase it seems, the metrics don't look good...

Revert "Handle dev-dependency cycles" Reverts #14475 I managed to fix the problem that causes the metric failures, but this still severely degrades IDE features in a way that makes the user experience ***worse*** than before. I am not sure how to address this, I wonder what the jetbrains plugin is doing in these cases.

chenyukang · 2023-04-25T12:35:57Z

Yes, this PR fixed cycle deps issues, but seems has a bug.
From my daily usage, functions from other crates are not working now, before this PR:

After merging this PR:

The hover tip for async_trait, extract_peer_id.etc also don't work.

Repo: https://github.com/nervosnetwork/ckb FYI.

Veykril · 2023-04-25T12:38:37Z

Yes, this PR in general causes a bunch of issues which are inherent to the fact that we now have a bunch of items come from two different crates that are semantically the same which breaks features everywhere. The approach in this PR doesn't seem to work out so it will be reverted.

rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 3, 2023

Veykril changed the title ~~Add more complex project-model test~~ Handle dev-dependency cycles Apr 3, 2023

Veykril force-pushed the crate-origins branch from cb752ee to e000c52 Compare April 3, 2023 16:29

davidbarsky reviewed Apr 4, 2023

View reviewed changes

Veykril mentioned this pull request Apr 11, 2023

Deduplicate crates in crate graph #14476

Closed

Veykril force-pushed the crate-origins branch 2 times, most recently from 1fa6d5f to 56de543 Compare April 13, 2023 14:11

Veykril marked this pull request as ready for review April 13, 2023 14:37

Veykril force-pushed the crate-origins branch from fc503b8 to 4d5b7b1 Compare April 13, 2023 14:49

Veykril added 2 commits April 25, 2023 10:49

Add more complex project-model test

980c75b

Handle dev-dependency cycles

a4c7a87

Veykril force-pushed the crate-origins branch from 4d5b7b1 to b4345e9 Compare April 25, 2023 08:57

Prefer test duped crates for ide features

e205af2

Veykril force-pushed the crate-origins branch from b4345e9 to e205af2 Compare April 25, 2023 09:40

bors merged commit a96bb45 into rust-lang:master Apr 25, 2023

Veykril deleted the crate-origins branch April 25, 2023 10:50

Veykril mentioned this pull request Apr 25, 2023

Revert "Handle dev-dependency cycles" #14657

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle dev-dependency cycles #14475

Handle dev-dependency cycles #14475

Veykril commented Apr 3, 2023 •

edited

Loading

davidbarsky Apr 4, 2023

davidbarsky commented Apr 4, 2023

Veykril commented Apr 4, 2023

bors commented Apr 5, 2023

davidbarsky commented Apr 5, 2023

winksaville commented Apr 8, 2023

Veykril commented Apr 13, 2023

Veykril commented Apr 13, 2023

Veykril commented Apr 13, 2023 •

edited

Loading

winksaville commented Apr 14, 2023 •

edited

Loading

bors commented Apr 20, 2023

chenyukang commented Apr 21, 2023

Veykril commented Apr 25, 2023

bors commented Apr 25, 2023

bors commented Apr 25, 2023

bors commented Apr 25, 2023

Veykril commented Apr 25, 2023

bors commented Apr 25, 2023

bors commented Apr 25, 2023

bors commented Apr 25, 2023

Veykril commented Apr 25, 2023 •

edited

Loading

chenyukang commented Apr 25, 2023

Veykril commented Apr 25, 2023

Handle dev-dependency cycles #14475

Handle dev-dependency cycles #14475

Conversation

Veykril commented Apr 3, 2023 • edited Loading

davidbarsky Apr 4, 2023

Choose a reason for hiding this comment

davidbarsky commented Apr 4, 2023

Veykril commented Apr 4, 2023

bors commented Apr 5, 2023

davidbarsky commented Apr 5, 2023

winksaville commented Apr 8, 2023

Veykril commented Apr 13, 2023

Veykril commented Apr 13, 2023

Veykril commented Apr 13, 2023 • edited Loading

winksaville commented Apr 14, 2023 • edited Loading

bors commented Apr 20, 2023

chenyukang commented Apr 21, 2023

Veykril commented Apr 25, 2023

bors commented Apr 25, 2023

bors commented Apr 25, 2023

bors commented Apr 25, 2023

Veykril commented Apr 25, 2023

bors commented Apr 25, 2023

bors commented Apr 25, 2023

bors commented Apr 25, 2023

Veykril commented Apr 25, 2023 • edited Loading

chenyukang commented Apr 25, 2023

Veykril commented Apr 25, 2023

Veykril commented Apr 3, 2023 •

edited

Loading

Veykril commented Apr 13, 2023 •

edited

Loading

winksaville commented Apr 14, 2023 •

edited

Loading

Veykril commented Apr 25, 2023 •

edited

Loading