Skip to content

et design issues are preventing more uniform behavior around commands #148444

@matanlurey

Description

@matanlurey

As seen on #148442, I accidentally removed special handling (specific to build_command.dart) that creates a missing ../out/{outDir} based on the selected build (i.e. -c host_debug_unopt_arm64 or similar), which I'll restore as part of #148442.

One thing I noticed is we special cased this for et build, that is, it doesn't work this way for et query, et test, and other commands that invoke gn (and never did, i.e. I didn't break all the commands, just one 😋 ). I was looking into solving this holistically, and ran into a number of design issues:

  • Each command that has a similar preamble that is not shared:

    @override
    Future<int> run() async {
      final String configName = argResults![configFlag] as String;
      final bool useRbe = argResults![rbeFlag] as bool;
      if (useRbe && !environment.hasRbeConfigInTree()) {
        environment.logger.error('RBE was requested but no RBE config was found');
        return 1;
      }
      final String demangledName = demangleConfigName(environment, configName);
      final Build? build =
          builds.where((Build build) => build.name == demangledName).firstOrNull;
      if (build == null) {
        environment.logger.error('Could not find config $configName');
        return 1;
      }

    That itself is a not a problem, but it means adding uniform behavior for commands requires extra caution (that we didn't do).

  • The inner-workings of et use divergent terminology and code-paths:

    For example, build_command, query_command, and test_command need to invoke gn desc in order to discover which
    targets exist based on a target pattern (i.e. //flutter/impeller/... resolves to [{...}, {...}, ...]), but then defer to another tool in another package (engine_build_configs) which again has knowledge of running tools like gn.

    It looks like goal of engine_build_configs (which, confusingly to me, is not limited to configs, that is it has extensive knowledge of running processes like ninja, gn, and even run_tests.py) is partially in conflict with that of et - if it's meant to be used only by et, I think it would be better to merge all of the runner code into et.

    The README.md of the package reads:

    This package contains libraries and example code for working with the engine_v2 build config json files

    I wonder if this just grew organically, or it needs an update - I'm having a hard time understanding. My preference is that unless there is a reason to have two tools that understand our build infrastructure, that we have all runner-specific code in one place.

Anyway, just filing this for 🍿 chatter from the contributors, I don't know what to make/to do at the moment.

Metadata

Metadata

Assignees

Labels

P2Important issues not at the top of the work listc: tech-debtTechnical debt, code quality, testing, etc.e: engine-toolEngine-specific tooling (i.e. `tools/engine_tool`).team-engineOwned by Engine teamtriaged-engineTriaged by Engine team

Type

No type

Projects

No projects

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions