Support for resolving package config and pubspec.lock in workspaces

[sigurdm]

Instead of expecting pubspec.lock and .dart_tool/package_config.json to reside in the same folder as pubspec.yaml, we visit each parent of the current directory until we find a .dart_tool/package_config.json, and expect pubspec.lock to exist next to that.

Also invoke the generated scripts with an explicit packageConfig.

This is to support running build_runner build in a workspace package, as implemented in dart-lang/pub#4127

[sigurdm]

The CI failures seem unrelated.

[jakemac53]

Is this different from just passing recurse: true?

[sigurdm]

The only difference is that we can use the discovered path rootDir where we find the package config below.

With recurse: true that would not be the case - we would only be given the packageConfig itself...

[jakemac53]

Sure, we could just set the root dir as the parent of the package config dir though. Not a big difference either way but I would slightly prefer it that way.

On a related note though - I am not sure we actually want to load the pubspec from the workspace root instead of the package root? We will end up seeing all the deps used by any package in the workspace then, instead of just the deps of the current package. Unless the current package doesn't list its deps at all?

[sigurdm]

With workspaces there is only a single pubspec.lock.

I'm not sure how build runner uses this dependency graph - we might need to prune it?

[sigurdm]

The only way I can see to do that would be to parse pubspec.yaml of the current package and all its dependencies transitively.

[sigurdm]

Is this a new problem? Did we know the ordering before?

[jakemac53]

Sorry I hadn't seen your other comments when I did my last response :).

But in any case:

I'm not sure how build runner uses this dependency graph - we might need to prune it?

Some builders will generate code for all packages in the whole dependency graph, so we could do extra work here if we see deps that are never used.

The only way I can see to do that would be to parse pubspec.yaml of the current package and all its dependencies transitively.

Right, this is what we do today, we parse all transitive pubspec.yaml files. We want to know the entire dependency structure including cycles, as it influences the order in which we build things.

Problem is, that with findPackageConfig we don't get a package config dir. Only the package config object (that doesn't expose it's location if I understand right).

Oh, ok then this code makes sense. Is there a different API that just gives us the location?

Is this a new problem? Did we know the ordering before?

Answered above, we crawl transitive pubspecs.

[sigurdm]

Answered above, we crawl transitive pubspecs.

Ah - then there should be no problem if we just start from the pubspec in packagePath (which we still do with this change) the workspace-wide package config and lockfile will just contain a superset of the needed packages.

Is there a different API that just gives us the location?

I don't think so. That's why I wrote it.

[jakemac53]

Ah ok, if we are still reading the actual package pubspec and not a root one, this is probably all fine. As long as we handle the case where there are more packages in the package config than we can find in the pubspecs.

[sigurdm]

If I read this code right: https://github.com/dart-lang/build/blob/db5180c903b27fd1c3d344c5b22c396540ea0c78/build_runner_core/lib/src/package_graph/package_graph.dart#L133-L144

The package graph will contain all packages in the workspace-wide resolution, but will have the correct root. And if I'm not wrong, only the reachable packages are handled according to:

build/build_runner/lib/src/build_script_generate/build_script_generate.dart

Line 82 in 5eb5d9b

final orderedPackages = stronglyConnectedComponents<PackageNode>(

[sigurdm]

I added a test of the package graph creation.

And support for .packages.

[sigurdm]

Not sure what goes wrong in the community test. I am not able to reproduce the failures locally.

[jakemac53]

Not sure what goes wrong in the community test. I am not able to reproduce the failures locally.

I can look into that, but probably not until next week. Or possibly @simolus3 would have some idea, but it seems like the code just isn't getting generated for some reason.

[simolus3]

The failure looks surprising to me as well. I commit generated code into the repository and the build integrity test shows that re-running build_runner does not alter the state of the checkout. So the generated code should be there.

My best guess is that we hit a race condition where the build integrity test started running (and thus deleting previous generated sources) while the other tests were being loaded, thus causing these errors? I have disabled concurrency for the community tests in the drift repository, I hope that resolves the problem.

[jakemac53]

I have disabled concurrency for the community tests in the drift repository, I hope that resolves the problem.

I restarted the job and it passed 👍

[sigurdm]

Anything more blocking this?

[jonasfj]

This is really another good case for adding the dependency types and relationships in package_config.json. That would much parsing all pubspec.yaml files unnecessary in this case.

[sigurdm]

This is really another good case for adding the dependency types and relationships in package_config.json. That would much parsing all pubspec.yaml files unnecessary in this case.

Yeah - lets do that another time!

dart-lang/pub#3795

[sigurdm]

Gentle ping