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

Proposal for resolution: external as support for mono-repositories #4022

Closed
sigurdm opened this issue Sep 15, 2023 · 24 comments
Closed

Proposal for resolution: external as support for mono-repositories #4022

sigurdm opened this issue Sep 15, 2023 · 24 comments
Labels
type-enhancement A request for a change that isn't a bug

Comments

@sigurdm
Copy link
Contributor

sigurdm commented Sep 15, 2023

NOTE: This proposal has been partially succeeded by a proposal with a proper workspace file. See https://docs.google.com/document/d/1UEEAGdWIgVf0X7o8WPQCPmL4LSzaGCfJTM0pURoDLfE/edit?resourcekey=0-c5CMaOoc_pg3ZwJKMAM0og . We probably want to support resolution: external as well as resolution: workspace for cases such as the dart sdk where the package-config is not created by pub.

This issue suggests a simple mechanism in the pub client for better supporting a shared resolution between the packages of a mono-repo. Namely adding

resolution: external

to a pubspec.yaml to indicate that pub should not treat it as a root package.

The main parts of this proposal does not contain much support in the pub client for how to manage multiple packages together in a single project. We instead suggest pushing more advanced support to external tools such as package:mono_repo or package:melos.

Motivation

Mono-repositories are repositories with tightly related packages that are often developed and released together.

The dart team is maintaining a number of mono-repos Eg:
dart-lang/test
google/protobuf.dart
dart-lang/ecosystem

Another special case of mono-repos is the dart SDK where a common resolution is used, but not maintained by pub, even though many of the subpackages have a pubspec.yaml, and some are published to pub.dev individually...

Externally we know of several customers using mono-repositories.

When developing in a mono-repo it is often desirable to use a shared version resolution for ensuring consistent behavior.

By having a shared resolution, tools such as the analyzer can also share context between analyzing each package, with big potential memory savings.

Some packages are in a mono-repo for organizational purposes, and might not want to share the same resolution (and would not benefit from this proposal). One such repo could be: flutter/packages.

Some tutorials are split in multiple "chapters" each with a single pubspec.yaml, and the analyzer has been known to choke on those because they each have a separate context. They could perhaps benefit from this proposal (if developing the pubspec.yaml is not part of the tutorial).

Existing support for mono-repositories

There exists tooling to support handling of mono-repos, such as mono_repo.dart and melos. These tools can help running the same command for all sub-projects
They, however, don't allow a shared resolution between the packages. These could be updated to allow for generating a shared root pubspec.yaml.

Proposed mechanism

A new section in pubspec.yaml would mark a pubspec that it should not be used for a "local resolution", but instead rely on the Dart VM's mechanism for searching parent directories for a .dart_tool/package_config.json containing a resolution to use.

name: test
environment:
  sdk: ^3.1.0
dependencies:
  ...
resolution: external
  • Given this, dart pub get would not generate a dart_tool/package_config.json file in the same directory. Instead it could either (still open):

    • Report an error: "This package is not intended for direct resolution"
    • Look for a pubspec.yaml in parent directories and resolve that instead.
    • Do nothing.
  • dart run and dart test needs to ignore pubspec.yamls with resolution: external when resolving packages.
    dart pub publish should however consider this pubspec.yaml when doing validations.
    We could create a lint discouraging from having dev_dependencies and dependency_overrides in a pubspec.yaml with resolution: external.

Creating a mono-repo

Now to create a mono-repo, one would create a "root" pubspec.yaml with dependency_overrides with path-dependencies on all the sub-projects.

name: my_mono_repo_root
publish_to: none
environment:
  sdk: ^3.0.0
dev_dependencies:
  build_runner: ^2.0.0
dependency_overrides:
  pkg1:
    path: pkgs/pkg1
  pkg2:
    path: pkgs/pkg2

All dev_dependencies should be collected in the root pubspec.yaml, and thus shared between the subprojects.

Running dart pub get in this root-directory will create a shared pubspec.lock (that can be checked into source control if desired) and a shared dart_tool/package_config.json for consumption by the Dart VM and analyzer.

  • We now rely on the dart run resolution mechanism to look up through parent directories for finding the dart_tool/package_config.json file, and providing the package resolution. This should enable dart test to still work from the sub-project folder (we need to test that this works).

  • One could think of mechanisms for "linking" the sub-projects into the root-project, such that they could be operated on as a whole (eg. when doing upgrades). We propose to (at least as a first step) leave such functionality out of the pub client, and for example

The Dart SDK

In the Dart SDK we can mark all the in-sdk-repo packages as resolution: external.

There will not (at least for now) be a root pubspec.yaml, but we will instead rely on the current alternative tooling to generate a shared dart_tool/package_config.json.

Open questions:

  • Do we need a way to mark the sub-dependencies as special, such that Pub knows how to update them in sync? Or are dependency_overrides good enough?
    • We could add this later if there's a high demand
    • We could also autodetect overridden path-dependencies from the root-package and handle them special.
  • We need to figure out how build_runner would work in such a setup. Can each sub-project contain its own build.yaml?
  • How does this affect dart pub add? Can you add to a sub-package, and should that resolve together with other packages?
  • Should dart pub add --dev find the root pubspec.yaml?
  • How does this affect dart pub upgrade --major-versions? Is there a way to upgrade all the pubspec.yaml's of a project in sync? (Could perhaps be added later)

We might need a way to force resolution of the single package, e.g. for deployment of a sub-package. (Could we piggy back on dart pub get --enforce-lockfile - then we should have called it dart pub get --deploy 😐)

Potential downsides

  • Relying on dependencies_overrides can cause some issues
    • Dependency constraints among sub-projects are not checked.
      When publishing you might have constraints not matching sibling dependencies

      This can to some extent be helped by doing a sub-package-only resolution in dart pub publish.

  • Diamonds where you depend on other packages on pub.dev, and these packages also depends on another package in your mono-repository
    • These are rather rare - perhaps we don't need strong support for those.
  • cycles with other packages on pub.dev
    • These are rather rare - perhaps we don't need to support those.
  • Mono-repository root pubspec.yaml might be missing the dependency_override that should include a child.
    • Could we warn against this?
@jonasfj
Copy link
Member

jonasfj commented Sep 15, 2023

I think the core of this approach is to enable tooling like mono_repo and melos to manage mono-repositories with a single resolution better.

So if the root pubspec.yaml does have dependency_overrides for all child-packages, that seems like an issue for mono-repo tooling to deal with.

I suspect that when dart pub get encounters a pubspec.yaml with resolution: external then it should walk up until it finds a root pubspec.yaml that should be resolved, or finds a .dart_tool/package_config.json which provides a resolution.

Regarding, tools that modify pubspec.yaml, such as:

  • dart pub upgrade --major-versions
  • dart pub add
  • dart pub add --dev

I don't know what we should do. If pub isn't doing the resolution. Then maybe we do not support these, how can we?

To be clear: the idea is to let mono-repo tooling disable pub get locally. Whether mono-repository tooling delegates resolution to a root pubspec.yaml, or does other magic to produce a resolution is left for the mono-repo tooling to decide.

@sigurdm sigurdm added the type-enhancement A request for a change that isn't a bug label Sep 21, 2023
@olexale
Copy link

olexale commented Sep 25, 2023

I’m working in a mono-repo with more than 300 packages, so I’m very curious about this proposal.
I’m unsure if I fully understand it, so I prepared some questions/comments:

  1. The name «external» makes a lot of sense from the tooling perspective, when you are thinking about individual packages, but in the context of the application development, an «internal» would make more sense, as for my mono-repo, these packages are internal, and external packages are packages from pub.dev and other sources.

  2. Could you please elaborate more on the expected additional support from the mono_repo and melos?

  3. What if I have multiple applications in the same monorepo? For example, I have the following structure:

apps
 - app1
 - app2
utilities
 - util1
 - util2

I.e. I have app1 that depends on util1 and util2, and app2 depends on both these utilities too. Will this scenario still be supported?

  1. I see that is still an open question, but if code generation and testing behavior changes, i.e. it will not be possible to run tests and generate code per package, that would kill the idea of having a mono-repo for our project as we will not be able to optimize CI pipelines (we are running tests and other checks on modified packages only), and locally I’ll not be able to quickly run tests for a package I’m working in - tests for the whole app will be executed, which, for our project, will take a ridiculous amount of time if I’m working on a single feature.

  2. The «dependency_overrides» has clear semantics, but adding internal (I mean «external») packages to the list mixes packages that are indeed overridden and the regular ones. If that change is going to happen, I think it will make more sense to have a separate section in the pubspec for it.

Unfortunately, I don’t think I understand the real benefit of this change. I can see how the proposed changes fix the analyzer performance, which is IMO the main pain for mono-repos now, but it seems like we are solving this problem of a tool at the expense of the developer’s experience. Even if the proposed functionality already existed, I would prefer keeping the «Only Analyze Projects With Open Files» flag on in my IDE (which fixes the analyzer performance, not without drawbacks ofc) and keep the project simpler.

@blaugold
Copy link
Contributor

blaugold commented Sep 25, 2023

This proposal looks really promising.

Analyzer performance in large mono-repos is an issue I have heard of from multiple people, so reducing analyzer contexts would be a big win. Having to only resolve one package instead of 10s or 100s also has the potential to speed up CI jobs.

Potential issues

  • It might not always be possible to find a shared resolution for all packages in a mono-repo. Resolving the dependencies of many packages with many more transitive dependencies together makes it that much more likely that there are conflicting dependency constraints. It's easy enough to opt-out individual packages out of shared resolution by removing resolution: external, though. Tools like package:mono_repo and package:melos may have to specifically support a mixed setup.
  • Is depend_on_referenced_packages still going to work when dev_dependencies are specified in the shared package used for resolution?
  • Is the flutter tool going to work without a dart_tool/package_config.json in the package root?

@sigurdm
Copy link
Contributor Author

sigurdm commented Sep 26, 2023

@olexale

Thanks for the feedback! Some initial responses:

  1. The name «external» makes a lot of sense from the tooling perspective, when you are thinking about individual packages, but in the context of the application development, an «internal» would make more sense, as for my mono-repo, these packages are internal, and external packages are packages from pub.dev and other sources.

Interesting point. But I still think that even though the packages are internal, the resolution is external, and thus I think the term makes some sense. But we are not fixed on the naming if you have better suggestions.

  1. Could you please elaborate more on the expected additional support from the mono_repo and melos?

Well I cannot promise what these packages will implement. But I could imagine that these packages could help generate the repo-wide pubspec.yaml. Perhaps they could also help making repo-wide upgrades (somehow).

  1. What if I have multiple applications in the same monorepo?

Yeah, I think your example should be supported, as these will all be included via dependency overrides.

  1. I see that is still an open question, but if code generation and testing behavior changes,

I don't think this is as much an open question, as tasks that will have to happen for this proposal to be useful.

  1. The «dependency_overrides» has clear semantics, but adding internal (I mean «external») packages to the list mixes packages that are indeed overridden and the regular ones. If that change is going to happen, I think it will make more sense to have a separate section in the pubspec for it.

Again, it is not the packages that are external, but the resolution mechanism.

The idea is to keep the proposal minimal for now.
If we find that in practice a special way of declaring that a dependency is part of a mono-repo is needed it can be added later.

Unfortunately, I don’t think I understand the real benefit of this change. I can see how the proposed changes fix the analyzer performance, which is IMO the main pain for mono-repos now, but it seems like we are solving this problem of a tool at the expense of the developer’s experience. Even if the proposed functionality already existed, I would prefer keeping the «Only Analyze Projects With Open Files» flag on in my IDE (which fixes the analyzer performance, not without drawbacks ofc) and keep the project simpler.

I think the main point is ensuring that all the sub-packages in a mono-repo share a single resolution (a single pubspec.lock), and thus reduces the potential for the confusion that could occur if two sub-packages end up with different versions of a dependency - causing surprising behavior.

Also there is potentially some time to save by only running pub get once.

@sigurdm
Copy link
Contributor Author

sigurdm commented Sep 26, 2023

@blaugold

Also thanks for feedback!

It might not always be possible to find a shared resolution for all packages in a mono-repo.

IMO if a shared resolution cannot be found, it is an advantage to find out early and solve that issue somehow, rather than developing with different versions of dependencies in different sub-projects.

It's easy enough to opt-out individual packages out of shared resolution by removing resolution: external, though.

Good point - though it would perhaps be a confusing state of the mono-repo, but it shows there are ways out if really needed.

Tools like package:mono_repo and package:melos may have to specifically support a mixed setup.

yeah - they probably need to recognize this pattern. Really good point.

Is depend_on_referenced_packages still going to work when dev_dependencies are specified in the shared package used for resolution?

Good question! I guess that depends what pubspec that lint is going to look into - @pq do you have input here?

Is the flutter tool going to work without a dart_tool/package_config.json in the package root?

Another good question. Ideally it 'should just work' (from the definition of package config), but there will probably be stuff breaking.

@christopherfujino can you foresee stuff that would work poorly in this model?

@pq
Copy link
Member

pq commented Sep 27, 2023

Is depend_on_referenced_packages still going to work when dev_dependencies are specified in the shared package used for resolution?

Good question! I guess that depends what pubspec that lint is going to look into - @pq do you have input here?

Good question. @jakemac53? 📟

@jakemac53
Copy link
Contributor

Is depend_on_referenced_packages still going to work when dev_dependencies are specified in the shared package used for resolution?

No it would not work. Is there any particular reason the proposal here says not to correctly specify dev_dependencies per-pubspec? I would suggest just requiring them to be there for each package still.

If we don't have a standard that requires a root level pubspec, I don't see how this could be supported either. Unless we just wanted the lint to ignore entirely non-lib dirs in packages with resolution: external.

  • We need to figure out how build_runner would work in such a setup. Can each sub-project contain its own build.yaml?

This should work fine as long as they still have valid pubspecs as well. This would need to include dev dependencies though, similarly to the depend_on_referenced_packages lint use case.

  • How does this affect dart pub add? Can you add to a sub-package, and should that resolve together with other packages?

It should error imo. And pub get/upgrade should also error.

@jakemac53
Copy link
Contributor

There is a potential separate issue here - there might be tools which just assume a file at exactly .dart_tool/package_config.json. These tools could certainly be updated but up until now it was a safe assumption to make. It is hard to know what would break without actually making the change, but I would expect some amount of pain in the transition.

@christopherfujino
Copy link
Member

There is a potential separate issue here - there might be tools which just assume a file at exactly .dart_tool/package_config.json. These tools could certainly be updated but up until now it was a safe assumption to make. It is hard to know what would break without actually making the change, but I would expect some amount of pain in the transition.

Yeah, I'm pretty sure many things would break in the tool. Worse, it seems like it would be more difficult to reason about where to find this information? Obviously, this is something we could overcome, but I suspect it would be painful.

@christopherfujino
Copy link
Member

What about if pub still wrote the package_config.json, but perhaps if its contents contained the absolute path to the root file?

@jakemac53
Copy link
Contributor

Yeah, I'm pretty sure many things would break in the tool. Worse, it seems like it would be more difficult to reason about where to find this information? Obviously, this is something we could overcome, but I suspect it would be painful.

Fwiw, the behavior is fully specified https://github.com/dart-lang/language/blob/main/accepted/2.8/language-versioning/package-config-file-v2.md#finding-the-file. But, that doesn't mean all tools actually do that lookup logic given they don't really have to for most external use cases.

@jakemac53
Copy link
Contributor

Also, reading the specification it actually does currently talk a lot about Pub specifically, and its current behavior of putting a file in each package root.

So we would want to probably treat this as a breaking change and go through the normal breaking change processes.

@jakemac53
Copy link
Contributor

jakemac53 commented Sep 27, 2023

What about if pub still wrote the package_config.json, but perhaps if its contents contained the absolute path to the root file?

That might be a nice way to work around things, although very likely it would still be breaking for users of package:package_config. I am not sure that package could migrate to understanding some sort of "include" format seamlessly.

Although possibly it could be a symlink? I am not sure this would work on windows.

@christopherfujino
Copy link
Member

Although possibly it could be a symlink? I am not sure this would work on windows.

Please no symlinks :)

@sigurdm
Copy link
Contributor Author

sigurdm commented Sep 28, 2023

No it would not work. Is there any particular reason the proposal here says not to correctly specify dev_dependencies per-pubspec? I would suggest just requiring them to be there for each package still.

Because dev-dependencies are only resolved for the root pubspec.

@sigurdm
Copy link
Contributor Author

sigurdm commented Sep 28, 2023

So we would want to probably treat this as a breaking change and go through the normal breaking change processes.

I would argue that this is not breaking. No existing code should be affected. The root pubspec will be the one resolved, and where the .dart_tool/package_config.json is placed exactly as specified in https://github.com/dart-lang/language/blob/main/accepted/2.8/language-versioning/package-config-file-v2.md#finding-the-file . So from that perspective nothing changes.

What is new is that we have to teach all our tools to not expect being run in the root project. And the specification explicitly talks about searching parent directories. And in that sense we also tap in to an existing (but perhaps not very used) mechanism.

@jakemac53
Copy link
Contributor

Because dev-dependencies are only resolved for the root pubspec.

What makes dev dependencies different from regular dependencies in this respect? The tools will still be used at the package level, so I think it is reasonable to specify them.

You should at least be allowed to specify them, as it will be required for build_runner and the depend_on_referenced_packages lint.

@jakemac53
Copy link
Contributor

jakemac53 commented Sep 28, 2023

Regarding breakage, the spec also has wording like this The new file will be stored by Pub in the .dart_tool sub-directory of the current pub package's root directory.. Now, you could claim that the new behavior won't change that because it only does resolution from the "root" package, but that is not imo a reasonable interpretation.

As a user, if I have a structure like this:

  • pubspec.yaml
  • foo_pkg
    • pubspec.yaml

And I am currently in the foo_pkg directory. The current package is not the root package, it is foo_pkg, and there will not be a file at foo_pkg/.dart_tool/package_config.json.

The description of "root_uri" for a package even explicitly calls this out All files inside this directory, including any subdirectories, are considered to belong to the package, except if they belong to a nested package. There is still a nested package here, it just doesn't have its own resolution.

More concretely, if this is known to break existing packages, then regardless of what the spec says or if you agree its breaking according to the spec, it is in fact breaking.

@jonasfj
Copy link
Member

jonasfj commented Oct 16, 2023

@olexale, I think you're touching on something really important here. Specifically, the question easily becomes are mono-repository developers willing to use a single resolution? Meaning picking a single-version of each dependency for all apps/packages in the mono-repository -- this is not necessarily a small price, but it doesn't have many other upsides.

  1. What if I have multiple applications in the same monorepo? For example, I have the following structure:
apps
 - app1
 - app2
utilities
 - util1
 - util2

If dependencies for app1 and app2 use the same versions of all dependencies, then you could imagine something like:

pubspec.yaml  # has dependency_overides pointing to apps/app1, apps/app2, utilities/util1, ...
pubspec.lock  # The only lock file in the project
.dart_tool/
  package_config.json # the only place we have resolution.
apps/
  app1/
    pubspec.yaml # resolution: external
  app2/
    pubspec.yaml # resolution: external
utilities/
  util1/
    pubspec.yaml # resolution: external
  util2/
    pubspec.yaml # resolution: external

If app1 and app2 don't use the same versions of all dependencies, then you can't really have a single-resolution mono-repository. It might be worthwhile adopting something like the one version rule, I don't think it's a secret that this is how google works internally.
This makes upgrading dependencies more painful. It makes pulling in new dependencies harder, in case they conflict with another existing dependency. There are lots of pain doing this, but it makes it easier to manage a large code base over time, and ensures that all utilities can be used together.

If you have a special case where app2 just has to use a specific version of some package, that is different from the rest, then it would be possible to do something like:

pubspec.yaml # has dependency_overides pointing to apps/app1, utilities/util1, ...
pubspec.lock
.dart_tool/
  package_config.json
apps/
  app1/
    pubspec.yaml # resolution: external
  app2/
    pubspec.yaml # not using "resolution: external", so opting out here
    pubspec.lock
    .dart_tool/
      package_config.json
utilities/
  util1/
    pubspec.yaml # resolution: external
  util2/
    pubspec.yaml # resolution: external

But this quickly gets messy. For this proposal (resolution: external) to be a useful mechanism, you pretty much have to use a single dependency resolution for all packages and applications in your mono-repository. If you do on occasion have a single project that needs to opt-out, then that's probably feasible -- but if conflicts are common and you're not willing to deal with them, this proposal won't fix much for you.

@srawlins
Copy link
Member

@jonasfj I agree that having the single version solve is important and the discipline really pays off in the long run. But I also think there are times when the discipline of committing small changes butts heads with the single version policy, and it's important to have an escape hatch. And I think this proposal supports that (correct me).

So let's say I have a mono-repo with 30 packages, and 4 of them are apps, which use package:super_widget v3.0.0, and then super_widget 4.0.0 is released, with sweeping breaking changes. The code is so different, it takes a few days to migrate one app to use the new APIs. Even then, the commit made to use super_widget v4.0.0 in that one app may introduce bugs, and the commit may need to be rolled back.

If you were forced to update all 4 apps in one commit, this could easily be a bridge too far. Maybe you don't know the business logic in the other 3, or even just 1 of the 3. Maybe after a Herculean effort, you commit a migration of all 4, but it's got bugs revealed in manual testing, and must be rolled back a few days after it lands, but in the meantime the 4 apps have received other commits. There are even more mundane reasons like App 1 has a high priority feature, relying on super_widget v4.0.0, shipping in Q1, but the other 3 apps have other priorities and cannot commit engineering time to bump super_widget in their apps this for Q1. It's easy to imagine disaster when some infra requirement prevents small commits.

So I think what you would do with resolution: external is "remove" some of the apps from the mono-repo. Not file-system-wise (leave them there), but in terms of participating in the single package config. It's a tidy, incremental process:

  1. Remove resolution: external from the one app. It now gets its own package config.
  2. Bump package:super_widget to v4.0.0 in that app.
  3. Work through the breaking changes in that one app.
  4. Commit.
  5. Remove resolution: external from the next app.
  6. Continue.
  7. Fold the four apps back into the mono-repo when the process is done.

In the interim time, you do not get all of the benefits of the mono-repo: For example, analyzer would run with 2 analysis contexts, then 3, 4, and 5, before dropping back to 1. But remember, the mono-repo has 30 packages, so you're still in a wildly better place in terms of analysis contexts. In terms of the hopes and dreams that your 30 packages have a single (or minimal) version solve, your much closer than 30. Again, you would have 2 version solves (package config files), then 3, 4, 5, then back to 1.

This scenario is actually pretty rosy, and there can be situations that will require more effort or coordination, but it's a good starting place. For example, maybe one of the 30 packages is a notifications widget that 3 of the apps use. If that notifications widget depends on super_widget v3.0.0, and one or some of the 3 apps use super_widget v3.0.0, it won't be as easy to bump super_widget to v4.0.0 incrementally. The idea is that developers should be empowered with some escape hatch to avoid at all costs a massive all-packages breaking-change pub dep version bump.

@jacob314
Copy link
Member

@sigurdm wrote up a slightly different proposal that accomplishes the same goal.
I think we will likely want resolution: external for cases like the Dart SDK monorepo but I think this proposal is a simpler solution for cases where you just want to define a workspace to include all packages of an app.

pub_workspace.yaml

@rrousselGit
Copy link

rrousselGit commented Nov 14, 2023

How would that resolution: external/workspace work once a package is published on pub?

@srawlins
Copy link
Member

It wouldn't affect it. When a package using that resolution is published, the resolution field does not play into the dependency constraints entered into the pub database.

@sigurdm
Copy link
Contributor Author

sigurdm commented Aug 1, 2024

Closing this as we are implementing workspaces #4127 instead.

@sigurdm sigurdm closed this as not planned Won't fix, can't repro, duplicate, stale Aug 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type-enhancement A request for a change that isn't a bug
Projects
None yet
Development

No branches or pull requests

10 participants