Evaluate using pub workspace feature, dropping custom package config generator

[jakemac53]

For context on workspaces, see flutter.dev/go/pub-workspace. cc @jonasfj @sigurdm in case you all had considered this, or think I am off my rocker 🤣.

Proposal

• Use the pub workspace feature to manage our SDK package config, instead of having a custom tool to manage it.
• All SDK developed packages become workspace packages in the root pubspec.
• All third party packages are listed as dependency overrides in the root pubspec.

See my WIP branch, which gets package:analyzer working. More packages would have to be added to complete it.

Benefits

• We would be able to use standard tools such as dart run and dart test (and dart pub get) in SDK packages without negative consequence.
• In particular the ability to use the standard test runner, with all its features.
  • Much nicer test output.
  • Ability to filter tests by dir/file/name.
  • Ability to mark tests as skipped in code instead of using the custom results database.
  • Ability to easily run web/AOT tests.
• Should work better with IDE and other tooling (debugging from IDE, running individual tests, etc). cc @DanTup
• Can possibly drop some other custom tools related to tests or other things.
• All SDK packages (vendored and local) are listed in one place - the root workspace pubspec.yaml.

Risks

• How would this interact with the SDK test runner? Is it risky to have both? People might use certain features from the test runner which are not respected when their tests are ran on the bots.
• How does this interact with the existing test configurations? Are package tests ran on multiple configurations?
  • Conversely, would this make it easier for packages to run tests on web/AOT?
• Version constraints for third party packages aren't checked (this doesn't really happen today either though), because they are all dependency overrides.
  • You do have the option of temporarily removing the package from the workspace, to check version constraints with a normal solve.
• There could be some edge cases around very recent SDK version constraints. If a package requires an SDK which is newer than an SDK developers SDK, they might be blocked? They wouldn't be able to pub get, and also wouldn't be able to build a new SDK that is compatible, because that requires a pub get.
  • Possibly, the pre-built SDK could be used for pub get in gclient runhooks to resolve this issue.

Additional work required

• Some validation that we have no hosted dependencies in the generated pubspec.
• Change gclient runhooks to run pub get instead of invoking the custom tool.
  • This way people don't have to run a manual pub get just like today.
• Possibly update how SDK tests are run, to use the actual test runner, at least for packages which opt in.
• Figure out a solution for node_preamble (custom placeholder package?)
• Update some tests which use patterns not allowed by the package:test test runner (arguments passed to main)
• Pub may need an option to omit some of the information from the bottom of the package config, as it contains local file system paths. If we don't check this file in though, then it doesn't matter.
• Pub may need an option to ignore SDK constraints entirely (see risks section). These are generally not meaningful when developing inside the SDK itself.

Other ideas

• Could we get a mode from pub where we can use a path dependency for packages, but still respect their version constraints?

Trying out the example

1. Check out the branch, from the SDK repo:

git remote add jakemac git@github.com:jakemac53/sdk.git
git fetch jakemac
git checkout jakemac/pub-workspace 

2. Run gclient sync -D (need some extra deps for the test runner)

3. Run pub get from the root of the SDK (creates the package config). Note that you should not run gclient runhooks, that will currently bash over this package config.

Note: You can also now run this from any workspace package, and it will not create a new package config for that package. It will only update the root one.

4. Use the test runner in pkg/analyzer:

cd pkg/analyzer
dart test

5. Profit

Actually, you will see some tests fail because they take command line arguments which isn't allowed in the test runner, this is called out above. It is rare but we would have to look into these.

[DanTup]

Should work better with IDE and other tooling (debugging from IDE, running individual tests, etc). cc @DanTup

Yes please! :)

Dart-Code might require some changes (because we have some assumptions about having a .dart_tool/package_config.json alongside a pubspec.yaml then disable a bunch of things in the Dart SDK where those assumptions aren't true), but I had always hoped (assumed?) that once this is done, we'd be able to use dart run and dart test in the SDK without breaking things and we can stop special-casing the SDK (who knows how many other repos there are that don't get special treatment in Dart-Code and have wonky behaviour).

There are some other minor niggles for some tests in VS Code (for ex. we don't understand package:test_reflective_loader tests) that I think are solvable but I haven't dug into them too much yet because not being able to run dart test has been the major blocker.

(FWIW, how the package_config is generated is less important to me/Dart-Code, as long as the SDK is flagged in a way that means dart test doesn't break things - which is what dart-lang/pub#4022 was about)

[jakemac53]

because we have some assumptions about having a .dart_tool/package_config.json alongside a pubspec.yaml

Have you tried the pub workspaces feature? Does it have the same issue or no? It would be worth fixing it to work with workspaces if it doesn't currently because that is now a more general feature.

[devoncarew]

We would be able to use standard tools such as dart run and dart test (and dart pub get) in SDK packages without negative consequence.

💯

I haven't looked at the impl. tool closely. The SDK+gclient is (mostly? completely?) hermetic currently. Can this work with running a pub get --offline after gclient setup? Is there enough info encoded in the pubspec for pub get to work with only the info in the repo - w/o having to download other packages?

You may try replacing this bootstrap into the dart setup code with the dart pub get --offline equivalent: https://github.com/dart-lang/sdk/blob/main/DEPS#L805.

[DanTup]

Have you tried the pub workspaces feature?

No, I thought it was still being designed/work in progress 😄 Are there instructions for trying it out? Does it need flags and/or a specific SDK version?

[jonasfj]

Can this work with running a pub get --offline after gclient setup?

If all dependencies are workspace packages I would think: yes.

I don't even think you need to do --offline since it will never go online to check for package versions, if all dependencies of all packages are workspace packages (or overridden with a path-dependency).

There could be corner cases, but it's worth testing, and possibly fixing if it doesn't work.

[jakemac53]

@jonasfj Would it be possible to add an offline: true option to the pubspec.yaml to enforce that no pub get for that package should ever reach out to pub?

[jakemac53]

No, I thought it was still being designed/work in progress 😄 Are there instructions for trying it out? Does it need flags and/or a specific SDK version?

I am not sure on instructions, but you can look at the build repo. Or the branch of the SDK I linked above.

tldr;

• Use a recent dev SDK (last one from 3.5 works)
• Add a root pubspec to your repo with a section like workspace: ["pkg/a", "pkg/b", ...].
• In each package listed as part of the workspace, add resolution: workspace to the pubspec
• Note that all SDK constraints will have to be a recent dev SDK (probably just whatever version you are on).

[jakemac53]

cc @athomas I hear you might be a good person to discuss this with?

[athomas]

@jakemac53 Happy to, I've wanted to use pub for a while to generate the package config and discussed it with sigurdm in the past (though this was pre-workspaces and never high enough priority).

I see in DEPS you didn't remove the old package_config generator. It should be replaced running pub get from the checked-in SDK. We'd also need to ensure the bots manage their pub cache but that's a relatively simple change (we should do anyway).

I would suggest we split "use workspaces" from the "run tests with the package:test runner". I'm broadly supportive of removing bespoke things, but I don't think these need to be coupled. I would assume that generating the package config in a different way can be made compatible with the SDK test runner relatively easily.

[jakemac53]

I see in DEPS you didn't remove the old package_config generator. It should be replaced running pub get from the checked-in SDK. We'd also need to ensure the bots manage their pub cache but that's a relatively simple change (we should do anyway).

Correct, my CL is not complete, just enough to get basic functionality working to test things out. I would be happy to complete it though if we want to move forward.

I would suggest we split "use workspaces" from the "run tests with the package:test runner".

Agreed, I think running actual tests using the test runner on the bots might not be a task worth investing in. In particular I am not sure how that would interact with the test database.

But, developers would be able to use the test runner locally still to run tests more easily. And the IDE tooling could use it to run tests, which would enable some nice functionality there.

I would assume that generating the package config in a different way can be made compatible with the SDK test runner relatively easily.

Yes I don't believe anything has to be changed here, it should just work.

[DanTup]

@jakemac53

I am not sure on instructions, but you can look at the build repo. Or the branch of the SDK I linked above.

Thanks! I grabbed build and can see that we definitely aren't doing the right thing - we are treated all projects without their own package_config as "needing to have pub get run.

I'll work on this (tracked via Dart-Code/Dart-Code#5067).

[3:33:54 PM] [General] [Info] Version last used for Pub is 3.2.2 (3.2.0), current is 3.6.0-edge.81d13ed904cc2d2c5c673df8114d80ac0c2a622c (3.6.0)
[3:33:54 PM] [General] [Info] Found 7 folders requiring "pub get" or "pub upgrade":
    C:\Dev\Google\build (get: true, upgrade: false, reason: package_config.json is missing)
    C:\Dev\Google\build\example (get: true, upgrade: true, reason: The current SDK version (3.6.0) is newer than the one last used to run "pub get" (3.2.0))
    C:\Dev\Google\build\scratch_space (get: true, upgrade: false, reason: package_config.json is missing)
    C:\Dev\Google\build\tool (get: true, upgrade: false, reason: package_config.json is missing)
    C:\Dev\Google\build\_test (get: true, upgrade: false, reason: package_config.json is missing)
    C:\Dev\Google\build\_test_common (get: true, upgrade: false, reason: package_config.json is missing)
    C:\Dev\Google\build\_test\pkgs\provides_builder (get: true, upgrade: false, reason: package_config.json is missing)

[jakemac53]

I updated my PR to change how gclient runhooks works, setting a custom pub cache and also a fake pub repository URL to ensure we never reach out to pub.

I ran into a separate issue, which is mockito has a regular dependency on source_gen for the builder that it implements. That builder was not used internally, and so we never actually had the dependency (sort of similar issue to node_preamble, but more pervasive).

This ends up needing many more transitive deps because source_gen depends on build.

In general, in the old world we had the ability to pull in only the transitive deps of packages that were used, which was a subset of their dependencies in some cases. We no longer have that option if we go this route, and it looks like that would result in a fair number of additional deps.

[jakemac53]

cc @srawlins have you considered splitting it out a mockito_builder package?

[srawlins]

I have not, I guess the primary benefit would be smaller, more targeted dependencies sets?

I am mostly surprised that mockito is in DEPS, and would 100% support an effort to get it out.

$ git grep 'package:mockito'
pkg/js/README.md:You can also use `package:mockito` to do the mocking with this API, by providing
pkg/js/README.md:a generated mocking object from `package:mockito` to `createStaticInteropMock`.
pkg/vm_service_interface/test/server_test.dart:import 'package:mockito/mockito.dart';
tests/lib/js/export/static_interop_mock/mockito_test.dart:import 'package:mockito/mockito.dart';

Those seem reconcilable...

[jakemac53]

I have not, I guess the primary benefit would be smaller, more targeted dependencies sets?

Right, it may not be worth it, idk. I didn't think mockito was useful for much without the codegen nowadays.

I am mostly surprised that mockito is in DEPS, and would 100% support an effort to get it out.

Yeah, this would be another approach, just don't allow it in the SDK.

[devoncarew]

cc @srujzs for the question of whether we can write around using mockito in tests/lib/js/export/static_interop_mock/mockito_test.dart, and @bkonyi for pkg/vm_service_interface/test/server_test.dart.

[srujzs]

whether we can write around using mockito in tests/lib/js/export/static_interop_mock/mockito_test.dart

I don't have the full context here, but if we can't use mockito in our tests, then this test should be deleted. The point of this test was to provide a proof of concept in using mockito to work with the export functionalities we added, but it isn't directly testing our implementation, so it's okay to delete.

[bkonyi]

@bkonyi for pkg/vm_service_interface/test/server_test.dart.

I think we should be able to rewrite this test to not rely on mockito if we need to.

[matanlurey]

@jonasfj Would it be possible to add an offline: true option to the pubspec.yaml to enforce that no pub get for that package should ever reach out to pub?

I had a similar-ish request here: dart-lang/pub#4328

[jakemac53]

Note that I realized offline: true isn't enough because it will still search for previously downloaded package versions in the pub cache. So you also need to override the pub cache dir.