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

Invalidate the build based on external dependency changes #967

Open
nshahan opened this issue Feb 6, 2018 · 17 comments · Fixed by #1009
Open

Invalidate the build based on external dependency changes #967

nshahan opened this issue Feb 6, 2018 · 17 comments · Fixed by #1009
Labels
P1 A high priority bug; for example, a single project is unusable or has many test failures package:build_runner type-enhancement A request for a change that isn't a bug

Comments

@nshahan
Copy link
Contributor

nshahan commented Feb 6, 2018

I ran into an issue where running build_runner build/serve was crashing due to an analyzer error (stack trace below). @jakemac53 correctly diagnosed this as a cached summary that needed to be rebuilt.

Is there some way that this can be detected and handled automatically? If not, can it be detected and given a better error message that doesn't include the message from dartdevc about a bug in the compiler?

[SEVERE] Instance of 'DevCompilerBuilder' on angular_components|lib/src/material_tree/material_tree_dropdown.template.module:
Error compiling dartdevc module:angular_components|lib/src/material_tree/material_tree_dropdown.template.ddc.js

We're sorry, you've found a bug in our compiler.
You can report this bug at:
    https://github.com/dart-lang/sdk/issues/labels/area-dev-compiler
Please include the information below in your report, along with
any other information that may help us track it down. Thanks!
    dartdevc arguments:
.
.
.
AnalysisException: Cannot compute LIBRARY_ELEMENT for packages/angular_components/src/material_tree/material_tree_dropdown.template.dart
Caused by Unexpected exception while performing BuildDirectiveElementsTask for source packages/angular_components/src/material_tree/material_tree_dropdown.template.dart
#0      AnalysisTask._safelyPerform (package:analyzer/task/model.dart:333)
#1      AnalysisTask.perform (package:analyzer/task/model.dart:220)
#2      AnalysisDriver.performWorkItem (package:analyzer/src/task/driver.dart:287)
#3      AnalysisDriver.computeResult (package:analyzer/src/task/driver.dart:112)
#4      AnalysisContextImpl.computeResult (package:analyzer/src/context/context.dart:719)
#5      AnalysisContextImpl.computeLibraryElement (package:analyzer/src/context/context.dart:687)
#6      ModuleCompiler.compile (package:dev_compiler/src/analyzer/module_compiler.dart:184)
#7      _compile (package:dev_compiler/src/analyzer/command.dart:187)
#8      compile (package:dev_compiler/src/analyzer/command.dart:57)
#9      _CompilerWorker.performRequest (file:///b/build/slave/dart-sdk-mac-dev/build/sdk/pkg/dev_compiler/bin/dartdevc.dart:43)
<asynchronous suspension>
#10     AsyncWorkerLoop.run.<anonymous closure> (package:bazel_worker/src/worker/async_worker_loop.dart:33)
#11     _rootRun (dart:async/zone.dart:1126)
#12     _CustomZone.run (dart:async/zone.dart:1023)
#13     runZoned (dart:async/zone.dart:1501)
#14     AsyncWorkerLoop.run (package:bazel_worker/src/worker/async_worker_loop.dart:33)
<asynchronous suspension>
#15     main (file:///b/build/slave/dart-sdk-mac-dev/build/sdk/pkg/dev_compiler/bin/dartdevc.dart:23)
<asynchronous suspension>
#16     _startIsolate.<anonymous closure> (dart:isolate-patch/dart:isolate/isolate_patch.dart:276)
#17     _RawReceivePortImpl._handleMessage (dart:isolate-patch/dart:isolate/isolate_patch.dart:164)

Caused by RangeError (index): Invalid value: Not in range 0..81, inclusive: 82
#0      List.[] (dart:core-patch/dart:core/array.dart:10)
#1      _FbGenericList.[] (package:front_end/src/base/flat_buffers.dart:824)
#2      _LibraryResynthesizerContext.buildImportedLibrary (package:analyzer/src/summary/resynthesize.dart:1105)
#3      ImportElementImpl.importedLibrary (package:analyzer/src/dart/element/element.dart:5779)
#4      LibraryElementImpl.importedLibraries (package:analyzer/src/dart/element/element.dart:6489)
#5      LibraryElementImpl.invalidateLibraryCycles.invalidate (package:analyzer/src/dart/element/element.dart:6913)
#6      List.forEach (dart:core-patch/dart:core/array.dart:79)
#7      LibraryElementImpl.invalidateLibraryCycles.invalidate (package:analyzer/src/dart/element/element.dart:6913)
#8      List.forEach (dart:core-patch/dart:core/array.dart:79)
#9      LibraryElementImpl.invalidateLibraryCycles.invalidate (package:analyzer/src/dart/element/element.dart:6912)
#10     LibraryElementImpl.invalidateLibraryCycles (package:analyzer/src/dart/element/element.dart:6917)
#11     BuildDirectiveElementsTask.internalPerform (package:analyzer/src/task/dart.dart:1222)
#12     AnalysisTask._safelyPerform (package:analyzer/task/model.dart:321)
#13     AnalysisTask.perform (package:analyzer/task/model.dart:220)
#14     AnalysisDriver.performWorkItem (package:analyzer/src/task/driver.dart:287)
#15     AnalysisDriver.computeResult (package:analyzer/src/task/driver.dart:112)
#16     AnalysisContextImpl.computeResult (package:analyzer/src/context/context.dart:719)
#17     AnalysisContextImpl.computeLibraryElement (package:analyzer/src/context/context.dart:687)
#18     ModuleCompiler.compile (package:dev_compiler/src/analyzer/module_compiler.dart:184)
#19     _compile (package:dev_compiler/src/analyzer/command.dart:187)
#20     compile (package:dev_compiler/src/analyzer/command.dart:57)
#21     _CompilerWorker.performRequest (file:///b/build/slave/dart-sdk-mac-dev/build/sdk/pkg/dev_compiler/bin/dartdevc.dart:43)
<asynchronous suspension>
#22     AsyncWorkerLoop.run.<anonymous closure> (package:bazel_worker/src/worker/async_worker_loop.dart:33)
#23     _rootRun (dart:async/zone.dart:1126)
#24     _CustomZone.run (dart:async/zone.dart:1023)
#25     runZoned (dart:async/zone.dart:1501)
#26     AsyncWorkerLoop.run (package:bazel_worker/src/worker/async_worker_loop.dart:33)
<asynchronous suspension>
#27     main (file:///b/build/slave/dart-sdk-mac-dev/build/sdk/pkg/dev_compiler/bin/dartdevc.dart:23)
<asynchronous suspension>
#28     _startIsolate.<anonymous closure> (dart:isolate-patch/dart:isolate/isolate_patch.dart:276)
#29     _RawReceivePortImpl._handleMessage (dart:isolate-patch/dart:isolate/isolate_patch.dart:164)


#0      AnalysisContextImpl.computeResult (package:analyzer/src/context/context.dart:724)
#1      AnalysisContextImpl.computeLibraryElement (package:analyzer/src/context/context.dart:687)
#2      ModuleCompiler.compile (package:dev_compiler/src/analyzer/module_compiler.dart:184)
#3      _compile (package:dev_compiler/src/analyzer/command.dart:187)
#4      compile (package:dev_compiler/src/analyzer/command.dart:57)
#5      _CompilerWorker.performRequest (file:///b/build/slave/dart-sdk-mac-dev/build/sdk/pkg/dev_compiler/bin/dartdevc.dart:43)
<asynchronous suspension>
#6      AsyncWorkerLoop.run.<anonymous closure> (package:bazel_worker/src/worker/async_worker_loop.dart:33)
#7      _rootRun (dart:async/zone.dart:1126)
#8      _CustomZone.run (dart:async/zone.dart:1023)
#9      runZoned (dart:async/zone.dart:1501)
#10     AsyncWorkerLoop.run (package:bazel_worker/src/worker/async_worker_loop.dart:33)
<asynchronous suspension>
#11     main (file:///b/build/slave/dart-sdk-mac-dev/build/sdk/pkg/dev_compiler/bin/dartdevc.dart:23)
<asynchronous suspension>
#12     _startIsolate.<anonymous closure> (dart:isolate-patch/dart:isolate/isolate_patch.dart:276)
#13     _RawReceivePortImpl._handleMessage (dart:isolate-patch/dart:isolate/isolate_patch.dart:164)
@nshahan nshahan added type-enhancement A request for a change that isn't a bug package:build_runner labels Feb 6, 2018
@natebosch
Copy link
Member

Two possibilities I can think of:

  • An "environment check" (Allow builder authors to define custom "environment checks" #961) that checks the analyzer version and can somehow communicate that the summaries are invalid (maybe change an artificial "input" to the summary builders?)
  • When the builder runs catch this specific exception and somehow communicate back that the inputs it read are invalid.

Either of these would require new APIs.

@jakemac53
Copy link
Contributor

This is more a general issue with using external dependencies I think. Today if you are using any external executable and invoking it inside a scratch space we don't know if/when that external executable changes.

In this case, its even more complicated because I believe the the dartanalyzer executable is actually just a shell script that runs the analyzer snapshot with the dart vm. We would need to track the dart vm itself, plus the snapshot and shell script. Computing hashes of some of these larger files would also likely be slow.

We could also just invalidate builds whenever the dart sdk version changes (add that as an encoded field on the asset graph). This would solve things for analyzer/dartdevc but not other external executables.

@matanlurey
Copy link
Contributor

matanlurey commented Feb 7, 2018

Could builders expose an optional externalDependencies?

class UsesDartSdkBuilder extends Builder {
  // Used as part of the Builder digest?
  @override
  Future<List<String>> get externalDependencies async => [
    _getDartSdkVersion(),
  ];
}

Optionally, I'd like to see some sort of easily-consumable pattern for a builder that executes via IPC so others can write stuff using other external dependencies more easily, maybe that can simplify this?

@matanlurey matanlurey changed the title Make sure summaries are invalidated between SDK and analyzer version changes. Invalidate the build based on external dependency changes Feb 7, 2018
@matanlurey matanlurey added the P1 A high priority bug; for example, a single project is unusable or has many test failures label Feb 7, 2018
@jakemac53 jakemac53 reopened this Feb 13, 2018
@jakemac53
Copy link
Contributor

leaving this open, we do track dart sdk versions now but a more general purpose thing would be better

@natebosch
Copy link
Member

After some discussion we think we could solve this by allowing you to configure an environment_check per builder definition in build.yaml. This would be the name of a top-level or static method conforming to the signature Digest Function(). We'd call this once per build (if that builder has any actions possible). We'd guarantee that the log variable from package:build is available, so you could log.severe if there is a missing binary or anything like that, and the Digest you return would be an expression of whatever dependencies you have (tool version information, files you might read non-hermetically with dart:io once per build, etc).

We could migrate our tools that are dependent on the SDK version to use this and be a bit more optimal than today. It would also satisfy both the environment check and build invalidation problems.

@jakemac53
Copy link
Contributor

jakemac53 commented Jun 5, 2018

Presumably we would not attempt the build if you log.severe? Or should we require a throw? Or use the setting for --fail-on-severe?

@natebosch
Copy link
Member

I think we'd not attempt the build (or maybe attempt the rest of the build but mark the actions using that builder as failures without trying to run them) with either log.severe or a throw. I'd not bother honoring the setting for --fail-on-severe since it's a new API and we want to drop that flag.

@jakemac53
Copy link
Contributor

or maybe attempt the rest of the build but mark the actions using that builder as failures without trying to run them

Interesting idea... we could also just require the user to disable the builder explicitly (but would need global config). My only real concern with either of those approaches is you may very well get a lot of cascading build failures from steps that try to read those files.

@natebosch
Copy link
Member

Yeah the tradeoff is between the two sharp edge scenarios: "Why did you bother running this build when it's all going to fail" vs "Why can't I just run the build and get DDC output to test things even though this inconsequential other thing is going to fail"

@frencojobs
Copy link

Could builders expose an optional externalDependencies?

class UsesDartSdkBuilder extends Builder {
  // Used as part of the Builder digest?
  @override
  Future<List<String>> get externalDependencies async => [
    _getDartSdkVersion(),
  ];
}

Optionally, I'd like to see some sort of easily-consumable pattern for a builder that executes via IPC so others can write stuff using other external dependencies more easily, maybe that can simplify this?

Any update on the API like this? I'm building a package that will generate source code for a dart file using the data from the .env file. So, I need to invalidate the cache if not only the .dart file but also when the .env file is changed. I really need this API. I'm having to manually clean the snapshot when the .env file is updated.

@jakemac53
Copy link
Contributor

Does this .env file live outside the package?

@frencojobs
Copy link

Yes, it does. But with #2851 it won't be a problem, right?

@jakemac53
Copy link
Contributor

That PR doesn't allow you to reach outside the package, it just allows you to expose files outside the lib dir of your package to other packages.

@frencojobs
Copy link

Okay, I think I misunderstood the PR then. But may I know why does it matter? Because, as long as there is an API like the suggested one, I can still make a function using dart:io to read the file, which is how I currently implemented.

I just need a way to invalidate the snapshot once the specified asset has been modified.

@jakemac53
Copy link
Contributor

Right, if you use dart:io then the build system doesn't track that and won't invalidate things. You will have to somehow manage that.

In an ideal world this would not mean deleting the entire build cache, as only a few actions are likely affected, and also ideally the user themselves wouldn't have to do something manually here.

@frencojobs
Copy link

Okay, I think I see your point. If I could read the value of .env with buildStep.readAsString, it would solve the problem. But in return, I might have to move the .env into the lib so that it can be read with an assetId. Right?

But is it possible to have a workaround for reading the .env file outside of the lib? It really can't be moved into the lib.

@jakemac53
Copy link
Contributor

There is not a real solution for what you want to do today, the closest thing would be using a symlink to the file in your package, but I don't think the file watcher would work so you would have to restart the build whenever that file changed, at which point we should see that it has been modified I think.

Note that once the pull request you linked above is published you could move the file outside of lib but it would still have to live under the package directory.

One potential workaround although I have no idea if it would be possible for your use case, is to make the .env file be a part of an actual package (but not your root package). You would then depend on that package wherever you want to use that file, using a path override:

dependencies:
  env:
    path: /some/place/env

And then you could construct an AssetId to read that file?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P1 A high priority bug; for example, a single project is unusable or has many test failures package:build_runner type-enhancement A request for a change that isn't a bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants