diff --git a/packages/shorebird_cli/lib/src/artifact_builder/artifact_builder.dart b/packages/shorebird_cli/lib/src/artifact_builder/artifact_builder.dart index dc22e30da..31e1cb8b5 100644 --- a/packages/shorebird_cli/lib/src/artifact_builder/artifact_builder.dart +++ b/packages/shorebird_cli/lib/src/artifact_builder/artifact_builder.dart @@ -103,44 +103,6 @@ fails when using the same flutter version, please file an issue: ${link(uri: Uri.parse('https://github.com/shorebirdtech/shorebird/issues/new'))} '''; - /// Cache of `flutter build ` help output checks for - /// `--shorebird-trace` support. Populated lazily by - /// [_supportsTraceFlag]. - final _traceSupport = {}; - - /// Returns whether `flutter build ` accepts `--shorebird-trace`. - /// - /// Probes the command's help output and caches the result per [command] - /// so that subsequent calls for the same command are free. - /// Returns `false` if the help check fails for any reason. - Future _supportsTraceFlag(String command) async { - if (_traceSupport.containsKey(command)) return _traceSupport[command]!; - - try { - final result = await process.run( - 'flutter', - ['build', command, '-h'], - runInShell: false, - ); - final supported = result.stdout.toString().contains('--shorebird-trace'); - _traceSupport[command] = supported; - return supported; - } on Exception { - _traceSupport[command] = false; - return false; - } - } - - /// Returns the `--shorebird-trace` argument if the current - /// [BuildTraceSession] has a trace file and the given [command] - /// supports it, or an empty list otherwise. - Future> _traceArgs(String command) async { - final traceFile = buildTraceSession.traceFile; - if (traceFile == null) return const []; - if (!await _supportsTraceFlag(command)) return const []; - return ['--shorebird-trace=${traceFile.path}']; - } - /// Builds an aab using `flutter build appbundle`. Runs `flutter pub get` with /// the system installation of Flutter to reset /// `.dart_tool/package_config.json` after the build completes or fails. @@ -154,6 +116,7 @@ ${link(uri: Uri.parse('https://github.com/shorebirdtech/shorebird/issues/new'))} await _runShorebirdBuildCommand(() async { const executable = 'flutter'; final targetPlatformArgs = targetPlatforms?.targetPlatformArg; + final traceFile = buildTraceSession.traceFile; final arguments = [ 'build', 'appbundle', @@ -161,7 +124,7 @@ ${link(uri: Uri.parse('https://github.com/shorebirdtech/shorebird/issues/new'))} if (flavor != null) '--flavor=$flavor', if (target != null) '--target=$target', if (targetPlatformArgs != null) '--target-platform=$targetPlatformArgs', - ...await _traceArgs('appbundle'), + if (traceFile != null) '--shorebird-trace=${traceFile.path}', ...args, ]; @@ -221,6 +184,7 @@ Reason: Exited with code $exitCode.''', await _runShorebirdBuildCommand(() async { const executable = 'flutter'; final targetPlatformArgs = targetPlatforms?.targetPlatformArg; + final traceFile = buildTraceSession.traceFile; final arguments = [ 'build', 'apk', @@ -233,7 +197,7 @@ Reason: Exited with code $exitCode.''', // coverage:ignore-start if (splitPerAbi) '--split-per-abi', // coverage:ignore-end - ...await _traceArgs('apk'), + if (traceFile != null) '--shorebird-trace=${traceFile.path}', ...args, ]; @@ -290,6 +254,7 @@ Reason: Exited with code $exitCode.''', return _runShorebirdBuildCommand(() async { const executable = 'flutter'; final targetPlatformArgs = targetPlatforms?.targetPlatformArg; + final traceFile = buildTraceSession.traceFile; final arguments = [ 'build', 'aar', @@ -297,7 +262,7 @@ Reason: Exited with code $exitCode.''', '--no-profile', '--build-number=$buildNumber', if (targetPlatformArgs != null) '--target-platform=$targetPlatformArgs', - ...await _traceArgs('aar'), + if (traceFile != null) '--shorebird-trace=${traceFile.path}', ...args, ]; @@ -334,12 +299,13 @@ Reason: Exited with code $exitCode.''', }) async { await _runShorebirdBuildCommand(() async { const executable = 'flutter'; + final traceFile = buildTraceSession.traceFile; final arguments = [ 'build', 'linux', '--release', if (target != null) '--target=$target', - ...await _traceArgs('linux'), + if (traceFile != null) '--shorebird-trace=${traceFile.path}', ...args, ]; @@ -387,6 +353,7 @@ Reason: Exited with code $exitCode.''', String? appDillPath; await _runShorebirdBuildCommand(() async { const executable = 'flutter'; + final traceFile = buildTraceSession.traceFile; final arguments = [ 'build', 'macos', @@ -394,7 +361,7 @@ Reason: Exited with code $exitCode.''', if (flavor != null) '--flavor=$flavor', if (target != null) '--target=$target', if (!codesign) '--no-codesign', - ...await _traceArgs('macos'), + if (traceFile != null) '--shorebird-trace=${traceFile.path}', ...args, ]; final buildStart = clock.now(); @@ -454,6 +421,7 @@ Reason: Exited with code $exitCode.''', String? appDillPath; await _runShorebirdBuildCommand(() async { const executable = 'flutter'; + final traceFile = buildTraceSession.traceFile; final arguments = [ 'build', 'ipa', @@ -461,7 +429,7 @@ Reason: Exited with code $exitCode.''', if (flavor != null) '--flavor=$flavor', if (target != null) '--target=$target', if (!codesign) '--no-codesign', - ...await _traceArgs('ipa'), + if (traceFile != null) '--shorebird-trace=${traceFile.path}', ...args, ]; @@ -517,12 +485,13 @@ Reason: Exited with code $exitCode.''', String? appDillPath; await _runShorebirdBuildCommand(() async { const executable = 'flutter'; + final traceFile = buildTraceSession.traceFile; final arguments = [ 'build', 'ios-framework', '--no-debug', '--no-profile', - ...await _traceArgs('ios-framework'), + if (traceFile != null) '--shorebird-trace=${traceFile.path}', ...args, ]; @@ -582,10 +551,13 @@ Reason: Exited with code $exitCode.''', final flutterVersion = await shorebirdFlutter.resolveFlutterVersion( revision, ); - // Treat an unknown version (e.g. a pinned dev revision) as new enough, - // matching the pattern used for other version-gated features. + // When resolveFlutterVersion returns null (dev revision not on any + // flutter_release/* branch), the gate falls through to the allowlist. + // That's intentional: a dev pin predating the tracing PR would otherwise + // get `--shorebird-trace` passed to a flutter build that doesn't know + // the flag, and fail hard. final supportsTrace = buildTraceSupportConstraint.isSatisfiedBy( - version: flutterVersion ?? buildTraceSupportConstraint.minVersion, + version: flutterVersion, revision: revision, ); if (!supportsTrace) { @@ -793,12 +765,13 @@ Either run `flutter pub get` manually, or follow the steps in ${cannotRunInVSCod }) async { await _runShorebirdBuildCommand(() async { const executable = 'flutter'; + final traceFile = buildTraceSession.traceFile; final arguments = [ 'build', 'windows', '--release', if (target != null) '--target=$target', - ...await _traceArgs('windows'), + if (traceFile != null) '--shorebird-trace=${traceFile.path}', ...args, ]; diff --git a/packages/shorebird_cli/lib/src/flutter_version_constraints.dart b/packages/shorebird_cli/lib/src/flutter_version_constraints.dart index 7d46bdd82..8cd0f6713 100644 --- a/packages/shorebird_cli/lib/src/flutter_version_constraints.dart +++ b/packages/shorebird_cli/lib/src/flutter_version_constraints.dart @@ -61,8 +61,15 @@ class FlutterSupportConstraint { final Set allowedRevisions; /// Whether the given [version]/[revision] pair satisfies this constraint. - bool isSatisfiedBy({required Version version, required String revision}) => - version >= minVersion || allowedRevisions.contains(revision); + /// + /// A null [version] (e.g. a Shorebird-fork dev revision that isn't on any + /// `flutter_release/*` branch and so has no parseable upstream version) + /// can only satisfy the constraint via [allowedRevisions]. Gating on + /// `version ?? minVersion` would treat every unknown revision as new + /// enough, silently opting pre-feature dev pins into feature behavior. + bool isSatisfiedBy({required Version? version, required String revision}) => + (version != null && version >= minVersion) || + allowedRevisions.contains(revision); } /// Flutter support for `flutter build --shorebird-trace=` for emitting @@ -76,9 +83,15 @@ class FlutterSupportConstraint { final buildTraceSupportConstraint = FlutterSupportConstraint( minVersion: Version(3, 41, 7), allowedRevisions: { - // Current Shorebird Flutter pin (bin/internal/flutter.version). Can - // be removed once a flutter_release/3.41.7 branch ships with this - // (or a later tracing-enabled) commit at its tip. + // Shorebird-fork Flutter pins that ship the tracing feature but + // still report upstream version 3.41.6, so `resolveFlutterVersion` + // can't satisfy the floor via the version path. + // + // Add a new entry here every time `bin/internal/flutter.version` + // is bumped to a revision that carries the tracing PRs; entries + // stop mattering once a `flutter_release/3.41.7` branch ships + // them and the floor can be satisfied directly. '3b10eecea184bb381f1045a878eeff36548ed11e', + 'c2c56ab2d5483bdf86152725342f55ca6faed946', }, ); diff --git a/packages/shorebird_cli/test/src/artifact_builder/artifact_builder_test.dart b/packages/shorebird_cli/test/src/artifact_builder/artifact_builder_test.dart index 4a92381e7..12e12ebe4 100644 --- a/packages/shorebird_cli/test/src/artifact_builder/artifact_builder_test.dart +++ b/packages/shorebird_cli/test/src/artifact_builder/artifact_builder_test.dart @@ -98,25 +98,6 @@ void main() { when( () => pubGetProcessResult.exitCode, ).thenReturn(ExitCode.success.code); - // Default stub for the `flutter build -h` probe used by - // `_supportsTraceFlag`. Returns help text that includes - // `--shorebird-trace` so that trace tests pass when tracing is enabled. - // Tests that need to verify the flag is NOT passed should either use a - // Flutter version below the trace threshold (the default 3.0.0) or - // override this stub. - when( - () => shorebirdProcess.run( - 'flutter', - any(), - runInShell: false, - ), - ).thenAnswer( - (_) async => ShorebirdProcessResult( - exitCode: ExitCode.success.code, - stdout: '--shorebird-trace', - stderr: '', - ), - ); when( () => shorebirdProcess.stream( any(), @@ -332,59 +313,6 @@ Either run `flutter pub get` manually, or follow the steps in ${cannotRunInVSCod ).called(1); }); - test('caches trace flag probe result across calls', () async { - when( - () => shorebirdFlutter.resolveFlutterVersion(any()), - ).thenAnswer((_) async => Version(3, 41, 7)); - - await runWithOverrides(() async { - await builder.prepareBuildTrace(platform: 'android'); - await builder.buildAppBundle(); - await builder.buildAppBundle(); - }); - - // The help probe should only be called once despite two builds. - verify( - () => shorebirdProcess.run( - 'flutter', - ['build', 'appbundle', '-h'], - runInShell: false, - ), - ).called(1); - }); - - test('skips trace when help probe throws', () async { - when( - () => shorebirdFlutter.resolveFlutterVersion(any()), - ).thenAnswer((_) async => Version(3, 41, 7)); - when( - () => shorebirdProcess.run( - 'flutter', - ['build', 'appbundle', '-h'], - runInShell: false, - ), - ).thenThrow(Exception('process failed')); - - await runWithOverrides(() async { - await builder.prepareBuildTrace(platform: 'android'); - await builder.buildAppBundle(); - }); - - verify( - () => shorebirdProcess.stream( - 'flutter', - [ - 'build', - 'appbundle', - '--release', - ], - environment: any(named: 'environment'), - runInShell: false, - onStart: any(named: 'onStart'), - ), - ).called(1); - }); - group('when base64PublicKey is not null', () { const base64PublicKey = 'base64PublicKey'; @@ -913,30 +841,24 @@ Either run `flutter pub get` manually, or follow the steps in ${cannotRunInVSCod const buildNumber = '1.0'; test( - 'skips --shorebird-trace when help probe shows no support', + 'passes --shorebird-trace when Flutter supports build tracing', () async { when( () => shorebirdFlutter.resolveFlutterVersion(any()), ).thenAnswer((_) async => Version(3, 41, 7)); - when( - () => shorebirdProcess.run( - 'flutter', - ['build', 'aar', '-h'], - runInShell: false, - ), - ).thenAnswer( - (_) async => ShorebirdProcessResult( - exitCode: 0, - stdout: 'no trace flag here', - stderr: '', - ), - ); await runWithOverrides(() async { await builder.prepareBuildTrace(platform: 'android'); await builder.buildAar(buildNumber: buildNumber); }); + final expectedTracePath = p.join( + projectRoot.path, + 'build', + 'shorebird', + 'debug', + 'build-trace-android.json', + ); verify( () => shorebirdProcess.stream( 'flutter', @@ -946,6 +868,7 @@ Either run `flutter pub get` manually, or follow the steps in ${cannotRunInVSCod '--no-debug', '--no-profile', '--build-number=1.0', + '--shorebird-trace=$expectedTracePath', ], environment: any(named: 'environment'), runInShell: false, @@ -1115,30 +1038,24 @@ Either run `flutter pub get` manually, or follow the steps in ${cannotRunInVSCod }); test( - 'skips --shorebird-trace when help probe shows no support', + 'passes --shorebird-trace when Flutter supports build tracing', () async { when( () => shorebirdFlutter.resolveFlutterVersion(any()), ).thenAnswer((_) async => Version(3, 41, 7)); - when( - () => shorebirdProcess.run( - 'flutter', - ['build', 'linux', '-h'], - runInShell: false, - ), - ).thenAnswer( - (_) async => ShorebirdProcessResult( - exitCode: 0, - stdout: 'no trace flag here', - stderr: '', - ), - ); await runWithOverrides(() async { await builder.prepareBuildTrace(platform: 'linux'); await builder.buildLinuxApp(); }); + final expectedTracePath = p.join( + projectRoot.path, + 'build', + 'shorebird', + 'debug', + 'build-trace-linux.json', + ); verify( () => shorebirdProcess.stream( 'flutter', @@ -1146,6 +1063,7 @@ Either run `flutter pub get` manually, or follow the steps in ${cannotRunInVSCod 'build', 'linux', '--release', + '--shorebird-trace=$expectedTracePath', ], environment: any(named: 'environment'), runInShell: false, @@ -1292,30 +1210,24 @@ Reason: Exited with code 70.'''), }); test( - 'skips --shorebird-trace when help probe shows no support', + 'passes --shorebird-trace when Flutter supports build tracing', () async { when( () => shorebirdFlutter.resolveFlutterVersion(any()), ).thenAnswer((_) async => Version(3, 41, 7)); - when( - () => shorebirdProcess.run( - 'flutter', - ['build', 'macos', '-h'], - runInShell: false, - ), - ).thenAnswer( - (_) async => ShorebirdProcessResult( - exitCode: 0, - stdout: 'no trace flag here', - stderr: '', - ), - ); await runWithOverrides(() async { await builder.prepareBuildTrace(platform: 'macos'); await builder.buildMacos(); }); + final expectedTracePath = p.join( + projectRoot.path, + 'build', + 'shorebird', + 'debug', + 'build-trace-macos.json', + ); verify( () => shorebirdProcess.stream( 'flutter', @@ -1323,6 +1235,7 @@ Reason: Exited with code 70.'''), 'build', 'macos', '--release', + '--shorebird-trace=$expectedTracePath', ], environment: any(named: 'environment'), runInShell: false, @@ -1781,30 +1694,24 @@ Reason: Exited with code 70.'''), }); test( - 'skips --shorebird-trace when help probe shows no support', + 'passes --shorebird-trace when Flutter supports build tracing', () async { when( () => shorebirdFlutter.resolveFlutterVersion(any()), ).thenAnswer((_) async => Version(3, 41, 7)); - when( - () => shorebirdProcess.run( - 'flutter', - ['build', 'ios-framework', '-h'], - runInShell: false, - ), - ).thenAnswer( - (_) async => ShorebirdProcessResult( - exitCode: 0, - stdout: 'no trace flag here', - stderr: '', - ), - ); await runWithOverrides(() async { await builder.prepareBuildTrace(platform: 'ios'); await builder.buildIosFramework(); }); + final expectedTracePath = p.join( + projectRoot.path, + 'build', + 'shorebird', + 'debug', + 'build-trace-ios.json', + ); verify( () => shorebirdProcess.stream( 'flutter', @@ -1813,6 +1720,7 @@ Reason: Exited with code 70.'''), 'ios-framework', '--no-debug', '--no-profile', + '--shorebird-trace=$expectedTracePath', ], environment: any(named: 'environment'), runInShell: false, @@ -2073,30 +1981,24 @@ Reason: Exited with code 70.'''), }); test( - 'skips --shorebird-trace when help probe shows no support', + 'passes --shorebird-trace when Flutter supports build tracing', () async { when( () => shorebirdFlutter.resolveFlutterVersion(any()), ).thenAnswer((_) async => Version(3, 41, 7)); - when( - () => shorebirdProcess.run( - 'flutter', - ['build', 'windows', '-h'], - runInShell: false, - ), - ).thenAnswer( - (_) async => ShorebirdProcessResult( - exitCode: 0, - stdout: 'no trace flag here', - stderr: '', - ), - ); await runWithOverrides(() async { await builder.prepareBuildTrace(platform: 'windows'); await builder.buildWindowsApp(); }); + final expectedTracePath = p.join( + projectRoot.path, + 'build', + 'shorebird', + 'debug', + 'build-trace-windows.json', + ); verify( () => shorebirdProcess.stream( 'flutter', @@ -2104,6 +2006,7 @@ Reason: Exited with code 70.'''), 'build', 'windows', '--release', + '--shorebird-trace=$expectedTracePath', ], environment: any(named: 'environment'), runInShell: false, @@ -2266,15 +2169,37 @@ Reason: Exited with code 70.'''), ); test( - 'treats unresolved Flutter version as new enough (dev pin)', + 'leaves traceFile null when version is unresolved and revision is not ' + 'allowlisted', () async { // resolveFlutterVersion returns null for revisions not on a - // flutter_release branch (e.g. a pinned dev revision). The - // minVersion fallback admits these. + // flutter_release branch (e.g. a pinned dev revision predating + // the tracing PR). Those must not opt into tracing, or flutter + // build will fail on an unknown --shorebird-trace option. when( () => shorebirdFlutter.resolveFlutterVersion(any()), ).thenAnswer((_) async => null); + await runWithOverrides(() async { + await builder.prepareBuildTrace(platform: 'android'); + expect(buildTraceSession.traceFile, isNull); + }); + }, + ); + + test( + 'sets traceFile when version is unresolved but revision is allowlisted', + () async { + // A current Shorebird dev pin that does include the tracing PR + // typically has no parseable version (not on a flutter_release + // branch) — the allowlist is the bridge that admits it. + when( + () => shorebirdFlutter.resolveFlutterVersion(any()), + ).thenAnswer((_) async => null); + when(() => shorebirdEnv.flutterRevision).thenReturn( + buildTraceSupportConstraint.allowedRevisions.first, + ); + await runWithOverrides(() async { await builder.prepareBuildTrace(platform: 'android'); expect(buildTraceSession.traceFile, isNotNull);