Split post process builder state out of the main graph. #3959
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
PR HealthChangelog Entry ✔️
Changes to files need to be accounted for in their respective changelogs. |
davidmorgan
force-pushed
the
remove-anchor-nodes
branch
from
0f0d0b2 to
27729e4
Compare
davidmorgan
changed the title
davidmorgan
force-pushed
the
remove-anchor-nodes
branch
4 times, most recently
from
d88218e to
6f58924
Compare
davidmorgan
force-pushed
the
remove-anchor-nodes
branch
from
6f58924 to
dacef53
Compare
Merged
jensjoha
approved these changes
Apr 11, 2025
| /// Created with empty outputs at the start of the build if it's a new build | ||
| /// step; or deserialized with previous build outputs if it has run before. | ||
| final Map<String, Map<PostProcessBuildStepId, Set<AssetId>>> | ||
| _postProcessBuildStepOutputs = {}; |
There was a problem hiding this comment.
is this really the indentation provided by dart format? (also below)
There was a problem hiding this comment.
It is ... formatting gets a bit surprising when there are very long identifiers.
There's a format presubmit :) that's checked before the tests run.
| buildStepId.input.package, | ||
| () => {}, | ||
| )[buildStepId] = | ||
| outputs; |
There was a problem hiding this comment.
you must just have forgotten to format it right?
There was a problem hiding this comment.
Nope, this is the output :)
| @@ -67,6 +67,19 @@ class _AssetGraphDeserializer { | |||
| graph._add(_deserializeAssetNode(serializedItem as List)); | |||
| } | |||
|
|
|||
| final postProcessOutputs = | |||
There was a problem hiding this comment.
(this frankly looks weird too, but I'll stop commenting on formatting now :x)
There was a problem hiding this comment.
Yeah, it's the long Map<String, ....
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
For #3811.
Per the docs on PostProcessBuilder, post process build steps interact in a very limited way with the rest of the build.
So, split them out.
built_valueserialization doesn't support serializing mutableMaporSetout of the box, but I don't particularly want to useBuildMap/BuiltSetfor these as there's no advantage to having them be immutable. So add the code for serializingMap+Sethere, I'll upstream it.An explanation of the
FullTypeboilerplate added:built_valuedifferentiates between generic types known at compile time and at runtime. If the types are known at compile time they're specified with aFullTypeand don't get written on the wire. So:serializers.serialize([1, 2, 3], specifiedType: FullType(List, [FullType(int)])gives you back JSON that is minimal,[1,2,3]. If the types are not known,serializers.serialize([1, 2, 3])then the types do get written on the wire:['list', ['int', 1], ['int', 2], ['int', 3]]. This works for arbitrarily nested generics, for example a list of lists of int.The "builderFactories" are used on deserializing to instantiate the types with the right generics--as without mirrors there is no other way to do this in Dart. For fields in serializable types
built_valuecan generate code to add builder factories, but here I'm just serializing collections directly so codegen can't help.Sorry for the complexity here! It would also work to handcode the new (de)serialization, I did that first, and can switch to that or something like it if you prefer. Both options are not pretty--but I have a slight preference for going fully with codegen/built_value as that seems better long term. We can improve some rought edges in built_value :) and the community can benefit as a result.
Thanks.