build_runner can fail after reading an asset outside the asset graph

[natebosch]

Bug was introduced here: fbf0e26#diff-7a296c2aaedb49f2b91b7fa9c81b536cL538

We always allow reads of assets outside the graph.

I think what should happen is we should disallow reads outside the graph and make sure all files from all packages are in the graph.

This caused a failure in json_serializable

[jakemac53]

Conceptually to me it doesn't make sense to have assets that aren't used in the build be a part of the graph? We don't want to increase the size of the graph unnecessarily.

That being said, we also shouldn't just blindly be returning true if the asset doesn't exist in the graph, we should validate that it exists on disk at the minimum.

[natebosch]

I think it makes sense to include any file that may be read in the graph. It makes me nervous to have some ways where the graph can change dynamically since it makes lazy execution riskier.

We could probably find a way to make that work if we needed to but IMO it isn't critical. For any builder using resolvers the entire import graph already ends up in the serialized graph. The set of builders or new assets that this will cause us to include should be minimal.

[jakemac53]

It makes me nervous to have some ways where the graph can change dynamically since it makes lazy execution riskier.

This shouldn't affect lazy execution since it only applies to non-primary inputs. Is there something specific you are worried about here?

We could probably find a way to make that work if we needed to but IMO it isn't critical. For any builder using resolvers the entire import graph already ends up in the serialized graph. The set of builders or new assets that this will cause us to include should be minimal.

I agree its not critical for now, but note that this will also include non-dart assets, which would have been skipped before. I doubt this will really matter for the majority use case, but there may be some pathological cases where people have a lot of static assets where it causes issues.

[natebosch]

I agree its not critical for now, but note that this will also include non-dart assets, which would have been skipped before.

With DDC every package will end up with a BuildAction so every asset will get included then - including non-dart assets. If we want to make the graph more minimal after that we'd have to do filtering based on the InputSets from the actions.

I'm open to splitting the graph into a static component and a dynamic component or looking at other way we can prune it before serialization.

[jakemac53]

With DDC every package will end up with a BuildAction so every asset will get included then - including non-dart assets. If we want to make the graph more minimal after that we'd have to do filtering based on the InputSets from the actions.

DDC should only need the .dart assets, but its true that the current logic just globs everything. We could be smarter about this in the future though.