Build: Sharing .dart_tool in CI

[todbachman-wf]

We currently share the .dart_tool directory between CI systems in order to speed up overall CI times as well as reduce the resource load on those systems. Sharing the .dart_tool directory is easier for us to do in a generic way than compiling to one or more build directories and sharing them. Sharing the .dart_tool directory has also typically sped up pub gets for us, although to be honest I'm not sure how much we benefit from that anymore.

This is a sketch of how we've been doing this:

• CI Build System:
  
  
  
  • pub get
  
  
  • pub run build_runner build
  
  
  • pub run build_runner build --release
  
  
  • tar czf dart_tool.tgz --exclude='*.snapshot' .dart_tool pubspec.lock
  
  
• CI Test System (executes the runs below in parallel):
  
  
  
  • DDC unit test run:
  
  
  • tar xzf dart_tool.tgz
  
  
  • pub get
  
  
  • pub run build_runner test
  
  
  • Dart2Js unit test run:
  
  
  • tar xzf dart_tool.tgz
  
  
  • pub get
  
  
  • pub run build_runner test --release
  
  

By reusing the .dart_tool directory from our Build system in our unit test runs in our Test system, those unit test runs don't have to recompile the code again. For us the compilation step of running unit tests can be fairly resource intensive and so avoiding that means we can reduce the memory and CPU requirements for our test system instances. Since those configs don't compile the code they also run faster and free up test instances sooner and mean we don't have to have as many test instances available in our pool.

Unfortunately for us, the 4.5.0 release of build_runner_core resulted in this approach breaking down. Specifically, if the package_config.json file changes it invalidates the builds. That file contains absolute paths which are not the same in our Build and Test systems, so we wouldn't be able to reuse that file between those systems. I believe this is causing all of our builds to now be invalidated and we're doing full builds in our unit test runs.

So, I'm wondering if it might be possible for you all to provide a way to more easily share the .dart_tool directory across systems. Or perhaps you can suggest a work around for us for the specific issue we've now hit with the package_config.json invalidating our builds. Or do you have a better approach you would suggest that might be equally easy to implement in a generic way?

Environment:

• Dart SDK Version: 2.7.1
• What package(s) from this repo you are using:
  
  
  
  • build_runner: 1.8.0
  
  
  • build_runner_core: 4.5.1
  
  

[jakemac53]

Hmmm, ya the absolute paths in there are unfortunate. I have been seeing similar issues on our travis runs as well.

What we actually care about is specifically the languageVersion fields in that file - originally we wanted to avoid trying to customize the invalidation for this and rely on the built in mechanisms but those are limited to just general content hashes.

For now I would recommend restricting your builds to an older version of build_runner_core which doesn't have this check - its only necessary if you want to start opting into NNBD or other language experiments.

[jakemac53]

cc @natebosch

[yury-yufimov]

I can add, that treating package_config.json as internal source results in difficulties even for local development.
Our project consists of many packages. Sometimes you need to develop in several libraries simultaneously. Imagine I have project with pubspec.yaml:

dependencies:
  my_lib: ^1.0.0
dependency_overrides:
  my_lib:
    git:
      url: [email protected]:my_lib.git
      ref: branch_for_development

After I've committed some changes in my_lib and perform pub upgrade, I can't restart build:
package_config.json contains commitRef in path to my_lib, and it was changed.

[jakemac53]

The fix for this has been published, please try it out and let us know if it doesn't resolve things.

[todbachman-wf]

@jakemac53 I'm still seeing the build invalidated with the latest set of packages (build_runner 1.8.1 and build_runner_core 5.0.0). We get the following message during the build:

[WARNING] Throwing away cached asset graph because the build phases have changed. This most commonly would happen as a result of adding a new dependency or updating your dependencies.

We tar up the .dart_tool directory and pubspec.lock like this:

tar czf dart_tool.tgz --exclude='*.snapshot' .dart_tool pubspec.lock

Maybe there is something more we should be excluding?

I haven't done much to debug this yet. Is there something in particular you think I should investigate?

[jakemac53]

Hmm, the build phases changing might have been a red herring - that should be a separate thing. Is it possible some other package happened to update between those two runs?

[todbachman-wf]

As far as I can tell, none of the dependencies changed. After inflating the tarball, I made a backup of the pubspec.lock file, ran pub get and then compared the updated pubspec.lock with the backed up version and they were identical.

[jakemac53]

I am not sure the right way to investigate this, we are seeing the proper behavior in our travis now (example incremental build here https://travis-ci.org/github/dart-lang/build/jobs/669869528#L313).

That message you are seeing is a different thing and essentially indicates either a new builder being added, a change to a build targets sources configuration, a new/deleted build target, or some package dependency changing.

[todbachman-wf]

@jakemac53 I finally had time to debug this issue and I think I found the source of the problem. The computed build phase digest is different between our two CI systems and it is invalidating the builds. I noticed the digest for one of those systems matched what was computed on my local machine, which helped me track down the source of the issue.

I verified the build phase digest is dependent on the order of the build phases. By adding some logging to the builds I was able to output the build phases on both CI systems. They appeared to be the same except for the order of the phases. So I focused in on how the build phases are computed and what affects the order of them.

I walked up the stack from the createBuildPhases method:

https://github.com/dart-lang/build/blob/master/build_runner_core/lib/src/package_graph/apply_builders.dart#L253

to find that eventually the order of the build phases is dependent on the order of PackageConfig.packages. The PackageConfig constructor we're using is here:

https://github.com/dart-lang/package_config/blob/master/lib/src/package_config_impl.dart#L19-L27

You'll notice in that constructor the list of packages read from the .dart_tool/package_config.json file is sorted by the rootUri field before iterating over it to eventually populate PackageConfig.packages.

Therefore, the build phases digest is ultimately dependent on the rootUri-sorted list of packages in package_config.json. It turns out our CI systems sort differently.

On one of our CI systems the source code for the package under test is copied into a /build/ directory, while on the other the source code is copied into /testing/. On both systems the pub cache is in something like /myuser/. So, on the system we copy source code into /build/ the package being built sorts first in the list of packages, while on the other system it sorts last. That minor difference in sorting is enough to invalidate every build for us.

I have verified that, as one would expect, if we use the same directory name for our source code in both CI systems, we end up computing the same build phase digest and our builds no longer get invalidated. Hooray!

It seems that an easy fix for this issue would be to update package_config to sort the packages in a way that is not susceptible to differences in file system structure.

[natebosch]

It seems that an easy fix for this issue would be to update package_config to sort the packages in a way that is not susceptible to differences in file system structure.

I don't know if it makes sense for package_config to promise a consistent order, although if it's going to be _generally_ consistent it might be sensible to make it fully consistent to avoid surprises like this. cc @lrhn

In this case I do think the bug is on our side. Since we depend on a consistent ordering we should be enforcing it ourselves, unless package_config does decide to make that part of it's guarantee.

[natebosch]

@jakemac53 - one thing that makes this tough is that we don't currently make any guarantees to give a consistent topological sort from stronglyConnectedComponents. We probably do have one generally, but I'm not sure I want to rely on that. I'm also not sure how much we'd have to sort things to a consistent state before calling it. We might need to think carefully about this.

[lrhn]

The package_config is not deliberately ordering package entries, that's just an accident of implementation (it sorts internally to be able to easily find duplicates or super/sub path relations, not because it cares about the end result). It's not a documented order, and not something anyone should depend on.

If I needed to rely on a specific ordering of all the packages, I'd copy the list of packages and sort it myself. That's the only way to be sure.

[natebosch]

If I needed to rely on a specific ordering of all the packages, I'd copy the list of packages and sort it myself. That's the only way to be sure.

Yeah that's what we'll do.

[jakemac53]

Interesting, I agree that we should be doing the initial sorting of packages ourselves, by something consistent across machines (package name seems a good candidate).

In terms of the topological sort being potentially inconsistent that is scary but in practice I don't know that we have seen it be a problem? I would suggest that for now at least we just do the packages sorting as I don't think it can hurt anything, and it should resolve some cases.

[natebosch]

Just synced on this. One thing we have going for us here is that a consistent ordering is an optimization that allows for caching, it doesn't impact correctness. We decided we'll do a best effort to sort the packages and dependencies and assume that the other code will generally give a consistent order even if it isn't guaranteed.

[todbachman-wf]

Awesome, thanks @natebosch !

[natebosch]

Huge thanks to you @todbachman-wf for the detailed root cause!

[todbachman-wf]

@natebosch do you happen to know when this fix might be released?

[natebosch]

I had been waiting on some other changes that we hoped to roll out together, but I think those got slowed down so I'll go ahead an publish soon. I should be able to publish after https://github.com/dart-lang/build/pull/2680 goes in.

In the mean time if you'd like to try it out you should be able to use a dependency_override to pick up build_runner_core: 5.1.0 and see if this fixes the situation for you. The next publish of build_runner will use that version.

[todbachman-wf]

Oh excellent! I didn't realize 5.1.0 had been released. I'll give that a try and report back!

[natebosch]

build_runner is now published as 1.9.0. If you pub upgrade and pick up that version you'll get the fix without a dependency override.

[todbachman-wf]

Awesome! I verified that it worked with the dependency override, so I think we're good to go. Thanks for the quick fix!