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

Break ckb-launcher and ckb-chain's cycle dependency by moving SharedPackage and SharedBuilder from ckb-launcher to ckb-shared #4236

Merged

Conversation

eval-exec
Copy link
Collaborator

@eval-exec eval-exec commented Nov 17, 2023

What problem does this PR solve?

This PR is based on #4235

In the develop branch, ckb-chain's unit test need ckb-launcher, and ckb-launcher need ckb-chain, this PR want to move SharedPackage and SharedBuilder from ckb-launcher to ckb-shared, then ckb-chain won't need ckb-launcher anymore, this will break the cycle dependencies:

before:

flowchart TD
subgraph ckb-workspace
ckb-launcher
ckb-chain
ckb-shared
end

    subgraph ckb-launcher
       Sp[SharedPackage]
       Sb[SharedBuilder]
       Launcher
    end
    subgraph ckb-shared
        Shared
    end
    subgraph ckb-chain
        ChainController
        ChainService
    end 
    ckb-launcher --> ckb-shared
    ckb-launcher --> ckb-chain

    ckb-chain --> ckb-shared 
    ckb-chain -.->|cycle deps| ckb-launcher

Loading

After this PR:

flowchart TD
subgraph ckb-workspace
ckb-launcher
ckb-chain
ckb-shared
end


    subgraph ckb-launcher
        Launcher
    end
    subgraph ckb-shared
        Shared
        Sp[SharedPackage]
        Sb[SharedBuilder]
    end
    subgraph ckb-chain
        ChainController
        ChainService
    end 
    ckb-launcher --> ckb-chain
    ckb-launcher --> ckb-shared
    ckb-chain --> ckb-shared 
Loading

Related changes

  • PR to update owner/repo:
  • Need to cherry-pick to the release branch

Check List

Tests

  • Unit test
  • Integration test
  • Manual test (add detailed scripts or steps below)
  • No code ci-runs-only: [ quick_checks,linters ]

Side effects

  • Performance regression
  • Breaking backward compatibility

Release note

Title Only: Include only the PR title in the release note.

Signed-off-by: Eval EXEC <execvy@gmail.com>
Signed-off-by: Eval EXEC <execvy@gmail.com>
@eval-exec eval-exec requested a review from a team as a code owner November 17, 2023 12:44
@eval-exec eval-exec requested review from quake and removed request for a team November 17, 2023 12:44
@eval-exec eval-exec marked this pull request as draft November 17, 2023 12:55
@eval-exec eval-exec marked this pull request as ready for review November 19, 2023 00:13
@eval-exec eval-exec changed the title Break ckb-launcher and ckb-chain's cycle dependency Break ckb-launcher and ckb-chain's cycle dependency, move SharedPackage and SharedBuilder from ckb-launcher to ckb-shared Nov 20, 2023
@eval-exec eval-exec changed the title Break ckb-launcher and ckb-chain's cycle dependency, move SharedPackage and SharedBuilder from ckb-launcher to ckb-shared Break ckb-launcher and ckb-chain's cycle dependency by moving SharedPackage and SharedBuilder from ckb-launcher to ckb-shared Nov 20, 2023
benches/benches/benchmarks/resolve.rs Outdated Show resolved Hide resolved
@eval-exec eval-exec added the dependencies Pull requests that update a dependency file label Nov 20, 2023
@quake quake added this pull request to the merge queue Nov 21, 2023
Merged via the queue into nervosnetwork:develop with commit 49e6325 Nov 21, 2023
32 checks passed
@doitian doitian mentioned this pull request Dec 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

2 participants