search

dart-lang / sdk

The Dart SDK, including the VM, JS and Wasm compilers, analysis, core libraries, and more.
https://dart.dev
BSD 3-Clause "New" or "Revised" License
10.24k stars 1.57k forks source link

☂️ Implement Workspace Feature #53875

Open jacob314 opened 1 year ago

jacob314 commented 1 year ago

Workspace feature

Description

Support shared resolution between packages of a workspace / mono-repo. Workspaces are tightly related packages that are developed and released together.

We have seen the analysis server consume large memory such that it is rendered unusable mainly due to the lack of a workspace feature. https://github.com/dart-lang/sdk/issues/52447 https://github.com/dart-lang/sdk/issues/41793

Work items

mraleph commented 1 year ago

@jacob314 @keertip Thanks for filing issue to track this work. Could we also produce some sort of overall design document that outlines our plans and their implications - so that we could circulate it in the community and collect feedback?

@incendial privately provided me some feedback that makes me a bit concerned: he is aware of monorepos which are not normalized with respect to external dependencies. That is monorepo might contain multiple entry point packages (e.g. different apps) and these entry point packages have inconsistent version resolutions when it comes to the external packages. This is similar to the feedback provided here https://github.com/dart-lang/pub/issues/4022#issuecomment-1733945174 by @olexale. Right now we are operating under the assumption that external monorepos either already operate like Google's internal monorepo (following one version rule) or can be easily brought under the same constraints. I wonder if we should pause before plunging here and validate our assumption? I wonder if pub team (cc @sigurdm @jonasfj) could create a tool which takes a path to monorepository root and computes the global resolution for all pubspec.yaml files found - and reports if such global resolution is possible or not. We could then ask prominent monorepository users to try running such tool and evaluate what percentage of users could easily adopt monorepos and benefit from our work here. If we discover that our assumption does not hold we might need to go back to the drawing board.

keertip commented 1 year ago

Part of the support for monorepo in the analysis server is based on the pub proposal #4022. The key points of that proposal that would make the analysis server treat the codebase as a single workspace/monorepo and use one version of dependencies for resolution are

As mentioned in the pub proposal, there is a way out if for some reason a package's dependencies are unable to meet the global constraints. It can opt out of the resolution: external, and then the analysis server would no longer consider it as part of the monorepo, resolution of the package will be done with its exact dependencies and not the shared ones.

From the analysis server's perspective, a monorepo would be one workspace containing several packages, the individual packages rooted at their respective pubspec.yaml files.

bwilkerson commented 1 year ago

I wonder if we should pause before plunging here and validate our assumption?

Yes, if we haven't already done so we should absolutely validate our assumptions.

That is monorepo might contain multiple entry point packages (e.g. different apps) and these entry point packages have inconsistent version resolutions when it comes to the external packages.

But presumably a consistent version resolution when it comes to the non-entry-point packages within the root directory?

If so, the first thing the analyzer would need is to have one package_config.json file per entry-point package and no package_config.json for the non-entry-point packages. That would get us down to one analysis context per entry-point package without the need for an addition context for each of the non-entry-point packages. I don't think we can get any better than that.

But, that repository style (not sure what to call it; maybe 'multi-rooted-mono-repo'?) would raise at least one interesting UX question: when a user is viewing a file from a non-entry-point package, which entry-point package's version resolution (package_config.json) do we use when

We could choose one arbitrarily, allow the user to tell us, or try to merge the information from all of them, but all of these approaches will require additional supporting implementation work. It isn't a blocker, but it is a question we'd need to answer.

On the other hand, unless we find evidence that the currently proposed form of mono-repo ('single-rooted-mono-repo'?) won't satisfy a reasonable percentage of our user's needs, we could support the current form as a first step while we try to figure out which other styles to support and how best to do that. That approach would allow the users we could help with what we have today get help sooner.

I definitely want us to be confident that the currently proposed form would provide significant value before we invest too much effort on it, but I don't believe that we should wait to support it just because we find out that it won't help every user's use case. (I'd be more cautious if I thought that expanding our support to a wider set of use cases was likely to invalidate the work done to get the first form of monorepo implemented, but I don't think that's going to be the case.)

jacob314 commented 1 year ago

I've created an umbrella doc for the feature that should hopefully clarify some of these concerns. Happy to chat more on discord and or in a meeting. Please add comments and questions on the doc. TLDR: workspace support is something optional that we have heard strong evidence that particularly enterprise customs could benefit from. Benefits are performance and less confusing IDE behavior. Unsurprising as our google3 users benefit from it in similar cases where they have large # of packages that make up a single app where few of the packages need to be published on pub but using multiple packages helps better structure the app. https://docs.google.com/document/d/1_SfTg0W8PASZ2neTkNhYjcVDdFvomeoAnZ_XqflIx88/edit?resourcekey=0-dcmTZFHicV9ZZL863zC5-w#heading=h.2e7qqcl8ynyv

incendial commented 1 year ago

@jacob314 thanks for sharing the doc, I left a few questions that I didn't find an answer to.

srawlins commented 1 year ago

I don't think the feature relies on a single version solve. As @keertip and the pub proposal note, "there is a way out if for some reason a package's dependencies are unable to meet the global constraints." I can think of two circumstances in which the developer might reasonably/commonly desire non-compatible version constraints on a dependency:

  1. incremental migration from an earlier version to a later version. I go into this scenario at https://github.com/dart-lang/pub/issues/4022#issuecomment-1786203301.

  2. dev_dependencies for testing or tooling code in two separate packages.

    It's not pretty or "great" engineering, but you can imagine a scenario where multiple teams working in one mono-repo end up using different versions of a dependency in their testing code or tooling code. Let's say an engineer on one team uses sweet_yaml_parser v2.0.0 in some scripts in packages/pkg_one/. They rarely edit the scripts and don't touch that code, and after a brief look, they've seen that sweet_yaml_parser v3.0.0 is a huge API change; it would take a few days of work to migrate to v3.0.0, and lots of manual testing because they have minimal test coverage for scripts in their packages/pkg_one/tools/ directory.

    Meanwhile a different developer, on a different team, needs sweet_yaml_parser v3.0.0 for packages/pkg_twenty/. They've looked into v2.0.0 but it actually doesn't have the features they need. Since sweet_yaml_parser is a dev dependency in pkg_one, it's ok if it's a different version from the dependency in pkg_twenty. Maybe the dependency in pkg_twenty is a regular one, or a dev one. Either way, apps will not have a version-solving issue, even if they depend on pkg_one and pkg_twenty. These teams would strongly prefer to keep these two versions of sweet_yaml_parser in play.

    The solution might be to not use external resolution in pkg_one. It might be to separate out the packages/pkg_one/tools/ code into a new package, and keep pkg_one's resolution external, and not use external resolution for the new awesome_yaml_tools package.

In these reasonable situations, the mono-repo support still allows a DAS instance rooted to the mono-repo root to have many fewer analysis contexts, and allow developers to have more confidence in the consistency of their external dependencies. Instead of 30 analysis contexts, maybe you get down to 1. Or maybe 5 due to dev_dependency version conflicts. Or maybe, during a big migration of a dependency, you have 10 analysis contexts temporarily, and you're back to 1 after the migration. Instead of 300 analysis contexts, maybe they get away with 20 as a steady state.

jacob314 commented 1 year ago

Analyzer memory usage should be fine with what Sam is suggesting + the monorepo workspace feature as described. We have telemetry tracking memory usage relative to the # of analysis contexts that show that having < 10 contexts is ok for memory usage. The reason is that while the Dart Analyzer increases memory usage with # of contexts the increase is sub linearr. P90 memory usage with 6 contexts is only 70% higher than P90 memory usage with 1 context not 500% higher as it would be if it was linear. The problem is with >=20 contexts where P90 memory is over 300% higher than with 1 context. These numbers also slightly exaggerate the correlation as projects with larger #s of contexts tend to have more code.

sigurdm commented 1 year ago

@mraleph

I wonder if pub team could create a tool which takes a path to monorepository root and computes the global resolution for all pubspec.yaml files found - and reports if such global resolution is possible or not.

I created https://github.com/sigurdm/pub_workspace_tool

I have tested it on the protobuf.dart monorepo, and there a common resolution works.

mit-mit commented 3 months ago

Status update: workspaces are now supported in dart pub, but not yet in flutter pub (see https://github.com/flutter/flutter/issues/150196)

rrousselGit commented 2 months ago

How does dart pub publish work when a package from the workspace depends on another package from said workspace?

sigurdm commented 2 months ago

How does dart pub publish work when a package from the workspace depends on another package from said workspace?

Which aspect are you asking about?

If a workspace has package A and B and A depends on B, then when publishing A validation of the version constraint happens against the local version of B.

But consumers of the published package will use the published version of B.

rrousselGit commented 2 months ago

Ignore me I was confused by a separate error.

Long story short, I tried workspaces and made two packages A and B where A depends on B ... but forgot to add a version number on either of them:

name: b
environment: 
  sdk: ^3.5.0-259.0.dev
resolution: workspace

Then, inside A, I tried to depend on B with:

name: a
environment: 
  sdk: ^3.5.0-259.0.dev
resolution: workspace
dependencies:
  b: ^1.0.0

But got:

Because _ depends on a which depends on b ^1.0.0, b ^1.0.0 is required.
So, because _ depends on b, version solving failed.

This error confused me and I thought the only way to make workspace work with unpublished packages was:

dependencies:
  b: # no constraint specified

Hence the earlier question. I wondered which constraint would be picked when publishing A.

Maybe we could improve the pub get error message when the constraint issue point to a package from the workspace?

Such as:

Because A depends B ^1.0.0 from workspace, B ^1.0.0 is required.
Because B from workspace has no version, no B ^1.0.0 version is found.
So, because A depends on B, version solving failed
sigurdm commented 2 months ago

Yeah I can see the confusion.

I have created https://github.com/dart-lang/pub/issues/4356 to see if we can improve the messaging here.