build
build copied to clipboard
dart-lang
Enforce `build_extensions` config from `build.yaml` to be accurate
We don't currently read this section of the config since it all happens based on the Builder instances. We only really require this section for dazel since it's needed statically in the bazel BUILD files.
If we want to (eventually) advertise compatibility across build systems we may want to enforce accuracy here so we can have some confidence that a config that works with build_runner will also work with dazel.
Example from @kevmoo:
- Just hit weirdness where the Builder was build.g.part and the yaml config had build.g.dart. (https://github.com/dart-lang/build/issues/1686)
+1. Definitely we should document that it's expected to be in sync, and we will enforce in future.
We discussed this a little bit. There is some nuance to this decision.
We have two places extensions are configured:
- static config
- Dart code
In both build_runner and bazel (2) is a limit - this will always be true.
In bazel (1) is a minimum (and build_runner ignores it). bazel uses this to decide what is visible to dependencies.
The static config must be a subset of <All Builders in the list of factories merged outputs> +
If we want to add all the enforcement of bazel we'd also want to introduce default_content and require that every Asset is either generated, or has default content.
We'd also need to decide whether the default content should go ahead and get created (potentially wasteful) or ignored (potentially different behavior if a dependency tries to read it in a later builder).
Supporting default_content would also require some refactoring in build_runner to work like it does in _bazel_codegen.
In bazel we don't really care that we end up with a lot of useless files, in build_runner we might.
Does this overlap a bit with the work required for transient files for build_runner?
I don't believe this overlaps with work to support transient - other than transient outputs must exist in "Dart code config" and not in "static config"
I think we'd like to push for consistency, but some of this is a UX decision that @kevmoo might make differently from me. My ideal scenario is to be able to tell builder authors "If it works with build_runner today it will work with bazel tomorrow"
The questions are whether we want to start with those guarantees/restrictions, and how much waste we're willing to tolerate to enforce them.
Are the restrictions considered cumbersome? I guess I don't understand why not.
The restrictions are potentially inefficient.
It means, for instance, that every .dart file in your package must get a .template.dart file, even if the content of that file is void initReflector() {}\n, which potentially means we'll do useless work for DDC, it'll sit around in your build output directory, etc.
Similar to https://github.com/dart-lang/build/issues/907, maybe we could support something like a --strict-build-extensions which guarantees additional compatibility with Bazel?
Closing for now, we don't have the bandwidth to add extra validation for a possible future we aren't building right now. When we revisit bazel builder authors will need to do some updates and we'll build validation into the bazel integration.
See https://github.com/dart-lang/build/issues/1258
Friendly ping – hit this again. Would be great if we helped folks "do the right thing"
This is really tricky. We really want to allow some amount of fudging the extensions.
Here's a concrete example from build_web_compilers:
builderFactories:
- dart2jsMetaModuleBuilder
- dart2jsMetaModuleCleanBuilder
- dart2jsModuleBuilder
build_extensions:
$lib$:
- .dart2js.meta_module.raw
- .dart2js.meta_module.clean
.dart:
- .dart2js.module
The build extensions for each of those builders, in order, is:
-
{'$lib$': ['.dart2js.meta_module.raw']} -
{'.dart2js.meta_module.raw': ['.dart2js.meta_module.clean']} -
{'.dart': ['.dart2js.module']}
When you run those in order you expect the .dart2js.meta_module.raw to not really be it's own input - but it is. If there was some source file named lib/anything.dart2js.meta_module.raw it would be an input to that second builder. So the merged extensions for these 3 builders ends up being:
build_extensions:
$lib$:
- .dart2js.meta_module.raw
- .dart2js.meta_module.clean
.dart:
- .dart2js.module
.dart2js.meta_module.raw:
- .dart2js.meta_module.clean
We could say this is a bug in the builder definition - we should maybe try to be more specific? But even if we change the middle builder to take an input of 'lib/.dart2js.meta_module.clean' it's still tricky to do the merging in code and understand what inputs aren't worth writing explicitly...
I started implementing a doctor command which can check this before we start enforcing it in https://github.com/dart-lang/build/pull/2149
Some thoughts that came up:
The checking is pretty complicated due to the whole "merging" of different Builder instances into one BuilderDefinition in the config. We don't really really on the full accuracy we're checking here. In fact, we don't use input extensions at all from build.yaml - we only use the combined set of output extensions.
We tried at one point removing the config altogether, and hit a problem because we do need the output extensions for builder ordering.
We could safely allow you to configure only output extensions as a List<String>. We can smoothly handle existing config by identifying that it's a map and doing buildExtensions = config is List ? config : config.values.expand((v) => v);. One downside is that changing the field is a breaking change for build_config, so while we're backwards compatible for users we do need to bump version ranges in pubpsecs to roll this out... It's also a much harder decision to roll back if we do decide we need to statically know the full merged buildExtensions statically. We would need that if we ever revisit generating build rules for bazel...
Closing this, we do have a doctor command but we also have some valid use cases for lying about build extensions (it allows them to be configured via builder options).
