building assets fix - #11
Conversation
Summary of ChangesHello @zs-dima, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the native asset building process, particularly for Windows, by integrating proper Visual Studio environment management and ensuring correct static library linking. It also addresses a critical dependency for Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![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 introduces significant improvements for building on Windows by properly setting up the Visual Studio environment and handling static library linking. It also refactors the build process for libpng to include zlib as a dependency. However, there are several critical issues that need to be addressed. The tests have been completely disabled, which is a major concern. There's a critical C-Runtime mismatch in the WebP Windows build that will likely cause linker errors or runtime instability. There are also several other bugs and regressions in the build scripts that could lead to build failures on different configurations.
| '/O2', | ||
| '/DRELEASE', | ||
| '/DNDEBUG', | ||
| '/MD', // Use dynamic CRT (matches how libwebp was built) |
There was a problem hiding this comment.
There's a mismatch in the C Runtime (CRT) linkage. You're using /MD (dynamic CRT) to compile the glue DLL, but the static libraries (libwebp, libsharpyuv) are built with /MT (static CRT) in buildWindowsStatic. Additionally, you are linking against libcmt.lib, which is the static CRT library. This will lead to linker errors or runtime crashes. The glue DLL must be compiled with the same CRT setting as the static libraries it links to. Please change /MD to /MT.
| '/MD', // Use dynamic CRT (matches how libwebp was built) | |
| '/MT', // Use static CRT to match how libwebp was built |
| // import 'package:test/test.dart'; | ||
|
|
||
| void main() { | ||
| group('A group of tests', () {}, skip: true); | ||
| } | ||
| // void main() { | ||
| // group('A group of tests', () {}, skip: true); | ||
| // } |
There was a problem hiding this comment.
The tests have been completely commented out. Introducing significant changes to the build logic, especially for a new platform, without running tests is very risky. Please re-enable the tests and ensure they pass on all supported platforms before merging.
| } | ||
|
|
||
| final env = <String, String>{}; | ||
| for (final line in (result.stdout as String).split('\n')) { |
There was a problem hiding this comment.
Using split('\n') is not robust for handling Windows-style line endings (\r\n). This can lead to lines with a trailing \r which might affect parsing of the environment variables. It's safer to use LineSplitter from dart:convert or a regular expression that accounts for the optional \r.
| for (final line in (result.stdout as String).split('\n')) { | |
| for (final line in (result.stdout as String).split(RegExp(r'\r?\n'))) { |
| logger.info('VS environment loaded successfully'); | ||
| logger.info('LIB: ${env['LIB']?.substring(0, 80)}...'); | ||
|
|
There was a problem hiding this comment.
This line can cause a RangeError if the LIB environment variable has a length less than 80. The substring method requires the end index to be less than or equal to the string length. You should ensure you don't try to access an index out of bounds when logging this information.
logger.info('VS environment loaded successfully');
final lib = env['LIB'];
if (lib != null) {
final snippet = lib.length > 80 ? lib.substring(0, 80) : lib;
logger.info('LIB: $snippet...');
}
| final platformTag = switch (os) { | ||
| OS.windows => 'windows-${arch == Architecture.x64 ? 'x64' : arch.toString()}', | ||
| OS.macOS => 'macos-${arch == Architecture.arm64 ? 'arm64' : 'x86_64'}', | ||
| OS.linux => 'linux-${arch == Architecture.x64 ? 'x86_64' : arch.toString()}', | ||
| OS.iOS => 'ios-${arch == Architecture.arm64 ? 'arm64' : 'x86_64'}', | ||
| OS.android => 'android-${abiForAndroid(arch)}', | ||
| _ => 'unknown', | ||
| }; |
There was a problem hiding this comment.
The logic for constructing platformTag is inconsistent with the logic inside the build...Static methods in lib_builder.dart. For example, for Windows ARM64, arch.toString() will produce 'Architecture.arm64', but buildWindowsStatic internally uses 'ARM64'. This will cause a mismatch in the expected directory names and the build will fail to find the installed libraries. You should unify this logic, perhaps by having the build...Static methods return the created installation path to avoid this kind of discrepancy.
| final actualInstallDir = Directory(p.join(zlibWs.path, 'install')) | ||
| .listSync() | ||
| .whereType<Directory>() | ||
| .firstWhere((d) => p.basename(d.path).contains(_zlibVersion)); |
There was a problem hiding this comment.
The logic to find the installation directory is fragile. It relies on searching for a directory whose name contains the version string. This can fail if there are other directories matching this condition. A more robust approach is to construct the exact path, since the build...Static methods create directories with predictable names. The path should be p.join(zlibWs.path, 'install', '$platformTag-$_zlibVersion'). Note that this depends on fixing the platformTag inconsistency mentioned in another comment.
| Future<String> _stageSources(Directory ws) async { | ||
| final srcRoot = Directory(p.join(ws.path, 'src'))..createSync(recursive: true); | ||
| final dst = Directory(p.join(srcRoot.path, 'libwebp-${defines.version}')); | ||
| if (dst.existsSync()) return dst.path; | ||
|
|
||
| if (defines.vendoredPath != null) { | ||
| final from = Directory(defines.vendoredPath!); | ||
| if (!from.existsSync()) { | ||
| throw StateError('Vendored path not found: ${from.path}'); | ||
| } | ||
| logger.info('Using vendored sources at ${from.path}'); | ||
| await copyTree(from, dst); | ||
| return dst.path; | ||
| final version = defines.version; | ||
| final cacheDir = Directory(p.join(ws.path, 'cache'))..createSync(recursive: true); | ||
| final tarball = File(p.join(cacheDir.path, 'libwebp-$version.tar.gz')); | ||
|
|
||
| if (!tarball.existsSync()) { | ||
| final url = defines.tarballUri ?? defines.downloadUrl; | ||
| await downloadTo(tarball, Uri.parse(url)); | ||
| } | ||
|
|
||
| final url = Uri.parse(defines.downloadUrl); | ||
| final cache = Directory(p.join(ws.path, 'cache'))..createSync(recursive: true); | ||
| final tar = File(p.join(cache.path, 'libwebp-${defines.version}.tar.gz')); | ||
| await downloadTo(tar, url); | ||
| await extractTarGz(tar, srcRoot); | ||
|
|
||
| final extractedTop = Directory( | ||
| srcRoot.path, | ||
| ).listSync().whereType<Directory>().firstWhere((d) => p.basename(d.path).startsWith('libwebp-')); | ||
| if (extractedTop.path != dst.path) { | ||
| await extractedTop.rename(dst.path); | ||
| final srcDir = Directory(p.join(ws.path, 'src')); | ||
| final webpSrcDir = Directory(p.join(srcDir.path, 'libwebp-$version')); | ||
|
|
||
| if (!webpSrcDir.existsSync()) { | ||
| await extractTarGz(tarball, srcDir); | ||
| } | ||
| logger.info('Sources staged → ${dst.path}'); | ||
| return dst.path; | ||
|
|
||
| logger.info('Sources staged → ${webpSrcDir.path}'); | ||
| return webpSrcDir.path; | ||
| } |
There was a problem hiding this comment.
This refactoring of _stageSources has removed the logic to handle vendored sources (defines.vendoredPath). This is a feature regression, as the WebpDefines class still seems to support it. Please restore the logic for using vendored sources if they are specified.
| try { | ||
| final cc = (input.json['config'] as Map?)?['extensions']?['code_assets']?['c_compiler']; | ||
| if (cc is Map) { | ||
| final windows = cc['windows'] as Map?; | ||
| if (windows != null) { | ||
| final devPrompt = windows['developer_command_prompt'] as Map?; | ||
| if (devPrompt != null) { | ||
| final script = devPrompt['script'] as String?; | ||
| if (script != null && File(script).existsSync()) { | ||
| return script; | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } catch (_) {} |
There was a problem hiding this comment.
The try { ... } catch (_) {} block for parsing input.json is very broad. It silently swallows any error, which could make debugging difficult if the configuration is malformed. Consider catching more specific exceptions (like TypeError or StateError) or at least logging the error, even if only in a debug build, to aid in troubleshooting configuration issues.
|
|
||
| // Find MSVC bin directory and move it to the front | ||
| final msvcBins = pathParts.where((p) => p.contains('MSVC') && p.contains('bin')).toList(); | ||
| final otherPaths = pathParts.where((p) => !p.toLowerCase().contains('llvm')).toList(); |
There was a problem hiding this comment.
Filtering the PATH by removing any entry containing "llvm" is a bit aggressive and could have unintended side effects if a legitimate, non-clang path happens to contain that substring. A more targeted approach would be to identify the specific LLVM/Clang path added by tools like Flutter and remove only that. While this is a pragmatic solution for a common problem, it's worth noting its potential brittleness.
| final srcDir = Directory(p.join(ws.path, 'src')); | ||
| final webpSrcDir = Directory(p.join(srcDir.path, 'libwebp-$version')); | ||
|
|
||
| if (!webpSrcDir.existsSync()) { | ||
| await extractTarGz(tarball, srcDir); | ||
| } |
There was a problem hiding this comment.
The new implementation of _stageSources assumes the name of the extracted directory inside the tarball is libwebp-$version. The previous implementation was more robust as it searched for a directory starting with libwebp- and renamed it. This made it resilient to minor variations in the tarball structure. Consider restoring the more robust directory discovery logic.
There was a problem hiding this comment.
Pull request overview
This PR addresses Windows build failures for the imagekit_ffi package by fixing path handling issues and adding Windows-specific build configurations for native libraries (WebP, JPEG, PNG). The main issue was that URI-to-path conversion on Windows was causing "The filename, directory name, or volume label syntax is incorrect" errors.
Changes:
- Updated dependencies (SDK to 3.10.1, hooks and code_assets to 1.0.0) and disabled tests temporarily
- Added Windows DLL export macros to native C headers and fixed a subsampling enum bug
- Implemented Windows-specific build logic with Visual Studio environment handling and CRT linkage configuration
- Added zlib dependency building for PNG support
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 23 comments.
Show a summary per file
| File | Description |
|---|---|
| test/imagekit_ffi_test.dart | All tests commented out (testing disabled) |
| pubspec.yaml | Updated SDK to 3.10.1, upgraded dependencies to 1.0.0, disabled test framework |
| native/webp/ik_webp.h | Added Windows DLL export macros (IK_EXPORT) for API functions |
| native/webp/ik_webp.c | Applied export macros, fixed subsampling enum value bug (S444=0 not 2) |
| native/png/ik_png.h | Removed duplicate ik_png_free declaration |
| helpers/defines/png_defines.dart | Added blank line for formatting |
| helpers/builders/webp_build.dart | Major refactor: fixed URI-to-path conversion, added Windows DLL build with cl.exe, separated build logic per platform |
| helpers/builders/turbo_jpeg_build.dart | Fixed URI-to-path conversion, added Windows-specific SIMD disable and library naming |
| helpers/builders/png_build.dart | Fixed URI-to-path conversion, integrated zlib building, added Windows CRT configuration |
| helpers/builders/lib_builder.dart | Fixed URI-to-path conversion, added VS environment detection/setup, improved CMake Windows build with toolchain file and CRT flags |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Add required Windows/CRT libraries | ||
| 'libcmt.lib', // C runtime (static) | ||
| 'libvcruntime.lib', // VC runtime | ||
| 'libucrt.lib', // Universal CRT | ||
| '/NODEFAULTLIB:msvcrt.lib', // Avoid CRT conflicts |
There was a problem hiding this comment.
Mixing static CRT libraries (libcmt.lib, libvcruntime.lib, libucrt.lib) with dynamic CRT linkage (/MD flag) will cause linker conflicts. The /MD flag specifies dynamic CRT linking, but then libcmt.lib (static C runtime) is explicitly linked. Additionally, /NODEFAULTLIB:msvcrt.lib prevents the dynamic runtime that /MD expects. Either use /MT with static libraries, or use /MD without the static CRT libraries.
| // Add required Windows/CRT libraries | |
| 'libcmt.lib', // C runtime (static) | |
| 'libvcruntime.lib', // VC runtime | |
| 'libucrt.lib', // Universal CRT | |
| '/NODEFAULTLIB:msvcrt.lib', // Avoid CRT conflicts |
| // import 'package:test/test.dart'; | ||
|
|
||
| void main() { | ||
| group('A group of tests', () {}, skip: true); | ||
| } | ||
| // void main() { | ||
| // group('A group of tests', () {}, skip: true); | ||
| // } |
There was a problem hiding this comment.
All test code has been commented out, which removes test coverage for the package. This is problematic even though the PR description states it was only tested for WebpKit().encode on Windows. The tests should be updated to work with the new changes rather than being completely disabled.
| hooks: ^0.20.1 | ||
| code_assets: ^0.19.7 | ||
| hooks: ^1.0.0 | ||
| code_assets: ^1.0.0 |
There was a problem hiding this comment.
The version update from code_assets ^0.19.7 to ^1.0.0 represents a major version bump which may include breaking changes. Ensure all usages of the code_assets package have been verified to work with version 1.0.0, as major version changes typically indicate API-breaking changes.
| code_assets: ^1.0.0 | |
| code_assets: ^0.19.7 |
| } else { | ||
| libPath = p.join(libDir, 'libz.a'); | ||
| } | ||
|
|
There was a problem hiding this comment.
The zlib library path determination falls back from 'zlibstatic.lib' to 'zlib.lib', but doesn't verify that the fallback file exists. If neither file exists, the function will return a non-existent path without any error, which will cause cryptic linker errors later. Add a check to throw an error if neither library file is found.
| if (!File(libPath).existsSync()) { | |
| throw StateError( | |
| '$_logTag: zlib library not found in $libDir. ' | |
| 'Tried ${os == OS.windows ? 'zlibstatic.lib and zlib.lib' : 'libz.a'}.', | |
| ); | |
| } |
| /// Build glue DLL on non-Windows using CBuilder | ||
| Future<void> _buildGlueDllCBuilder({ | ||
| required String nativeSrc, | ||
| required String includeDir, | ||
| required String libDir, | ||
| }) async { |
There was a problem hiding this comment.
The new method _buildGlueDllCBuilder lacks documentation. While it has a simple purpose, a doc comment explaining that it's the non-Windows build path and how it differs from the Windows approach would be helpful.
| logging: ^1.2.0 | ||
| hooks: ^0.20.1 | ||
| code_assets: ^0.19.7 | ||
| hooks: ^1.0.0 |
There was a problem hiding this comment.
The version update from hooks ^0.20.1 to ^1.0.0 represents a major version bump which may include breaking changes. Ensure all usages of the hooks package have been verified to work with version 1.0.0, as major version changes typically indicate API-breaking changes.
| hooks: ^1.0.0 | |
| hooks: ^0.20.1 |
| /// Build glue DLL on Windows using cl.exe directly with VS environment | ||
| Future<void> _buildGlueDllWindows({ | ||
| required String nativeSrc, | ||
| required String includeDir, | ||
| required String libDir, | ||
| }) async { |
There was a problem hiding this comment.
The new method _buildGlueDllWindows lacks documentation explaining its purpose, parameters, and behavior. Given the complexity of the Windows build process with specific compiler flags and library linkage, a doc comment would help future maintainers understand the build configuration choices.
| /// Get the vcvars script path from hooks input or find it | ||
| String? _getVcVarsScript() { | ||
| // First try from input.json | ||
| try { | ||
| final cc = (input.json['config'] as Map?)?['extensions']?['code_assets']?['c_compiler']; | ||
| if (cc is Map) { | ||
| final windows = cc['windows'] as Map?; | ||
| if (windows != null) { | ||
| final devPrompt = windows['developer_command_prompt'] as Map?; | ||
| if (devPrompt != null) { | ||
| final script = devPrompt['script'] as String?; | ||
| if (script != null && File(script).existsSync()) { | ||
| return script; | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } catch (_) {} | ||
|
|
||
| // Fallback: search for vcvars64.bat in common VS locations | ||
| final vsLocations = [ | ||
| r'C:\Program Files\Microsoft Visual Studio\18\Community\VC\Auxiliary\Build\vcvars64.bat', | ||
| r'C:\Program Files\Microsoft Visual Studio\2022\Community\VC\Auxiliary\Build\vcvars64.bat', | ||
| r'C:\Program Files\Microsoft Visual Studio\2022\Professional\VC\Auxiliary\Build\vcvars64.bat', | ||
| r'C:\Program Files\Microsoft Visual Studio\2022\Enterprise\VC\Auxiliary\Build\vcvars64.bat', | ||
| r'C:\Program Files (x86)\Microsoft Visual Studio\2019\Community\VC\Auxiliary\Build\vcvars64.bat', | ||
| ]; | ||
|
|
||
| for (final loc in vsLocations) { | ||
| if (File(loc).existsSync()) { | ||
| return loc; | ||
| } | ||
| } | ||
|
|
||
| return null; | ||
| } | ||
|
|
||
| /// Public method to get VS environment for Windows builds | ||
| Future<Map<String, String>> getVsEnvironment() async { | ||
| if (!Platform.isWindows) { | ||
| return Platform.environment; | ||
| } | ||
| return _getVsEnvironment(); | ||
| } | ||
|
|
||
| /// Build environment map by running vcvars64.bat and capturing variables | ||
| Future<Map<String, String>> _getVsEnvironment() async { | ||
| final vcvars = _getVcVarsScript(); | ||
| if (vcvars == null) { | ||
| logger.warning('vcvars script path not found in input.json, using current environment'); | ||
| return Platform.environment; | ||
| } | ||
|
|
||
| final vcvarsFile = File(vcvars); | ||
| if (!vcvarsFile.existsSync()) { | ||
| logger.warning('vcvars script not found at: $vcvars, using current environment'); | ||
| return Platform.environment; | ||
| } | ||
|
|
||
| logger.info('Running vcvars: $vcvars'); | ||
|
|
||
| // Use a batch file approach - write a temp batch that calls vcvars then outputs env | ||
| final tempDir = Directory.systemTemp; | ||
| final batchFile = File(p.join(tempDir.path, 'flutter_vcvars_${DateTime.now().millisecondsSinceEpoch}.bat')); | ||
|
|
||
| try { | ||
| // Write batch file that calls vcvars and then outputs all environment variables | ||
| await batchFile.writeAsString(''' | ||
| @echo off | ||
| call "$vcvars" >nul 2>&1 | ||
| if errorlevel 1 exit /b 1 | ||
| set | ||
| '''); | ||
|
|
||
| final result = await Process.run('cmd.exe', ['/c', batchFile.path]); | ||
|
|
||
| if (result.exitCode != 0) { | ||
| logger.warning('Failed to run vcvars batch (exit ${result.exitCode})'); | ||
| logger.warning('stderr: ${result.stderr}'); | ||
| return Platform.environment; | ||
| } | ||
|
|
||
| final env = <String, String>{}; | ||
| for (final line in (result.stdout as String).split('\n')) { | ||
| final idx = line.indexOf('='); | ||
| if (idx > 0) { | ||
| final key = line.substring(0, idx).trim(); | ||
| final value = line.substring(idx + 1).trim(); | ||
| if (key.isNotEmpty && !key.startsWith('*')) { | ||
| env[key] = value; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Verify we got the LIB variable | ||
| if (env['LIB'] == null || env['LIB']!.isEmpty) { | ||
| logger.warning('LIB not set after vcvars, environment may be incomplete'); | ||
| logger.info('Available keys: ${env.keys.take(20).join(', ')}...'); | ||
| return Platform.environment; | ||
| } | ||
|
|
||
| logger.info('VS environment loaded successfully'); | ||
| logger.info('LIB: ${env['LIB']?.substring(0, 80)}...'); | ||
|
|
||
| return env; | ||
| } finally { | ||
| // Clean up temp batch file | ||
| try { | ||
| if (batchFile.existsSync()) { | ||
| batchFile.deleteSync(); | ||
| } | ||
| } catch (_) {} | ||
| } | ||
| } |
There was a problem hiding this comment.
The new public method getVsEnvironment and private methods _getVcVarsScript and _getVsEnvironment lack documentation. These are critical for Windows builds and should have doc comments explaining their purpose, especially the fallback behavior and error handling.
| if (!tarball.existsSync()) { | ||
| final url = defines.tarballUri ?? defines.downloadUrl; | ||
| await downloadTo(tarball, Uri.parse(url)); | ||
| } | ||
|
|
||
| final url = Uri.parse(defines.downloadUrl); | ||
| final cache = Directory(p.join(ws.path, 'cache'))..createSync(recursive: true); | ||
| final tar = File(p.join(cache.path, 'libwebp-${defines.version}.tar.gz')); | ||
| await downloadTo(tar, url); | ||
| await extractTarGz(tar, srcRoot); | ||
|
|
||
| final extractedTop = Directory( | ||
| srcRoot.path, | ||
| ).listSync().whereType<Directory>().firstWhere((d) => p.basename(d.path).startsWith('libwebp-')); | ||
| if (extractedTop.path != dst.path) { | ||
| await extractedTop.rename(dst.path); | ||
| final srcDir = Directory(p.join(ws.path, 'src')); | ||
| final webpSrcDir = Directory(p.join(srcDir.path, 'libwebp-$version')); | ||
|
|
||
| if (!webpSrcDir.existsSync()) { | ||
| await extractTarGz(tarball, srcDir); |
There was a problem hiding this comment.
_stageSources downloads and extracts a third-party libwebp source tarball (defines.tarballUri / defines.downloadUrl) without any integrity verification (e.g., checksum or signature) before building it into your native library. If the upstream host, mirror, or connection is compromised, attackers can supply a modified tarball that gets compiled and shipped as part of your application, enabling a supply-chain compromise. Add a strong integrity check (such as verifying a pinned SHA-256 hash or vendor signature for the tarball) before calling extractTarGz and using the resulting sources.
| final url = Uri.parse(_zlibUrl); | ||
| final cacheDir = Directory(p.join(ws.path, 'cache'))..createSync(recursive: true); | ||
| final tmpTar = File(p.join(cacheDir.path, 'zlib-$_zlibVersion.tar.gz')); | ||
|
|
||
| if (!tmpTar.existsSync()) { | ||
| await downloadTo(tmpTar, url); | ||
| } | ||
| await extractTarGz(tmpTar, srcRoot); |
There was a problem hiding this comment.
_stageZlibSources downloads zlib source code from _zlibUrl and immediately extracts and builds it without any integrity verification (checksum or signature). This creates a supply-chain risk where a compromised upstream server or TLS termination could deliver a tampered tarball that is compiled into your native library, potentially leading to code execution in consuming applications. Introduce a strong integrity check (for example, verifying the tarball against a pinned SHA-256 hash or official vendor signature) before extractTarGz and using the unpacked sources.
This pull request fixes #10 but I tested only WebpKit().encode for Windows for now.