Repository navigation
Conversation
github-actions
Bot
added
the
tool
|
@dcharkes I'm back at it 😛 |
b5f49c2 to
ea4efbc
Compare
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the native assets build system to provide host-tool paths, specifically IMPELLERC and LIBTESSELLATOR, to build hook subprocesses. It adds a buildHookEnvironment utility to filter environment variables and resolve artifact paths, with accompanying unit tests. Feedback includes a suggestion to remove a potentially redundant toUpperCase() call and a recommendation to use absolute paths for host artifacts to ensure reliability across different working directories.
Build hooks (the cross-package replacement for native_assets_cli) run as separate Dart subprocesses spawned by hooks_runner. They don't have access to flutter_tools' in-process `Artifacts` API, so packages that need host-side tools (the canonical case being `flutter_gpu_shaders`, which compiles `.shaderbundle.json` via `impellerc`) re-implement their own artifact resolution by walking up from the dart binary into `bin/cache/artifacts/engine/<host>/`. That homegrown lookup misses the `--local-engine` override that `CachedLocalEngineArtifacts.getHostArtifact` already wires up for flutter_tools' own targets. The result is silent version skew between the local engine and host tools, which surfaces downstream as opaque failures (e.g. shader bundle format mismatches). This change extends the same pattern desktop builds already use for LOCAL_ENGINE / LOCAL_ENGINE_HOST: when flutter_tools constructs its NativeAssetsBuildRunner, also build a `hookEnvironment` that augments the default filtered env with `IMPELLERC` and `LIBTESSELLATOR`, resolved through `Artifacts.getHostArtifact(...)`. Hooks that read these env vars get the correct host tools whether or not `--local-engine` is set, and don't have to re-implement artifact resolution. Fixes flutter#186299.
ea4efbc to
bad5505
Compare
flutter_tools sets IMPELLERC on build hook subprocesses (see flutter/flutter#186300) so that the --local-engine override that CachedLocalEngineArtifacts already wires up for flutter_tools' own build targets also reaches third-party hooks. Honor the runtime env var as a second resolution step (after the existing compile-time String.fromEnvironment define, before walking up to the SDK cache), so this package picks up the correct impellerc with no per-user configuration when developing against a locally built engine.
|
Just retriggered CI with an empty commit. The previous Running |
CI's Linux analyze shard fails on dev/tools/bin/format.dart because testUsingContext and one expect call were left on multiple lines where the formatter collapses them onto one. No behavior change.
|
Hi @bdero! Welcome back! I think allowlisting env vars for specific packages is an anti-pattern. So far we've only been allowlisting variables for OS-specific things or build-system specific things. If you want your end users to configure whether a feature is available or where a native library can be found, this should be done via user-defines. // pubspec.yaml
hooks:
user_defines:
<package-name>:
<key>: <value>These are accessible in https://pub.dev/documentation/hooks/latest/hooks/HookInputUserDefines-class.html If at all possible, I'd encourage to structure your package in a way that doesn't require end-users to configure everything. If your package ends up being used by 100000 devs, all of them would need to configure their system. Is it possible to have good defaults? And to write your hook in a way that would download any necessary files from a CDN (GitHuhb releases/artifacts). Edit: If you want to expose Flutter-internal artifacts to build hooks, then we should add some kind of flutter-specific extension to the input.config with a Dart API as an extension method that is available in Flutter. Something along the lines of: |
|
Yup, just trying to expose a couple of binaries that ship in the Flutter engine artifacts to build hooks. This replaces the fragile code in Are you suggesting I do something similar to the Happy to try and go for this approach if it sounds right to you. |
Yes! |
|
Alright, I pushed the package here: flutter/packages#11701 If we're able to land it, I'll repurpose this PR for the integration. |
This PR was closed in favor of flutter/core-packages#19. @bdero, I'm guessing you're still working on figuring out the name for the package before picking this back up? |
|
http://github.com/flutter/flutter/pull/189821 replaces the need for this, closing now. |
4 participants
Fixes #186299. When flutter_tools spawns build hooks (the cross-package replacement for
native_assets_cli), it now publishesIMPELLERCandLIBTESSELLATORenvironment variables resolved throughArtifacts.getHostArtifact(...), so build hooks that need host-side tools see the same versions of those tools that flutter_tools' own build targets do.Build hooks run as separate Dart subprocesses spawned by
hooks_runner. They don't have access to flutter_tools' in-processArtifactsAPI, so packages that need host tools (the canonical case beingflutter_gpu_shaders, which compiles.shaderbundle.jsonviaimpellerc) re-implement their own artifact resolution by walking up from the dart binary intobin/cache/artifacts/engine/<host>/. That homegrown lookup misses the--local-engineoverride thatCachedLocalEngineArtifacts.getHostArtifactalready wires up for flutter_tools' own targets (the FragmentProgram shader compiler atbuild_system/tools/shader_compiler.dart:167works correctly under--local-enginefor this exact reason). The downstream symptom is silent version skew: a freshly built local engine paired with a hook that picked up the cached SDK'simpellercproduces shader bundles in a format the local engine doesn't understand. The user sees an opaqueUnsupported shader bundle format version: 1, expected: 2at runtime with no pointer back to the artifact mismatch.flutter_tools/lib/src/isolated/native_assets/native_assets.dartadds abuildHookEnvironment(...)helper that takesArtifactsandPlatform, replicates theNativeAssetsBuildRunnerdefault filter, and addsIMPELLERCandLIBTESSELLATORfrom theHostArtifactresolver.ArtifactsandPlatformare threaded throughFlutterNativeAssetsBuildRunnerImpl's constructor and used to populateNativeAssetsBuildRunner.hookEnvironment.flutter_tools/lib/src/build_system/targets/native_assets.dartupdatescreateFlutterNativeAssetsBuildRunner(environment)to passenvironment.artifactsandenvironment.platformthrough to the impl. The pattern matches what desktop builds already do forLOCAL_ENGINEandLOCAL_ENGINE_HOSTenv vars (seelib/src/linux/build_linux.dart:75-76,lib/src/windows/build_windows.dart:292-293).Hooks that don't read these env vars are unaffected. Hooks that opt in (e.g. by checking
Platform.environment['IMPELLERC']) get correct behavior under both--local-engineand the normal cached engine without any per-package logic. The cache hash for build hook inputs already includes the hook environment, so consumers will see a one-time cache miss on the first run after this lands and stable hits thereafter.packages/flutter_tools/test/general.shard/isolated/build_system/targets/native_assets_test.dartadds three direct tests forbuildHookEnvironment: verifyingIMPELLERCandLIBTESSELLATORare present in the produced env; verifying the values matchArtifacts.getHostArtifact(...)(the contract that propagates the--local-engineoverride to hooks); and verifying the parent-process env is filtered through the same allowlistNativeAssetsBuildRunneruses by default. All 14 tests in the file pass, including the original 11.Pre-launch Checklist
///).