Skip to content

NoSuchMethodError from package:build_runner_core/src/package_graph/package_graph.dart 74:15 PackageGraph.forPath #41913

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

Closed
devoncarew opened this issue May 6, 2020 · 29 comments
Labels
area-vm Use area-vm for VM related issues, including code coverage, and the AOT and JIT backends. closed-duplicate Closed in favor of an existing report

Comments

@devoncarew
Copy link
Member

We're seeing this failure on the DevTools bots:

NoSuchMethodError from package:build_runner_core/src/package_graph/package_graph.dart 74:15 PackageGraph.forPath

With the possible root cause being in package:package_config?

https://travis-ci.org/github/flutter/devtools/jobs/684021378#L591

You have hit a bug in build_runner
Please file an issue with reproduction steps at https://github.com/dart-lang/build/issues
NoSuchMethodError: Closure call with mismatched arguments: function 'MutablePackageTree.allPackages'
Receiver: Closure: (_SyncIterator<Package>) => bool
Tried calling: MutablePackageTree.allPackages()
Found: MutablePackageTree.allPackages(_SyncIterator<Package>) => bool
dart:core                                                              _SyncIterable.iterator
package:package_config/src/package_config_impl.dart 26:36              new SimplePackageConfig
package:package_config/src/package_config_json.dart 213:10             parsePackageConfigJson
package:package_config/src/package_config_json.dart 45:10              parsePackageConfigBytes
package:package_config/src/package_config_io.dart 117:10               readPackageConfigJsonFile
package:package_config/src/discovery.dart 109:18                       findPackagConfigInDirectory
package:package_config/src/discovery.dart 44:31                        findPackageConfig
package:package_config/package_config.dart 114:5                       findPackageConfig
package:build_runner_core/src/package_graph/package_graph.dart 74:15   PackageGraph.forPath
package:build_runner_core/src/package_graph/package_graph.dart 110:20  PackageGraph.forThisPackage
package:build_runner/src/entrypoint/run.dart 22:64                     run
.dart_tool/build/entrypoint/build.dart 96:22                           main
@devoncarew
Copy link
Member Author

This is when running flutter pub run build_runner build -o web:build --release; it's for the latest flutter at master, and it seems to be consistent on the bot.

@devoncarew
Copy link
Member Author

cc @jakemac53

@devoncarew
Copy link
Member Author

This reproduces at flutter master, but not on the flutter from this morning.

@devoncarew
Copy link
Member Author

It looks like it was this engine roll to flutter: flutter/flutter@3a67a8b .

@devoncarew
Copy link
Member Author

devoncarew commented May 6, 2020

We were on build_test 0.10.0, but evening upgrading to 1.0.0 We see the same exception:

NoSuchMethodError: Closure call with mismatched arguments: function 'MutablePackageTree.allPackages'
Receiver: Closure: (_SyncIterator<Package>) => bool
Tried calling: MutablePackageTree.allPackages()
Found: MutablePackageTree.allPackages(_SyncIterator<Package>) => bool
dart:core                                                              _SyncIterable.iterator
package:package_config/src/package_config_impl.dart 26:36              new SimplePackageConfig
package:package_config/src/package_config_json.dart 213:10             parsePackageConfigJson
package:package_config/src/package_config_json.dart 45:10              parsePackageConfigBytes
package:package_config/src/package_config_io.dart 117:10               readPackageConfigJsonFile
package:package_config/src/discovery.dart 109:18                       findPackagConfigInDirectory
package:package_config/src/discovery.dart 44:31                        findPackageConfig
package:package_config/package_config.dart 114:5                       findPackageConfig
package:build_runner_core/src/package_graph/package_graph.dart 74:15   PackageGraph.forPath
package:build_runner_core/src/package_graph/package_graph.dart 110:20  PackageGraph.forThisPackage
package:build_runner/src/entrypoint/run.dart 22:64                     run
.dart_tool/build/entrypoint/build.dart 96:22                           main

@jakemac53 jakemac53 transferred this issue from dart-lang/build May 6, 2020
@jakemac53
Copy link
Contributor

This looks like something internal failing when we call findPackageConfig, moved to that repo.

@devoncarew
Copy link
Member Author

Possibly related to this CL: https://dart-review.googlesource.com/c/sdk/+/145402

@ghost ghost unassigned lrhn May 7, 2020
@ghost
Copy link

ghost commented May 7, 2020

For completeness, let me repost what I just posted on the email thread:

Yes, this is likely due to the sync* transform in 145402.
This kind of error happens when you're running an older platform kernel on the new VM which is incompatible due to the changes.

The issue is mixing versions, so ultimately I don't think this is a Dart issue.
We should track down where the versions are being mixed and preferably update the pre-built kernels for the new version.

As an aside, this error is a bit less informative because the change landed without bumping the ABI version.
Liam stepped in and bumped the version separetely shortly after in 1ef44413 but it seems Flutter Engine rolled in between the two CLs, and Flutter in turn rolled that.
Of course even if the latter CL had been included, the change still makes older kernels incompatible, so a similar setup should instead have failed with a message saying this..

@devoncarew
Copy link
Member Author

@jonahwilliams - for the cache that flutter uses (in .dart_tool/flutter_build ?), does it use the sdk version as a signal to flush the cache? It sounds like the kernel artifacts aren't guaranteed to be compatible between VM versions.

@ghost
Copy link

ghost commented May 7, 2020

I'm not too familiar with Flutter and Engine's rolls, but it might be worth trying a new roll to include 1ef444139c4ce8a69b9772a600be1794b86d5c9a.
There are various scripts and bits of code that reacts to the ABI_VERSION so getting that in sync might make the other issue go away.

@jakemac53
Copy link
Contributor

In build_runner it looks like we rely on getting an IsolateSpawnException if the kernel versions don't match up for a previous snapshot.

So if that failed to happen as we expected then that is likely what is causing this issue?

@jakemac53
Copy link
Contributor

Given that this was just at flutter master and not a dev/stable release I think we just wait for the next roll and assume that will fix it?

@jonahwilliams
Copy link
Contributor

The flutter tool doesn't manage .dart_tool/build/entrypoint/build.dart if you're using pub run

@kevmoo
Copy link
Member

kevmoo commented May 13, 2020

@cskau-g – should there be an issue in the SDK repo for this?

@jakemac53
Copy link
Contributor

It looks like the latest dev sdk does include 1ef4441 but this issue just came up again with that release :(.

@ghost
Copy link

ghost commented May 15, 2020

@cskau-g – should there be an issue in the SDK repo for this?

I'm not sure I'm the best person to say. But I suppose if this issue persists and we suspect it isn't simply a matter of updating some build scripts or pre-built files somewhere, then it should probably be tracked up-stream.

I'm surprised though that you keep seeing the same error even after 1ef4441 has rolled, since I'd expect that to trigger a different error if versions were being mixed.
Among my upcoming projects is tightening kernel versioning, so if you have a reproducible example I can have a look?

@devoncarew devoncarew transferred this issue from dart-archive/package_config May 15, 2020
@devoncarew
Copy link
Member Author

Moved to dart-lang/sdk. If we continue to see this we should investigate.

It sounds like we may need to:

  • ensure that kernel version needs to be rev'd when there are breaking changes?
  • embedding SDK version strings into Kernel files to prevent version skew from happening?
  • ensure we throw an IsolateSpawnException when trying to run a kernel file with an out-of-date snapshot?

@srawlins srawlins added legacy-area-front-end Legacy: Use area-dart-model instead. front-end-kernel labels May 15, 2020
@devoncarew
Copy link
Member Author

We see this on the stagehand CI as well: https://travis-ci.org/github/dart-lang/stagehand/jobs/687544226

@johnniwinther johnniwinther removed legacy-area-front-end Legacy: Use area-dart-model instead. front-end-kernel labels May 18, 2020
@srawlins
Copy link
Member

@johnniwinther suggested this may be a VM issue, w.r.t. the version skew of package_config.

@srawlins srawlins added the area-vm Use area-vm for VM related issues, including code coverage, and the AOT and JIT backends. label May 19, 2020
@a-siva
Copy link
Contributor

a-siva commented May 19, 2020

@devoncarew do you have reproduction instructions for the above? the link you have doesn't seem to show any errors.

@devoncarew
Copy link
Member Author

No repro instructions, but we have seen this on three separate CIs now. The link in the 1st comment does still show the issue.

@a-siva
Copy link
Contributor

a-siva commented May 19, 2020

The first link shows the "Closure call with mismatched arguments" error which @cskau-g says was expected because of mismatched kernel files being used.
The final comment from @srawlins indicates this is a VM issue in response to your comment about stagehand CI, I am trying to understand what is the VM issue and how to reproduce it.

@mkustermann
Copy link
Member

@jakemac53 @natebosch Does package:build_runner_core or any other package involved in running tests cache kernel / .dill files?
@sigurdm @jonasfj Does pub cache kernel / .dill files?

We see this on the stagehand CI as well: https://travis-ci.org/github/dart-lang/stagehand/jobs/687544226

That link points to a green build.

It sounds like we may need to:
ensure that kernel version needs to be rev'd when there are breaking changes?
embedding SDK version strings into Kernel files to prevent version skew from happening?
ensure we throw an IsolateSpawnException when trying to run a kernel file with an out-of-date
snapshot?.

See #41802 . @cskau-g is drafting a design for this atm.

We maintain currently no compatibility between SDK versions. The kernel binary version is just a version for the AST encoding. Though the VM does requires stronger guarantees than to just be able to parse the files, it also expects e.g. certain transformers to be run, etc.

@jonasfj
Copy link
Member

jonasfj commented May 20, 2020

pub run ... creates source snapshots, but we don't track which SDK version created. We just rebuild the snapshot if using it fails.

@jakemac53
Copy link
Contributor

pub run ... creates source snapshots, but we don't track which SDK version created. We just rebuild the snapshot if using it fails.

This is exactly what build_runner does as well. We rely on the IsolateSpawnException.

@mkustermann
Copy link
Member

This is exactly what build_runner does as well. We rely on the IsolateSpawnException.

That is unsafe atm. If we do lock-step changes to VM and kernel transformers they have to match. This is not validated at runtime until #41802 is addressed.

(/cc @mraleph Proofs the point we should really embed the SDK hash in the kernel files.)

@jakemac53
Copy link
Contributor

If running a snapshot from a different sdk version has potentially undefined semantics then I would argue that it should detect this itself and throw before attempting to run? This has been the behavior for many years, and existing tools do rely on it.

@a-siva a-siva added the closed-duplicate Closed in favor of an existing report label May 20, 2020
@a-siva
Copy link
Contributor

a-siva commented May 20, 2020

The title of the bug does not match the issue being reported, looks like this issue is a duplicate of
#38969 and
#41802
Marking as such.

@a-siva a-siva closed this as completed May 20, 2020
@devoncarew
Copy link
Member Author

I think we should bump the priorities of those other issues, given that we've seen more problems related to this recently. Sounds like those two issues are the best way to make progress here; I'll bump them to P1s.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-vm Use area-vm for VM related issues, including code coverage, and the AOT and JIT backends. closed-duplicate Closed in favor of an existing report
Projects
None yet
Development

No branches or pull requests

10 participants