Implement version code overrides for split ABIs#187742
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces a new Gradle property, force-version-code-ignoring-abi, allowing developers to bypass ABI-specific version code overrides during Flutter builds. Feedback on the changes highlights several critical issues in the integration tests, including a missing comma leading to implicit string concatenation, a misplaced closing brace that prematurely terminates the main function, and an incorrect aapt command for extracting the APK version code. Additionally, a relative import should be replaced with a package import to adhere to the style guide.
There was a problem hiding this comment.
Code Review
This pull request introduces a new Gradle property force-version-code-ignoring-abi to allow forcing the version code while ignoring the ABI split during Flutter builds, and adds an integration test to verify this behavior. Feedback on the integration test points out compatibility issues on Windows environments, such as incorrect path escaping, carriage return handling, missing directory checks, and the wrong executable name for aapt.
| final String fileContent = localPropertiesFile.readAsStringSync(); | ||
| final regex = RegExp(r'sdk\.dir=(.+)'); | ||
| final Match? match = regex.firstMatch(fileContent); | ||
| final String sdkPath = match?.group(1) ?? ''; | ||
|
|
||
| if (sdkPath.isEmpty) { | ||
| throw StateError('SDK path not found in local.properties'); | ||
| } | ||
|
|
||
| final FileSystemEntity? toolsDir = fileSystem | ||
| .directory(sdkPath) | ||
| .childDirectory('build-tools') | ||
| .listSync() | ||
| .firstWhereOrNull((FileSystemEntity entry) => | ||
| entry.statSync().type == FileSystemEntityType.directory); | ||
| if (toolsDir == null) { | ||
| throw StateError('No build-tools directory found'); | ||
| } | ||
| final String aapt = fileSystem | ||
| .directory(toolsDir) | ||
| .childFile(Platform.isWindows ? 'aapt.bat' : 'aapt') | ||
| .path; |
There was a problem hiding this comment.
There are several issues here that will cause this integration test to fail on Windows environments:
- Windows Path Escaping: In
local.properties, paths are stored with escaped backslashes and colons (e.g.,C\:\\Android\\Sdk). Reading this raw string directly will result in an invalid path when passed tofileSystem.directory. - Carriage Returns (
\r): On Windows, lines end with\r\n. The regex(.+)will capture the trailing\r, making the directory path invalid. - Missing Directory Check: Calling
listSync()onbuild-toolswithout checking if the directory exists first will throw aFileSystemExceptionif the SDK path is incorrect or incomplete. - Incorrect Executable Name: On Windows, the
aaptexecutable is namedaapt.exe, notaapt.bat.
Let's update this block to robustly handle Windows paths, trim carriage returns, check directory existence, and use the correct executable extension.
final String fileContent = localPropertiesFile.readAsStringSync();
final RegExp regex = RegExp(r'sdk\.dir=(.+)');
final Match? match = regex.firstMatch(fileContent);
if (match == null) {
throw StateError('sdk.dir not found in local.properties');
}
final String sdkPath = match.group(1)!
.trim()
.replaceAll(r'\:', ':')
.replaceAll(r'\\', r'\');
if (sdkPath.isEmpty) {
throw StateError('SDK path is empty in local.properties');
}
final Directory buildToolsDir = fileSystem.directory(sdkPath).childDirectory('build-tools');
if (!buildToolsDir.existsSync()) {
throw StateError('build-tools directory not found at ${buildToolsDir.path}');
}
final FileSystemEntity? toolsDir = buildToolsDir
.listSync()
.firstWhereOrNull((FileSystemEntity entry) =>
entry.statSync().type == FileSystemEntityType.directory);
if (toolsDir == null) {
throw StateError('No build-tools directory found');
}
final String aapt = fileSystem
.directory(toolsDir)
.childFile(Platform.isWindows ? 'aapt.exe' : 'aapt')
.path;|
I still get a test failure. Not sure whether that is a wrong local setup (using wrong flutter), or a bug. I guess running in CI is easiest to check that. |
closes #49544 by implementing a flag to force the flutter plugin to not run override the version code for split APIs.
More or less follows what #177753 did. The design should be sufficient to hook into custom build systems and doesn't do anything else.
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.
If this change needs to override an active code freeze, provide a comment explaining why. The code freeze workflow can be overridden by code reviewers. See pinned issues for any active code freezes with guidance.
Note: The Flutter team is currently trialing the use of Gemini Code Assist for GitHub. Comments from the
gemini-code-assistbot 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.