From c87cd30aeafc49cd30b820676510c86a2cb3cc2a Mon Sep 17 00:00:00 2001 From: Vedant Madane <6527493+VedantMadane@users.noreply.github.com> Date: Wed, 26 Aug 2026 23:56:39 +0530 Subject: [PATCH] fix: skip chmod on Windows so generation does not half-write MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Process.runSync('chmod', ...) throws ProcessException on Windows when chmod is missing, aborting the write loop mid-package. Skip the call on Windows (execute bit is meaningless on NTFS), check chmod exit code on Unix, keep Android package paths as POSIX separators, and document that tool/fetch_*.sh need WSL/Git Bash. Windows coverage is a separate `test-windows` job rather than a matrix on `test`. A matrixed job reports `test (ubuntu-latest)` and `test (windows-latest)` and never `test` — the name branch protection on main requires — so matrixing it would leave every later PR waiting on a check that is no longer reported. Fixes #3 Signed-off-by: Vedant Madane <6527493+VedantMadane@users.noreply.github.com> --- .github/ISSUE_TEMPLATE/bug_report.md | 2 +- .github/workflows/ci.yml | 31 +++++++++++++++++ CHANGELOG.md | 9 ++++- README.md | 7 ++++ bin/binary_sdk_bridge.dart | 6 ++++ lib/src/generator.dart | 27 +++++++++++++-- lib/src/spec.dart | 4 ++- test/generator_test.dart | 52 ++++++++++++++++++++++++++++ 8 files changed, 132 insertions(+), 6 deletions(-) diff --git a/.github/ISSUE_TEMPLATE/bug_report.md b/.github/ISSUE_TEMPLATE/bug_report.md index 67dfa97..be50d1d 100644 --- a/.github/ISSUE_TEMPLATE/bug_report.md +++ b/.github/ISSUE_TEMPLATE/bug_report.md @@ -20,5 +20,5 @@ dart run bin/binary_sdk_bridge.dart --name ... --org ... **Environment** - Dart version: -- Host platform (macOS / Linux): +- Host platform (macOS / Linux / Windows): - Consumer (Flutter version, or plain Xcode/Gradle): diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 205bdf4..41cb9e2 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -9,6 +9,11 @@ permissions: contents: read jobs: + # Deliberately NOT a matrix. A matrixed job reports its checks as + # `test (ubuntu-latest)` etc., never `test` — and `test` is the check name + # branch protection on main requires, so matrixing this job would leave every + # future PR waiting forever on a check that is no longer reported. Windows + # lives in its own job below instead. test: runs-on: ubuntu-latest steps: @@ -24,6 +29,7 @@ jobs: # The generator's real contract is that its OUTPUT works, not that its # own tests pass. Generate both flavours and check the results hold up. + # Bash-only, hence Linux-only — this is why it stays in this job. - name: Generated output is valid run: | set -euo pipefail @@ -40,6 +46,13 @@ jobs: exit 1 fi + # Nor an unrendered @...@ script placeholder. + if grep -rqE '@[A-Z][A-Z_]*@' /tmp/flutter /tmp/native; then + echo "::error::an unrendered @...@ placeholder reached generated output" + grep -rnE '@[A-Z][A-Z_]*@' /tmp/flutter /tmp/native + exit 1 + fi + # The generated shell scripts must at least parse. for s in /tmp/flutter/acme_ads/tool/*.sh /tmp/native/acme_sdk/tool/*.sh; do bash -n "$s" @@ -47,3 +60,21 @@ jobs: # The native flavour must not drag Dart in. test ! -f /tmp/native/acme_sdk/pubspec.yaml + + # Windows coverage for the generator itself (#3: chmod, path separators). + # Separate job, separate check name, so the required `test` check above keeps + # being reported. Not required to merge — a failure here is visible on the PR + # without blocking, which is the trade for not touching branch protection. + test-windows: + runs-on: windows-latest + steps: + - uses: actions/checkout@v4 + - uses: dart-lang/setup-dart@v1 + with: + sdk: stable + + - run: dart pub get + - run: dart analyze --fatal-infos + # No `dart format --set-exit-if-changed` here: it is host-independent and + # already gated above, and the Windows runner checks out CRLF. + - run: dart test diff --git a/CHANGELOG.md b/CHANGELOG.md index 72fa0e3..d45efca 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,7 +2,8 @@ ## Unreleased -Fixes found by running the README end to end as a new user would. +Fixes found by running the README end to end as a new user would, plus +Windows support for the generator itself. - The generated `tool/fetch_ios_sdk.sh` ran `flutter build` while trying to explain it: the backticks in the advice line were command substitution @@ -20,6 +21,12 @@ Fixes found by running the README end to end as a new user would. Flutter's `Runner.app`. - The README's install step was missing entirely, and its example commands (`dart run binary_sdk_bridge`) could not work from a fresh checkout. +- Skip `chmod +x` on Windows so generation no longer throws + `ProcessException` mid-write and leaves a half-written package (#3). On + Unix a non-zero `chmod` exit is now surfaced instead of ignored. +- Keep Android package relative paths with `/` separators on Windows so + planned and written layouts match other hosts. +- On Windows the CLI notes that `tool/fetch_*.sh` need WSL or Git Bash. ## 0.1.0 diff --git a/README.md b/README.md index 80404fb..a94b1dc 100644 --- a/README.md +++ b/README.md @@ -183,6 +183,13 @@ reconciliation. Not generator-enforced — it is a consumer-side pattern — but the generated bridge's callback contract is shaped to make it easy to get right. +## Host platforms + +Generation works on macOS, Linux, and Windows. The generated `tool/fetch_*.sh` +scripts are bash (`#!/usr/bin/env bash`, `curl`, `shasum`, `unzip`) — on +Windows run them from WSL or Git Bash. Building the iOS `.xcframework` half +still needs macOS; Android generation and Gradle builds are fine on Windows. + ## Options | Flag | Default | | diff --git a/bin/binary_sdk_bridge.dart b/bin/binary_sdk_bridge.dart index 633a1a5..3381e6c 100644 --- a/bin/binary_sdk_bridge.dart +++ b/bin/binary_sdk_bridge.dart @@ -126,6 +126,12 @@ void main(List arguments) { ..writeln() ..writeln('Next:'); generator.nextSteps(root.path).forEach(stdout.writeln); + if (Platform.isWindows) { + stdout + ..writeln() + ..writeln('Note: tool/fetch_*.sh are bash scripts (curl, shasum, unzip). ' + 'Run them from WSL or Git Bash — not from cmd.exe or PowerShell.'); + } } String _usage(ArgParser parser) => ''' diff --git a/lib/src/generator.dart b/lib/src/generator.dart index 2e9e607..d219ef0 100644 --- a/lib/src/generator.dart +++ b/lib/src/generator.dart @@ -154,12 +154,33 @@ class BridgeGenerator { target.parent.createSync(recursive: true); target.writeAsStringSync(file.contents); if (file.executable) { - // Dart has no chmod; the shell one is universal on the platforms that - // can build these packages at all. - Process.runSync('chmod', ['+x', target.path]); + _markExecutable(target); } } return packageRoot; } } + +/// Sets the owner-execute bit on [file]. +/// +/// Skipped on Windows: there is no `chmod`, and +/// `Process.runSync` throws [ProcessException] when the executable is +/// missing rather than returning a non-zero exit code — which used to abort +/// the write loop mid-package (see #3). The execute bit has no meaning on +/// NTFS anyway; [GeneratedFile.executable] still records intent for dry-run +/// and tests. +void _markExecutable(File file) { + if (Platform.isWindows) return; + + final result = Process.runSync('chmod', ['+x', file.path]); + if (result.exitCode != 0) { + final stderr = (result.stderr as String).trim(); + throw ProcessException( + 'chmod', + ['+x', file.path], + stderr.isEmpty ? 'exit code ${result.exitCode}' : stderr, + result.exitCode, + ); + } +} diff --git a/lib/src/spec.dart b/lib/src/spec.dart index 5d4b2fb..89cb4ad 100644 --- a/lib/src/spec.dart +++ b/lib/src/spec.dart @@ -78,7 +78,9 @@ class BridgeSpec { String get androidPackage => '$organization.$pluginName'; - String get androidPackagePath => p.joinAll(androidPackage.split('.')); + // Always `/` — [GeneratedFile.relativePath] is platform-independent, and + // `path.joinAll` would inject `\` on Windows (#3 / Windows generation). + String get androidPackagePath => p.posix.joinAll(androidPackage.split('.')); /// Method-channel name. The `_method` suffix is deliberate: Flutter's binary /// messenger keys handlers by name alone, so an EventChannel added later diff --git a/test/generator_test.dart b/test/generator_test.dart index 59e8a90..0c6f261 100644 --- a/test/generator_test.dart +++ b/test/generator_test.dart @@ -1,6 +1,7 @@ import 'dart:io'; import 'package:binary_sdk_bridge/binary_sdk_bridge.dart'; +import 'package:path/path.dart' as p; import 'package:test/test.dart'; BridgeSpec _spec({ @@ -147,6 +148,26 @@ void main() { ); }); + // #3: `path.joinAll` injects `\` on Windows, which would make the planned + // layout differ per host and break the `relativePath` contract on + // GeneratedFile. The Android package path is the only place the plan + // builds a nested path from user input. + test('the Android package path is POSIX regardless of host', () { + expect(_spec().androidPackagePath, 'com/example/acme_ads'); + }); + + test('every relative path is POSIX, for both flavours', () { + for (final flavor in BridgeFlavor.values) { + for (final file in BridgeGenerator(_spec(flavor: flavor)).plan()) { + expect( + file.relativePath, + isNot(contains(r'\')), + reason: '${file.relativePath} (${flavor.name}) is not POSIX', + ); + } + } + }); + test('omits iOS files when no framework is given', () { final paths = BridgeGenerator(_spec(ios: null)).plan().map((f) => f.relativePath); @@ -454,5 +475,36 @@ void main() { returnsNormally, ); }); + + // Regression for #3: chmod does not exist on Windows, and + // Process.runSync throws ProcessException there rather than returning a + // non-zero exit — which aborted the write loop part-way and left a + // half-written package on disk. Asserting the WHOLE plan landed is the + // real contract, and it is the assertion that fails on Windows without + // the fix. + test('writes every planned file, on every host', () { + final generator = BridgeGenerator(_spec()); + final root = generator.write(tmp.path); + + for (final file in generator.plan()) { + expect( + File(p.join(root.path, p.joinAll(file.relativePath.split('/')))) + .existsSync(), + isTrue, + reason: '${file.relativePath} was planned but not written', + ); + } + }); + + test( + 'writes the fetch scripts executable where the platform has the concept', + () { + final root = BridgeGenerator(_spec()).write(tmp.path); + final script = File(p.join(root.path, 'tool', 'fetch_ios_sdk.sh')); + expect(script.existsSync(), isTrue); + if (!Platform.isWindows) { + expect(script.statSync().mode & 0x40, isNot(0)); // owner +x + } + }); }); }