Improve error messages from cyclical references

[mpodwysocki]

Verify canary release

• I verified that the issue exists in the latest Turborepo canary release.

Link to code that reproduces this issue

https://github.com/Azure/azure-sdk-for-js/tree/feat/pnpm-migration

What package manager are you using / does the bug impact?

pnpm

What operating system are you using?

Mac

Which canary version will you have in your reproduction?

2.1.3

Describe the Bug

When trying to use turbo when trying to move the Azure SDK for JS to the build system, we get an unhelpful error message

  × Invalid package dependency graph: cyclic dependency detected:
  │ 	@azure/core-xml, @azure/logger, @azure/core-util, @azure/core-auth, @azure/core-tracing,
  │ @azure/core-rest-pipeline, @azure/core-client, @azure-tools/test-recorder, @azure/core-paging,
  │ @azure-tools/test-utils-vitest, @azure/core-http-compat, @azure-tools/test-credential, @azure/
  │ keyvault-common, @azure/keyvault-keys, @azure-tools/test-utils, @azure/identity, @azure/dev-
  │ tool, @azure/abort-controller
  ╰─▶ cyclic dependency detected:
      	@azure/core-xml, @azure/logger, @azure/core-util, @azure/core-auth, @azure/core-tracing,
      @azure/core-rest-pipeline, @azure/core-client, @azure-tools/test-recorder, @azure/core-
      paging, @azure-tools/test-utils-vitest, @azure/core-http-compat, @azure-tools/test-
      credential, @azure/keyvault-common, @azure/keyvault-keys, @azure-tools/test-utils, @azure/
      identity, @azure/dev-tool, @azure/abort-controller

And digging into the code, there are many instances that seem unhelpful as @azure/abort-controller, @azure/core-xml etc do not have circular references.

Expected Behavior

Other systems such as Lage give us the following error message which is much more helpful

lage - Version 2.11.6 - 8 Workers
[14:16:48] - error     Error: Cycles detected in the target graph: @azure/keyvault-keys#build -> @azure/identity#build -> @azure-tools/test-credential#build -> @azure/keyvault-keys#build

To Reproduce

Running turbo build

Additional context

No response

[chris-olszewski]

Thank you for the issue, just adding some commentary:

And digging into the code, there are many instances that seem unhelpful as @azure/abort-controller, @azure/core-xml etc do not have circular references.

I believe that the error message is displaying a cycle in the graph, it is just the largest possible cycle. It is listing nodes that are all reachable from each other (a strongly connected component).

Lage is using a DFS to eagerly return the first cycle it encounters so if you cut the @azure-tools/test-credential#build -> @azure/keyvault-keys#build edge, there would still be a cycle in the graph, just a different one.

I am in agreement that just listing the largest set of nodes that is cyclic isn't very actionable, but I also don't want to omit the full picture. One could invest in cutting an edge expecting it to remove the cycle only to discover that there's another cycle that wasn't reported.

The solution here might be for us to invest in a package graph level visualization as that would give the complete picture.