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
Monorepo is:
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
Flutter's instructions for building the engine are at Compiling the engine
They can be followed closely, with a few changes:
goma_ctl ensure_start is sufficient.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
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
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
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.gclient sync needs to be run in an administrator session, because some installed dependencies create symlinks.