-
Notifications
You must be signed in to change notification settings - Fork 236
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
chore: remove deps cycle #3929
chore: remove deps cycle #3929
Conversation
aba2d2c
to
edae6a8
Compare
@@ -59,7 +59,7 @@ case $GITHUB_WORKFLOW in | |||
make check-whitespaces | |||
make check-dirty-rpc-doc | |||
make check-dirty-hashes-toml | |||
devtools/ci/check-cyclic-dependencies.py | |||
devtools/ci/check-cyclic-dependencies.py --dev |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we didn't check the exit code of this script, should we fix it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes it should be fixed
edae6a8
to
3048cc2
Compare
@@ -36,7 +36,6 @@ ckb-reward-calculator = { path = "../util/reward-calculator", version = "= 0.110 | |||
ckb-tx-pool = { path = "../tx-pool", version = "= 0.110.0-pre", features = ["internal"] } | |||
ckb-jsonrpc-types = { path = "../util/jsonrpc-types", version = "= 0.110.0-pre" } | |||
ckb-network = { path = "../network", version = "= 0.110.0-pre" } | |||
ckb-launcher = { path = "../util/launcher", version = "= 0.110.0-pre" } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we should remove ckb-launcher
from [dev-dependencies]
, because there are some unit tests in ckb-chain
need ckb-launcher
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the deps cyclic is related to dev-dependencies only, some unit test will failed if you remove it. Could you try the unit test on your local env to verify the result?
Yes, I can reproduce it in my local env. |
According to rust-lang/rust-analyzer#14167 |
What problem does this PR solve?
Remove deps cycle in Cargo.toml.
Problem Summary:
rust-analyzer
is blaming for it:And if we remove the deps cycle, we could use
cargo deps
to visualize deps graph.devtools/ci/check-cyclic-dependencies.py
in CI is not working, do we need to enable it to check the dep order name?Check List
Tests
Side effects
Release note