Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Consider whether it's possible to remove build_extensions from build.yaml #3295

Open
wujek-srujek opened this issue May 3, 2022 · 3 comments
Labels
type-enhancement A request for a change that isn't a bug

Comments

@wujek-srujek
Copy link

wujek-srujek commented May 3, 2022

Here https://github.com/dart-lang/build/tree/master/build_config#defining-builders-to-apply-to-dependents one can read about build_extensions from build.yaml, it is required and cannot have null values, 'must match the merged buildExtensions maps from each Builder' etc. However, my tests show that this is not really validated in any way nor is it enforced. For example, I have this in build.yaml:

    build_extensions:
      .dart:
        - .foo

but in the builder I have:

  @override
  final buildExtensions = const {
    '.dart': ['.bar'],
  };

and my builder works just fine.

The only thing that seems to mind is the doctor command, which itself is undocumented and doesn't appear here https://github.com/dart-lang/build/tree/master/build_runner#built-in-commands nor in the output of flutter pub run build_runner --help, so it is actually pretty hard to find a thing that fails for different extension settings.

It also seems like the build_extensions configuration is redundant because the builder has the same information. I have also seen past tickets that deprecated this (and target), but somehow this configuration made it back.

Why is this information needed in build.yaml? Can it be dropped? I have to admit it is pretty confusing for newcomers.

@natebosch
Copy link
Member

The build extensions in build.yaml only impact builder ordering today. We use the outputs listed in build.yaml, along with requiredInputs config to order builders which read certain files after the builders that write them.

We originally had more plans for the config, then tried to remove it and realized we still needed it for inputs, and decided to keep the existing format rather than change to a format where you only express output extensions.

@wujek-srujek
Copy link
Author

Ok so you use it is a 'static' way where you can't or don't want to invoke the Builder method, right?
But this can be tricky sometimes, e.g. when Builder.buildExtensions returns something that depends on configuration, as the docs allow it ('Builders may also choose extensions based on BuilderOptions).' - they could be something that the builder doesn't know statically as it's defined by the user when configured in a target. Would the ordering logic break in such cases?

@jakemac53
Copy link
Contributor

Would the ordering logic break in such cases?

Yes, it will break the ordering logic.

Resolving this and removing build_extensions from build.yaml would I think be possible, but likely would be somewhat challenging and require a fair bit of restructuring of the code.

For instance we would probably have to compute builder ordering per target, I can't actually remember right now if we do it globally or per package, but I am pretty sure it isn't per target.

@natebosch natebosch changed the title What purpose does build_extensions in builder definition in build.yaml really serve? Consider whether it's possible to remove build_extensions from build.yaml May 3, 2022
@natebosch natebosch added the type-enhancement A request for a change that isn't a bug label May 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type-enhancement A request for a change that isn't a bug
Projects
None yet
Development

No branches or pull requests

3 participants