Don't compile shaders to SkSL unless --sksl arg is present (#182519)

The main change of this PR is to make `impellerc_main` no longer use
`TargetPlatformBundlesSkSL(switches.SelectDefaultTargetPlatform())` to
determine whether to compile to SkSL.

Addresses the "sksl unexpectedly compiling when building for ios"
mentioned in #182400.

`TargetPlatformBundlesSkSL()` returned `true` when running impellerc
targeting *any* runtime shaders. So even though
https://github.com/flutter/flutter/pull/163144/changes changed iOS
runtime shaders to no longer call impellerc with the `--sksl` runtime
target, it would still compile to SkSL because of the
`--runtime-stage-metal` runtime target.

After this PR, impellerc will only compile to SkSL if `--sksl` is
explicitly provided when calling impellerc.

The rest of the changes in this PR are related changes to improve code
organization and to update tests.

- `impellerc_main.cc`:
- `OutputIPLR()`: Remove the custom logic for compiling to SkSL. SkSL is
now treated the same as all the other compile targets in the `for (const
auto& platform : switches.PlatformsToCompile())` loop. This is the only
change with a behavioral difference in the PR. We no longer use
`TargetPlatformBundlesSkSL(switches.SelectDefaultTargetPlatform())` to
determine whether to compile to SkSL. Instead, it's treated the same as
any other compilation target and will only be targeted when it is
specified as an arg to the executable.
- Remove the `CompileSkSL()` helper function. This had the exact same
logic as what is used for all the other compilation targets in the `for
(const auto& platform : switches.PlatformsToCompile())` loop. So it is
not needed.
- `OutputDepfile()`: Remove the switch/case. The
`TargetPlatform::kUnknown` case is unreachable, so this used the other
case 100% of the time.
- `Main()`: `switches.CreateSourceOptions()` no longer has a default
platform target parameter. It doesn't matter which target it's called
with here. Arbitrarily call it with
`switches.PlatformsToCompile().front()`.

- `types.h/cc`:
- Remove the `TargetPlatformBundlesSkSL()` function. The only place this
was used was for `impellerc_main.cc`'s special case logic for SkSL,
which was removed as described above.

- `switches.h/cc`:
- Remove the optional/default parameter for `CreateSourceOptions()`.
This used to fall back to calling `SelectDefaultTargetPlatform()` to
determine the default value. I found this to be very unclear behavior.
The only place this was actually called with the default parameter is in
one function in `shader_bundle.cc` where the selected target platform
does not actually matter, so it doesn't even need this
somewhat-convoluted logic to select a platform with
`SelectDefaultTargetPlatform()`. I changed `CreateSourceOptions()` to
require an explicit parameter, which I think makes the function's
behavior clearer by removing the confusing default parameter.
- Remove the `SelectDefaultTargetPlatform()` function. I think it was a
little unclear/nonobvious what this was actually returning. It was only
used in 3 places, all of which were kind of confusing/unneeded:
1. In this same file, to pick the default target platform in
`switches.CreateSourceOptions()`. As described above, this seems
entirely unnecessary.
1. In `impeller_main.cc`, used in
`TargetPlatformBundlesSkSL(switches.SelectDefaultTargetPlatform())` to
determine whether to compile to SkSL. As described above, we don't want
this behavior. I think it was also kind of confusing.
1. In `impeller_main.cc`, used for the switch/case in `OutputDepfile()`.
As described above, this switch/case is entirely unnecessary.
- Change the `kKnownRuntimeStages` map to be a vector of pairs. This is
iterated through to populate the returned `runtime_stages_` list of
`Switches::PlatformsToCompile()`. Making it a vector makes the
`runtime_stages_` list maintain the ordering of `kKnownRuntimeStages`
(previously, iterating through the map would iterate in alphabetical
order of the keys).
- `impellerc_main.cc`'s `OutputIPLR()` now compiles to targets based on
`Switches::PlatformsToCompile()`, without special case logic to always
compile to "sksl" first. Changing this to a vector with "sksl" as the
first value preserves the original behavior of compiling to "sksl"
before any other targets. We do this because certain tests that perform
a failed shader compilation check specifically for an SkSL-based error
message (e.g. [this
one](https://github.com/flutter/flutter/blob/64866862f623ceeb45fd8be4782e8db8b58910c0/packages/flutter_tools/test/integration.shard/shader_compiler_test.dart#L150-L170)).
So for these tests, we need to try/fail with the SkSL compiler first,
before trying/failing with other compilers which would produce a
different error message.

- `shader_bundle.cc`
- As described above, change the usage of
`switches.CreateSourceOptions()` to require an explicit target platform
parameter. This particular usage doesn't matter, so use
`TargetPlatform::kUnknown` and add an explanatory comment.

- `compiler.cc`
- In `CreateCompiler()`, Add an `FML_UNREACHABLE` for the
`TargetPlatform::kUnknown` case, instead of falling back to using a
vulkan compiler. It doesn't make sense to call `CreateCompiler()` with
`TargetPlatform::kUnknown`. And currently, it can't happen: The only use
of `CreateCompiler()` is in the `Compiler` constructor on [line
432](https://github.com/flutter/flutter/blob/24ce716cfddfef201027c1a5fa2299a8aeffb03e/engine/src/flutter/impeller/compiler/compiler.cc#L432),
and there is a check preventing `TargetPlatform::kUnknown` earlier on
[line
292](https://github.com/flutter/flutter/blob/24ce716cfddfef201027c1a5fa2299a8aeffb03e/engine/src/flutter/impeller/compiler/compiler.cc#L292).

- `compiler_unittests.cc`, `compiler_test.h`
- The
`INSTANTIATE_{TARGET|RUNTIME_TARGET|SKSL_TARGET}_PLATFORM_TEST_SUITE_P`
defines were oddly located in the file in the middle of the tests. Move
them to the top of the file.
  - `INSTANTIATE_TARGET_PLATFORM_TEST_SUITE_P`
- Remove the `kSkSL` target. These tests seem to be specifically for
non-runtime targets, so SkSL doesn't belong here. All of these tests had
a filter to skip for SkSL, so none of them actually ran for SkSL. These
skips are now removed.
- Add the `kVulkan` target, so all non-runtime platform targets are
covered: opengles, openglesdesktop, metaldesktop, metalios, vulkan.
  - `INSTANTIATE_RUNTIME_TARGET_PLATFORM_TEST_SUITE_P`
- This used to only test with `kRuntimeStageMetal`. For better coverage,
I added all other runtime stages to this. It now tests on the metal,
gles, gles3, vulkan, and sksl runtime targets.
- Two tests, `UniformsAppearInJson` and `PositionedUniformsAppearInJson`
fail when running with vulkan and with sksl. I added skips for these. I
haven't dug deeper, but the failures seem unexpected to me. It's
possible that this is revealing a bug with the vulkan and sksl
compilers.
  - `INSTANTIATE_UNKNOWN_TARGET_PLATFORM_TEST_SUITE_P`
- Added this new define for running tests with
`TargetPlatform::kUnknown`.
    - Added a `MustFailDueToUnknownPlatform` test for this case.

- `fixtures/BUILD.gn`, `runtime_stage_unittests.cc`
- For the `impellerc("runtime_stages")` build target, add `--sksl` to
the impellerc flags. This preserves the existing behavior of these
targets being compiled for SkSL. They used to compile for SkSL because
other runtime targets are specified. But now impellerc only compiles to
SkSL when `--sksl` is explicitly specified.
- Create a new `impellerc("runtime_stages_non_sksl")` target that runs
impellerc without `--sksl`. Use this for a new
`ContainsExpectedShaderTypesNoSksl` test in the unit test file. That
test is the same as the existing `ContainsExpectedShaderTypes` test, but
using the non-sksl output from `impellerc("runtime_stages_non_sksl")`.

### Update for commit 2 of the PR:
The original PR had an issue that failed CI because a build rule
expected an impellerc output to include C++ reflection data, but the
reflection data was not output. I added a sizable commit to address
this:

- Fix compiler.gni logic around when to generate reflection state.
- This used to incorrectly generate reflection state whenever the last
shader_target_flags is not "--sksl".
- Instead, generate reflection state when any non-runtime target is in
shader_target_flags.
  - Consolidate some of the if/else logic to reduce duplicate code.
- Remove the TargetPlatformNeedsReflection check in impeller_main.cc.
- Instead, whether reflection state is generated depends only on the
presence or absense of "reflection_{json|header|cc}_name" flags.
- The logic of whether to include these flags is already in
compiler.gni. So it's redundant to also have logic for whether to
generate the reflection state here.
  - The TargetPlatformNeedsReflection method had faulty logic.
- It returned true for everything except SkSL, even though reflection
state isn't needed for runtime targets.
- It was called on the target from
Switches::SelectDefaultTargetPlatform. When impellerc is used with
multiple runtime targets, this would return the runtime target that is
first alphabetically by flag name. So if --sksl is provided along with
any other --runtime-stage-* target, SelectDefaultTargetPlatform returns
the non-sksl runtime target. Effectively this meant that
TargetPlatformNeedsReflection returns true except for when --sksl is the
only provided runtime target.
  - Removes the TargetPlatformNeedsReflection function entirely.

## Pre-launch Checklist

- [x] I read the [Contributor Guide] and followed the process outlined
there for submitting PRs.
- [x] I read the [Tree Hygiene] wiki page, which explains my
responsibilities.
- [x] I read and followed the [Flutter Style Guide], including [Features
we expect every widget to implement].
- [x] I signed the [CLA].
- [x] I listed at least one issue that this PR fixes in the description
above.
- [x] I updated/added relevant documentation (doc comments with `///`).
- [x] I added new tests to check the change I am making, or this PR is
[test-exempt].
- [x] I followed the [breaking change policy] and added [Data Driven
Fixes] where supported.
- [x] All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel
on [Discord].

**Note**: The Flutter team is currently trialing the use of [Gemini Code
Assist for
GitHub](https://developers.google.com/gemini-code-assist/docs/review-github-code).
Comments from the `gemini-code-assist` bot should not be taken as
authoritative feedback from the Flutter team. If you find its comments
useful you can update your code accordingly, but if you are unsure or
disagree with the feedback, please feel free to wait for a Flutter team
member's review for guidance on which automated comments should be
addressed.

<!-- Links -->
[Contributor Guide]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#overview
[Tree Hygiene]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md
[test-exempt]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#tests
[Flutter Style Guide]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md
[Features we expect every widget to implement]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md#features-we-expect-every-widget-to-implement
[CLA]: https://cla.developers.google.com/
[flutter/tests]: https://github.com/flutter/tests
[breaking change policy]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#handling-breaking-changes
[Discord]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Chat.md
[Data Driven Fixes]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Data-driven-Fixes.md
https://dart.googlesource.com/external/github.com/flutter/flutter/+/3109939de842eb6fe00a831e436963d4568bdc0e
2 files changed
tree: e0f84f3bcf056a6a084543ed07b062a583366be6
  1. engine/
  2. tools/
  3. .gitignore
  4. commits.json
  5. DEPS
  6. OWNERS
  7. README.md
README.md

Monorepo

A gclient solution for checking out Dart and Flutter source trees

Monorepo is:

  • Optimized for Tip-of-Tree testing: The Monorepo DEPS used to check out Dart and Flutter dependencies comes from the Flutter engine DEPS with updated dependencies from Dart.

Checking out Monorepo

With depot_tools installed and on your path, create a directory for your monorepo checkout and run these commands to create a gclient solution in that directory:

mkdir monorepo
cd monorepo
gclient config --unmanaged https://dart.googlesource.com/monorepo
gclient sync -D

This gives you a checkout in the monorepo directory that contains:

monorepo/
  DEPS - the DEPS used for this gclient checkout
  commits.json - the pinned commits for Dart, flutter/engine,
                 and flutter/flutter
  tools/ - scripts used to create monorepo DEPS
engine/src/ - the flutter/buildroot repo
    flutter/ - the flutter/engine repo
    out/ - the build directory, where Flutter engine builds are created
    third_party/ - Flutter dependencies checked out by DEPS
      dart/ - the Dart SDK checkout.
        third_party - Dart dependencies, also used by Flutter
flutter/ - the flutter/flutter repo

Building Flutter engine

Flutter's instructions for building the engine are at Compiling the engine

They can be followed closely, with a few changes:

  • Googlers working on Dart do not need to switch to Fuchsia's Goma RBE, except for Windows. The GOMA_DIR enviroment variable can just point to the .cipd_bin directory in a depot_tools installation, and just goma_ctl ensure_start is sufficient.
  • The --no-prebuilt-dart-sdk option has to be added to every gn command, so that the build is set up to build and use a local Dart SDK.
  • The --full-dart-sdk option must be added to gn for the host build target if you will be building web or desktop apps.

Example build commands that work on linux:

MONOREPO_PATH=$PWD
if [[ ! $PATH =~ (^|:)$MONOREPO_PATH/flutter/bin(:|$) ]]; then
  PATH=$MONOREPO_PATH/flutter/bin:$PATH
fi

export GOMA_DIR=$(dirname $(command -v gclient))/.cipd_bin
goma_ctl ensure_start

pushd engine/src
flutter/tools/gn --goma --no-prebuilt-dart-sdk --unoptimized --full-dart-sdk
autoninja -C out/host_debug_unopt
popd

Building Flutter apps

The Flutter commands used to build and run apps will use the locally built Flutter engine and Dart SDK, instead of the one downloaded by the Flutter tool, if the --local-engine option is provided.

For example, to build and run the Flutter spinning square sample on the web platform,

MONOREPO_PATH=$PWD
cd flutter/examples/layers
flutter --local-engine=host_debug_unopt \
  -d chrome run widgets/spinning_square.dart
cd $MONOREPO_PATH

To build for desktop, specify the desktop platform device in flutter run as -d macos or -d linux or -d windows. You may also need to run the command

flutter create --platforms=windows,macos,linux

on existing apps, such as sample apps. New apps created with flutter create already include these support files. Details of desktop support are at Desktop Support for Flutter

Testing

Tests in the Flutter source tree can be run with the flutter test command, run in the directory of a package containing tests. For example:

MONOREPO_PATH=$PWD
cd flutter/packages/flutter
flutter test --local-engine=host_debug_unopt
cd $MONOREPO_PATH

Troubleshooting

Please file an issue or email the dart-engprod team with any problems with or questions about using monorepo.

We will update this documentation to address them.

  • flutter commands may download the engine and Dart SDK files for the configured channel, even though they will be using the local engine and its SDK.

Windows

  • On Windows, gclient sync needs to be run in an administrator session, because some installed dependencies create symlinks.