chore: harden repo CI, docs, license and native logging - #28
chore: harden repo CI, docs, license and native logging#28joshuakrueger-dfx wants to merge 1 commit into
Conversation
Build the native Android (example APK) and iOS (example, --no-codesign) code in CI so native-only changes can no longer pass every check while breaking the consuming app's build. Drop the stale flutter-create template tests across Android, iOS and macOS (and their Xcode/Gradle wiring) that exercised a getPlatformVersion the plugin never implemented; remove the matching template getPlatformVersion handler from the macOS plugin so it is consistent with the Android and iOS bridges. Add a real MIT LICENSE, a source-accurate README and CHANGELOG (0.0.1-0.0.8), fill pubspec metadata, and log previously swallowed native error paths. Closes DFXswiss#27
|
@marassteiner Review request — formal GitHub reviewer assignment is not available for this repo (collaborator permission). Please review when you can. |
|
EN: Working on this now — job |
marassteiner
left a comment
There was a problem hiding this comment.
Recommendation: Approve
No merge-blockers identified. CI is green (Flutter analyze+test, Go tests, YAML lint, Android example build, iOS example build). Follow-up items only — filed as issues and linked below.
Gate A — Diff review (Konformität + Logik)
Two independent reviews of origin/develop...HEAD (6a45c97 vs merge-base 6d70a85).
Behavior / correctness (no blockers)
| Area | Verdict |
|---|---|
InitBitBoxOperation.kt / CloseOperation.kt / ConnectBitBoxOperation.kt |
Control flow unchanged; only Log.w("bitbox_flutter", …) on existing error paths. No seed/key/serial in log strings. |
| Template test removal (Android/iOS/macOS) | Dead getPlatformVersion flutter-create stubs only; no product coverage lost. No dangling RunnerTests refs left in pbxproj/schemes/Podfiles. |
macOS getPlatformVersion drop |
Consistent with Android/iOS bridges; not part of the Dart platform API. |
| New CI jobs | Plausible (example/, Flutter 3.41.6, APK debug / iOS --no-codesign). |
| LICENSE / README / pubspec metadata | Appropriate hardening; README APIs checked against BitboxUsbPlatform. |
Follow-up findings (not merge-blockers)
-
Docs drift — CONTRIBUTING PR gate table
CONTRIBUTING.md:13-19still says three jobs and documentsflutter analyze --no-fatal-infos, while the workflow now has five jobs and already uses--fatal-infos.
→ #36 -
Branch lag / CHANGELOG vs develop
PR rewritesCHANGELOG.mdfrom0.0.8downward;developalready has0.0.9/0.0.10(#29/#30). merge-tree:CHANGELOG.mdchanged in both. Rebase before merge and keep the union of both histories.pubspec.yaml:3remains0.0.1(pre-existing vs tags throughv0.0.10).
→ #37 -
No macOS CI for macOS native edits
Android/iOS example builds added; macOS plugin + Xcode cleanup still unguarded in CI.
→ #38
Note on severity: One review lane initially labeled the CONTRIBUTING / version-metadata gaps as merge-blockers. Against the project merge-blocker bar (production harm, security, red CI, mandatory CONTRIBUTING violation that takes effect on merge), both are documentation / process items only — hence follow-up, not request-changes.
Gate B — Local software run
Host: review machine with Flutter SDK at /Users/dfxai/sdk/flutter (3.44.9; CI pins 3.41.6).
| Command | Result |
|---|---|
flutter pub get |
OK |
dart format --set-exit-if-changed … |
OK (0 changed) |
flutter analyze --fatal-infos |
OK — no issues |
flutter test |
OK — 11/11 |
go vet ./... + go test -race -timeout 60s ./... (in go/) |
OK — api + u2fhid |
cd example && flutter build apk --debug |
Skipped (env) — no Android SDK / ANDROID_HOME on this host |
cd example && flutter build ios --no-codesign |
Skipped (env) — Xcode not installed (CLT only; “Application not configured for iOS”) |
Native-build failures are Case 2 (environmental): host lacks Android SDK and Xcode; the PR does not introduce that dependency gap, and GitHub Actions already passed both new jobs on this PR. Core plugin gate (format / analyze / unit tests / Go) exercised successfully here.
CI (remote, all pass): Android example build, iOS example build, Flutter analyze + test, Go unit tests, Workflow YAML lint.
Summary
Approve. Please rebase onto current develop before merge so CHANGELOG keeps 0.0.9/0.0.10 (#37). Follow-ups: #36, #37, #38.
Closes #27
Repo hardening across CI, tests, docs, license and native error logging. Each item from #27 addressed; scope kept tight and consistent with the existing project conventions.
High priority
pull-request.yamlgains two jobs alongside the existing flutter/go/yaml-lint:android(ubuntu, JDK 17,flutter build apk --debugofexample/) andios(macos-latest,flutter build ios --no-codesignofexample/). A native-only change can no longer pass every check while breaking the consuming app's build.flutter createleftovers that exercised agetPlatformVersionthe plugin never implements: the Android Kotlin test (wrong packagecom.cakewallet.bitbox) and the iOSRunnerTests.swift, including a clean removal of theRunnerTeststarget from the Xcode project, scheme and Podfile (no dangling refs). The now-orphaned JUnit5/mockito wiring inandroid/build.gradleis removed too. For consistency the identical macOS template stub (test + the templategetPlatformVersionhandler) is removed as well.Medium priority
run_build_tool_android.sh,ffigen_config.yaml). Every referenced API verified against source.description, plushomepage/repository/issue_tracker.Low priority
InitBitBoxOperation,CloseOperation,ConnectBitBoxOperation) now emitLog.w("bitbox_flutter", ...)on the error path; no behaviour change.Out of scope per the issue: no full native unit-test coverage, vendored gomobile artifacts left as-is.
Verification (local, Flutter 3.41.6 — the CI pin)
dart formatclean ·flutter analyze --fatal-infosclean ·flutter test11/11go vet+go testgreenflutter build apk --debug(example) → builtapp-debug.apkflutter build ios --no-codesign(example) → builtRunner.appflutter build macos --debug(example) → built, confirming the macOS project edits compile