build icon indicating copy to clipboard operation
build copied to clipboard

Support for the `include` keyword to include other YAML files

Open AlexV525 opened this issue 4 years ago • 9 comments

  • Dart SDK Version: 2.14.2
  • Packages using: json_serializable: ^5.0.0
  • Builders:
targets:
  $default:
    builders:
      json_serializable:
        options:
          field_rename: snake
          explicit_to_json: true

I'm using a base package to store our base configurations currently, e.g. analysis options. The structure may looks like this:

app_a
  - (dependency) base_package
app_b
  - (dependency) base_package
app_c
  - (dependency) base_package

Now I'm trying to use the include keyword to include the base build.yaml from our base package in projects, instead of writing the same builder everywhere. But I've got this error:

Invalid argument(s): line 1, column 1 of build.yaml: Unrecognized keys: [include]; supported keys: [builders, post_process_builders, targets, global_options, additional_public_assets]
  ╷
1 │ include: ../app_base/build.yaml
  │ ^^^^^^^
  ╵

So I'm assuming the include keyword is not supported. Could we have that supported?

AlexV525 avatar Sep 28 '21 02:09 AlexV525

We would need to define the merging strategies for these files (what if they both configure the same builder?). For every configuration option we would need to decide on and specify the merging strategy.

I don't really see this as a high priority for us, but if you (or somebody else) would like to drive the work here we would likely be open to external contributions. cc @natebosch what are your thoughts?

jakemac53 avatar Sep 28 '21 16:09 jakemac53

I'm having a hard time imagining a good strategy for doing anything sensible while merging targets. I think a general include approach is not feasible.

I could imagine supporting this case with include_global_builder_config (maybe a shorter name though) where the included config becomes another level in our merging strategy.

https://github.com/dart-lang/build/blob/master/docs/faq.md#how-is-the-configuration-for-a-builder-resolved

We could slot in that config like:

  • Builder defaults without a mode.
  • Builder defaults by mode.
  • Target configuration without a mode.
  • Target configuration by mode.
  • Global included options without a mode.
  • Global included options by mode.
  • Global local options without a mode.
  • Global local options by mode.
  • Options specified on the command line.

natebosch avatar Sep 28 '21 17:09 natebosch

Ya, I don't really see what we would do for merging targets either. If the src globs don't match up then its pretty difficult to imagine what we would do.

We could maybe support an option that doesn't allow you to specify anything other than an include line - basically just making the build file be a symlink to a different one.

jakemac53 avatar Sep 28 '21 17:09 jakemac53

We could also maybe disallow overlapping top level keys, so if the include defines targets, you could only define builders and global locally. I think my preference still leans towards it being explicitly a new level of config. It does mean it would be a different format for the included files - at the top level would be builder keys mapped to config.

natebosch avatar Sep 28 '21 17:09 natebosch

We could maybe support an option that doesn't allow you to specify anything other than an include line - basically just making the build file be a symlink to a different one.

Yeah I would say symlinks is a reasonable and proper workaround for now, which should be better than an YAML that only has a single line with include.

We could also maybe disallow overlapping top level keys, so if the include defines targets, you could only define builders and global locally.

I'm not familiar with these keys. IMO those top level (or maybe plus the second level) keys can be merged between multiple includes. After that, we should throw exceptions when detected conflict keys.

Here's a pseudo example for the best merging result that I can imagine:

base_build.yaml

targets:
  $default:
    builders:
      json_serializable:
        options:
          field_rename: snake

project_build.yaml

include ../base_package/base_build.yaml

targets:
  $default:
    builders:
      json_serializable:
        options:
          explicit_to_json: true

After merged

targets:
  $default:
    builders:
      json_serializable:
        options:
          field_rename: snake
          explicit_to_json: true

The final result could be one between the perfect one and other alternatives, but at least some of the base configurations can be included.

AlexV525 avatar Sep 29 '21 05:09 AlexV525

The problem with those examples is it only works if you only use the $default target and don't alter anything other than the builder configuration. Even then it only works if you don't also set the generate_for globs in the builder configuration I think.

It isn't clear how to merge any of those other things.

jakemac53 avatar Sep 29 '21 14:09 jakemac53

For sure, but I can't tell more since I'm not familiar about it. Maybe it can be worked as the analysis options with some magic. 👀

AlexV525 avatar Sep 30 '21 05:09 AlexV525

What if we use some JSON deep equality merger as base for this to happen?
It seems that parsing two build.yaml files into JSONs and using some standard merger would lead into expected, and probably desired, result

jodinathan avatar Apr 04 '22 12:04 jodinathan

It seems that parsing two build.yaml files into JSONs and using some standard merger would lead into expected, and probably desired, result

We could do some essentially "non-semantic" merge. It would be predictable, but would also likely only be useful in the simplest of cases. You would quickly run into things that you simply can't do, without some additional special syntax to directly affect the merge (such as dropping specific keys or entries from inherited json maps/collections).

jakemac53 avatar Apr 04 '22 14:04 jakemac53