package dependency graph has cycles

[turadg]

Describe the bug

Running a Lerna command warns,

lerna notice cli v3.20.2
lerna WARN ECYCLE Dependency cycles detected, you should fix these!
lerna WARN ECYCLE @agoric/swingset-vat -> @agoric/notifier -> @agoric/swingset-vat
lerna WARN ECYCLE @agoric/deploy-script-support -> @agoric/vats -> @agoric/pegasus -> @agoric/deploy-script-support
lerna WARN ECYCLE @agoric/governance -> @agoric/zoe -> @agoric/governance
lerna WARN ECYCLE @agoric/run-protocol -> (nested cycle: @agoric/deploy-script-support -> @agoric/vats -> @agoric/pegasus -> @agoric/deploy-script-support) -> @agoric/run-protocol
lerna WARN ECYCLE @agoric/store -> (nested cycle: @agoric/swingset-vat -> @agoric/notifier -> @agoric/swingset-vat) -> @agoric/store
lerna WARN ECYCLE @agoric/swing-store -> @agoric/swing-store
lerna WARN ECYCLE @agoric/wallet-backend -> (nested cycle: @agoric/run-protocol -> (nested cycle: @agoric/deploy-script-support -> @agoric/vats -> @agoric/pegasus -> @agoric/deploy-script-support) -> @agoric/run-protocol) -> @agoric/wallet-backend
lerna WARN ECYCLE (nested cycle: @agoric/store -> (nested cycle: @agoric/swingset-vat -> @agoric/notifier -> @agoric/swingset-vat) -> @agoric/store) -> (nested cycle: @agoric/store -> (nested cycle: @agoric/swingset-vat -> @agoric/notifier -> @agoric/swingset-vat) -> @agoric/store)
lerna WARN ECYCLE (nested cycle: @agoric/wallet-backend -> (nested cycle: @agoric/run-protocol -> (nested cycle: @agoric/deploy-script-support -> @agoric/vats -> @agoric/pegasus -> @agoric/deploy-script-support) -> @agoric/run-protocol) -> @agoric/wallet-backend) -> (nested cycle: @agoric/wallet-backend -> (nested cycle: @agoric/run-protocol -> (nested cycle: @agoric/deploy-script-support -> @agoric/vats -> @agoric/pegasus -> @agoric/deploy-script-support) -> @agoric/run-protocol) -> @agoric/wallet-backend)

To Reproduce

Steps to reproduce the behavior:

1. yarn lerna --reject-cycles run lint

Expected behavior

No cyclic dependencies among packages.

Prioritization Analysis

This problem is not catastrophic nor urgent. Here are the reasons to do it anyway:

1. Friction in development

The cycles in the dependency graph muddy the code architecture. It's not always clear where to put something and we end up moving things around as the boundaries become apparent. This suggests a lack of overall architectural clarity. We also sometimes hack around the irrational boundaries, such as #5232

2. Prevents CI optimization of skipping unchanged packages

We can speed up CI and pre-push testing by only examining what has changed. For example, lerna run test --since=main will only run tests on packages that have changed since master. With cycles in the dependency graph it infers everything changed.

This is less relevant in CI where we run everything in parallel. We waste work but we don't waste wall time except for jobs like deployment-test which is very long and could automatically be skipped when it's known not to be affected.

3. Complicates semver

When concerns are mixed in a package, a breaking change to one part implies a breaking change to the whole package.

4. Larger bundles

Because the Endo bundler don't yet have tree shaking, the lax package factoring can cause unnecessarily large bundles.

5. Unnecessary version bumps

Lerna bumps a version when its dependency bumps.

Tasks

Beta Give feedback

1. eliminate cycle between swingset-vat and vat-data #8088
   +0
   

[erights]

Is this looking at just dependencies: or is it also looking at devDependencies:?

IMO we should avoid cyclic dependencies: per se. But when taking devDependencies: into account as well, cycles are fine.

[turadg]

Is this looking at just dependencies: or is it also looking at devDependencies:?

Good question. Here's an analysis of what's left after #4647

• @agoric/swingset-vat -> @agoric/store -> @agoric/swingset-vat
  • swingset deep imports storeModule that should leave this package
  • swingset imports makeStore
  • store imports swingset only for its prepare-test-env-ava.js
• @agoric/pegasus -> @agoric/vats -> @agoric/pegasus
  • vats imports pegasus for the pegasusBundle and its install-on-chain utilities
  • pegasus imports vats for types
• @agoric/run-protocol -> (nested cycle: @agoric/pegasus -> @agoric/vats -> @agoric/pegasus) -> @agoric/run-protocol
  • run-protocol imports vats for CENTRAL_ISSUER_NAME ("RUN"), some non-vat utilities and some types.
  • vats imports run-protocol for some generic utilities (collect.js) and econ behaviors
• @agoric/wallet-backend -> (nested cycle: @agoric/run-protocol -> (nested cycle: @agoric/pegasus -> @agoric/vats -> @agoric/pegasus) -> @agoric/run-protocol) -> @agoric/wallet-backend
  • skipping
• (nested cycle: @agoric/wallet-backend -> (nested cycle: @agoric/run-protocol -> (nested cycle: @agoric/pegasus -> @agoric/vats -> @agoric/pegasus) -> @agoric/run-protocol) -> @agoric/wallet-backend) -> (nested cycle: @agoric/wallet-backend -> (nested cycle: @agoric/run-protocol -> (nested cycle: @agoric/pegasus -> @agoric/vats -> @agoric/pegasus) -> @agoric/run-protocol) -> @agoric/wallet-backend)
  • skipping

IMO we should avoid cyclic dependencies: per se. But when taking devDependencies: into account as well, cycles are fine.

Agree with a qualification. Cycles in devDependencies need not to forbidden, but can often be a smell. For example, the generic utils like collect.js living in @agoric/run-protocol.

There's also the pragmatic consideration of the tools we have available, which don't distinguish between dependency types: lerna/lerna#1198