[CP-stable] Build hooks: Don't require toolchain for unit tests #179214
Conversation
|
This pull request was opened from and to a release candidate branch. This should only be done as part of the official Flutter release process. If you are attempting to make a regular contribution to the Flutter project, please close this PR and follow the instructions at Tree Hygiene for detailed instructions on contributing to Flutter. Reviewers: Use caution before merging pull requests to release branches. Ensure the proper procedure has been followed. |
github-actions
bot
added
tool
dcharkes
changed the title
![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 refactors the native toolchain discovery for Linux and macOS to be optional, which is a great improvement. This fixes an issue where flutter test would fail on systems without a native desktop toolchain (e.g., clang) when a project had dependencies with build hooks, even if the project only targeted Android. The changes correctly make cCompilerConfigLinux and cCompilerConfigMacOS return null instead of throwing an error if the toolchain isn't found, controlled by a new throwIfNotFound parameter. This parameter is appropriately set to false for test runs. I also appreciate the improvement to the macOS toolchain discovery to use xcrun --find, which is more robust. The changes are well-tested with new unit tests verifying the new behavior. I have a couple of minor suggestions to improve an error message and a test assertion.
| ); | ||
|
|
||
| if (found == null && throwIfNotFound) { | ||
| throwToolExit('Failed to find any of $possibleExecutableNames in $path'); |
There was a problem hiding this comment.
For better log readability, consider using path.path here to avoid printing the Directory: prefix in the error message.
| throwToolExit('Failed to find any of $possibleExecutableNames in $path'); | |
| throwToolExit('Failed to find any of $possibleExecutableNames in ${path.path}'); |
| } | ||
|
|
||
| await fileSystem.file('/a/path/to/clang++').create(recursive: true); | ||
| expect(cCompilerConfigLinux(throwIfNotFound: false), completes); |
There was a problem hiding this comment.
The completes matcher is correct, but not very specific. Since cCompilerConfigLinux is expected to return null in this scenario, using isNull would make the test's intent clearer and more precise. This also improves consistency with the corresponding test for macOS which uses isNull.
| expect(cCompilerConfigLinux(throwIfNotFound: false), completes); | |
| expect(await cCompilerConfigLinux(throwIfNotFound: false), isNull); |
|
cc @goderbauer |
eyebrowsoffire
added
the
autosubmit
3 participants
Issue Link:
#178954
(beta CP: #179211)
Impact Description:
Running
flutter teston Linux or MacOS which does not have aclangstarts failing on projects that target only Android otherwise (and do not require a Linux or MacOS toolchain) when there are build hooks anywhere in the dependencies.This PR changes the behavior for
flutter testto only passclangand friends if they are installed. This means that if a Linux/MacOS toolchain is installed on the system, it will be used for bothflutter run -d Linux/MacOSandflutter test(so that the same native compiler is used), but if it's not installed on the system bothflutter run -d AndroidDeviceandflutter testrun successfully.This means end users don't have to go modify their systems/CIs to install Linux/MacOS toolchains for Android projects.
Changelog Description:
[flutter/178954] Fixes running
flutter teston Linux/MacOS for Android projects with build hooks without the desktop native tooling installed.Workaround:
All end users have to install
clangandlddon Linux/MacOS hosts and CI bots.Risk:
What is the risk level of this cherry-pick?
It changes the flow in Flutter tools from throwing an error to returning a nullable value.
Test Coverage:
Are you confident that your fix is well-tested by automated tests?
Validation Steps:
On a Linux and MacOS system, make a Flutter app that only targets Android. Add a build hook. And ensure there's no
clangon thePATH(Can be verified byflutter doctor -vfinding no tooling for Flutter desktop). Runflutter testand it shouldn't fail due toclangnot being installed. For MacOS, XCode needs to not be installed.