Build: [Meta] Configuring Builders

[matanlurey]

This is a meta post for discussion around how to configure builders to be run.

Prior Art

Barback

Today, pub and barback transformers use a combination of convention + configuration:

• Configuration: A transformers section in pubspec.yaml:

transformers:
    - some_pkg
    - another_pkg/transform/something

• Convention: If a file is not specified, some_pkg means some_pkg|lib/transformers.dart.

Ordering is, I believe, initiated by the application package (i.e. the one with pub {serve|build} being run on it), by looking at the transformers section. I'm not 100% sure (Jake? Nate?) how a dependencies' transformers section weighs into calculating order, i.e.:

# app_pkg
dependencies:
    - dep_pkg

transformers:
    - some_pkg_a

# dep_pkg
transformers:
    - some_pkg_b

... Does some_pkg_a or some_pkg_b build first? Or is it undefined?

Questions

Builder Location

Or, where a package's class {...} implements Builder should live. Ideas:

1. In lib/build[er].dart (Would be most similar to barback today)
2. In lib/src/build[er].dart (Would reinforce that the builder implementation is private)
3. In tool/build[er].dart (Would require custom code to be able to import properly)
4. In {%pkg_root}/build[er].dart Similar to above.

It's hard to choose without answering some basic questions:

• Should a builder be considered private or public?

That is, is using a build.yaml (or similar) and automating the code generation process considered the "only" golden path, or do we want users to be able to "roll their own" similar to today (and similar to systems like webpack)? If we do, then lib/build[er].dart might be the only sensible option. But see Builder Dependencies for issues with this approach.

• Should the user have to write some "factory" functions like the _bazel_codegen package?

i.e.

Builder codegen() => new MyCodegenBuilder(...);

A. If so, where would this file live?
B. Could it be generated for builders with no parameters?
C. How could your builder take arguments?

Taking Arguments

For Bazel, there are two ways to pass arguments to code generation: globally and per-package.

Global flags make sense when _all_ builder invocations of a specific implementation _must_ use the same parameter. For example, you might have a debug and release mode of a builder, where debug mode _requires_ that other packages using the builder are _also_ built in debug mode, otherwise the application will not work - or it might be a flag that you only use for debugging/developing and it would be impossible to set it all packages.

In Bazel, these are --define flags:

$ bazel build <target> --define=<flag>=<value>

Per-package flags allow you to change or configuration code generation locally. These changes are usually cross-compatible, i.e. you are only changing your implementation details, and only for packages you control. One example of this is using "legacy style encapsulation" in AngularDart, and it is configured using generator_args in Bazel today:

dart_library(
  genreator_args = {
      "angular": {
        "some_flag": "some_value",
      }
  }, 
)

Builder Dependencies

Some builders will be able to get away with just a dependency on pkg/build, others might need to use other libraries that are considered VM only, explicitly or otherwise. Even pkg/build transitively ties you to pkg/analyzer, which is pretty heavyweight and difficult to revision against.

Packages today like @davidmorgan's build_value ship two packages, including a {%pkg_name}_generator package. It's non-obvious (to me at least) if this is the expected workflow for packages, or whether we should have guidelines around this. For the meantime AngularDart needs to ship the builder as part of the main package.

[jakemac53]

Re: barback ordering, the transformers section of each package determines the order things are ran on each package, but the ordering between packages is pretty confusing. Essentially, all transformers will block when trying to read a file in another package until everything in that package is done building, because there is no guarantee that file won't change even if it already exists. This lack of real ordering leads to deadlocks when you have package dependency cycles.

See https://github.com/dart-lang/barback/pull/84 as a concrete example of this deadlock situation.

[matanlurey]

So I guess "undefined" was a good guess!

[natebosch]

I'd be in favor of pushing for a convention like lib/build/builder.dart - It's not in src/ so if we ever enforce against those imports we won't be broken, and the build/ hopefully signals that it shouldn't be imported in normal code.

I think it makes the most sense for packages to add a factory like with _bazel_codegen. I don't think we should get into the business of autogenerating them even in the simple case.

I think we should keep List<String> args as the interface for that factory. There is some inefficiency there where if multiple Builders need to parse args to the same type it'll happen once per builder instead of once overall, but that's probably still cheap enough that it isn't worth the extra implementation complexity of doing something fancier.

I think any package which vends generation must have a build.yaml and we should avoid any automagic on the Builder author side, and limit magic on the consuming side.

[matanlurey]

@natebosch:

I'd be in favor of pushing for a convention like lib/build/builder.dart - It's not in src/ so if we ever enforce against those imports we won't be broken, and the build/ hopefully signals that it shouldn't be imported in normal code.

How would this works with the built_value|built_value_generator convention?

I think it makes the most sense for packages to add a factory like with _bazel_codegen. I don't think we should get into the business of autogenerating them even in the simple case.

So build/builder.dart is only responsibile for having top-level factories that return Builder?

I think we should keep List<String> args as the interface for that factory.

Ah, you mean like:

Builder codegen(List<String> args) => new CodegenBuilder(...);

?

There is some inefficiency there where if multiple Builders need to parse args to the same type it'll happen once per builder instead of once overall, but that's probably still cheap enough that it isn't worth the extra implementation complexity of doing something fancier.

Yeah. I'd like to _consider_ auto-generating, but I'm totally fine for skipping that until well-into 1.0.

I think any package which vends generation must have a build.yaml and we should avoid any automagic on the Builder author side, and limit magic on the consuming side.

Sgtm! I'll keep this issue open for more feedback.

[natebosch]

How would this works with the built_value|built_value_generator convention?

For the places where we want to split it (less necessary once pub 'features' are working well) I'd put the builder at package:built_value_generator/build/builder.dart

So build/builder.dart is only responsibile for having top-level factories that return Builder?

Yeah depending on whether the complexity is in the Builder class or hidden elsewhere, or for very simple builders, could also put the class in this library. At a minimum it would need the top-level factories but I wouldn't say we need a convention around whether that's the only thing that should be there.

Ah, you mean like:

Builder codegen(List<String> args) => new CodegenBuilder(...);

?

Yeah this is exactly the pattern I like - easy to read for the simple case but allows for much more complexity whenever useful.

[davidmorgan]

Maybe follow what the analyzer does for plugins? Seems like exactly the
same problem :)

https://github.com/dart-lang/angular/tree/master/angular/tools/analyzer_plugin

It's in a separate tree, "tools", with its own pubspec.yaml. So it gets to
have distinct dependencies.

This has the advantage that "built_value" can declare that it needs
"built_value_generator".

On Tue, 31 Oct 2017 at 00:15 Nate Bosch [e-mail] wrote:

How would this works with the built_value|built_value_generator
convention?

For the places where we want to split it (less necessary once pub
'features' are working well) I'd put the builder at
package:built_value_generator/build/builder.dart

So build/builder.dart is only responsibile for having top-level factories
that return Builder?

Yeah depending on whether the complexity is in the Builder class or
hidden elsewhere, or for very simple builders, could also put the class in
this library. At a minimum it would need the top-level factories but I
wouldn't say we need a convention around whether that's the only thing that
should be there.

Ah, you mean like:

Builder codegen(List args) => new CodegenBuilder(...);

?

Yeah this is exactly the pattern I like - easy to read for the simple case
but allows for much more complexity whenever useful.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/dart-lang/build/issues/554#issuecomment-340612979,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AFR8mKYv2aFuak470eiWf1bZSfyzZDY5ks5sxliRgaJpZM4QLcDq
.

[natebosch]

@davidmorgan
I don't understand what this does. Does the analyzer have it's own pubspec loader and package resolver to read that file and find the plugin?

[davidmorgan]

No idea how it works, sorry -- just discovered it myself last week. But I guess it must do something like that.

@bwilkerson can presumably shed some light on the question :)

[matanlurey]

Any other ideas here?

[natebosch]

I'm not sure what the remaining open questions specific to this issue are.

For our current implementation the builder author has a lot of freedom. The requirements are:

1. You must implement the typedef Builder Function(List<String> args) in some file in lib/.
2. You must give us config that tells us:
   
   
   
   • What that functions name is
   
   
   • What the package: import is to reach it
   
   
   • If you have multiple "target"s in your package, which target exposes that import. (note that targets aren't actually honored today)
   
   

By convention we'd like to push for the builder factory to exist in either lib/builders.dart or lib/builders/some_specific.dart. Beyond that we don't have any specific guidance.

In our current forming implementation, simply defining that you expose a Builder will get it automatically applied to packages which depend on you.

We still need:

• To honor the manual config around targets.
• To honor the manual config around applying builders to targets.
• To decide how manual and auto config interact with eachother: https://github.com/dart-lang/build/issues/596
• To decide how users will configure arguments for builders.
• To decide how we'll order Builders across packages (for instance that Sass should run before Angular if you're using both).

[natebosch]

We're now allowing a yaml Map in build.yaml which gets turned into a BuilderOptions and passed to the builder factory. For now we won't allow extra arguments on the command line but we can revisit this.

I think all the other questions have been answered. See https://github.com/dart-lang/build/blob/master/build_runner/lib/src/build_script_generate/builder_ordering.dart for the implementation of how builders are ordered.