-
Notifications
You must be signed in to change notification settings - Fork 29.7k
Description
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
etuse divergent terminology and code-paths:For example,
build_command,query_command, andtest_commandneed to invokegn descin 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 likegn.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 likeninja,gn, and evenrun_tests.py) is partially in conflict with that ofet- if it's meant to be used only byet, I think it would be better to merge all of the runner code intoet.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.