Simplify build-trace gate; remove dynamic subcommand probe#3709
Draft
eseidel wants to merge 2 commits into
Draft
Simplify build-trace gate; remove dynamic subcommand probe#3709eseidel wants to merge 2 commits into
eseidel wants to merge 2 commits into
Conversation
2 tasks
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
`prepareBuildTrace` used `flutterVersion ?? minVersion` to handle revisions that aren't on any `flutter_release/*` branch (dev pins). That fallback is wrong for a feature-enable check: a Shorebird-fork dev revision predating the tracing PR would be treated as new enough, get `--shorebird-trace` forwarded to flutter, and fail argparse hard. Flip `FlutterSupportConstraint.isSatisfiedBy` to accept `Version?` and require an actual version above the floor (or an allowlisted revision) to satisfy it. Unknown version + non-allowlisted revision now leaves the trace session off. The current pin remains covered via the allowlist.
…rt it (#3708)" This reverts commit 2d0f891. #3708 probed each `flutter build <cmd> -h` at runtime to decide whether to pass `--shorebird-trace`. That was a belt-and-suspenders guard around two real bugs: 1. The Shorebird gate mis-classified unresolved Flutter versions (dev revisions not on any `flutter_release/*` branch) as new enough, so it opted pre-feature pins into tracing. 2. `flutter build aar` / `ios-framework` / `linux` / `macos` / `windows` didn't register the option, so the flag couldn't be forwarded to them safely. The previous commit on this branch fixes (1). The companion shorebirdtech/flutter#136 fixes (2) and must be on the Shorebird Flutter pin before this lands. With both in place, every subcommand this CLI invokes accepts the flag when the gate admits tracing, and every subcommand refuses it when the gate rejects tracing — so the per-subcommand runtime probe (plus its 8 per-command call-site detours and its help-output cache) is dead weight.
6709633 to
838924a
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Two changes, stacked:
1.
fix(cli): gate build tracing off for unknown Flutter versionsprepareBuildTraceusedflutterVersion ?? minVersionto handle revisions that aren't on anyflutter_release/*branch (dev pins). That fallback is wrong for a feature-enable check: a Shorebird-fork dev revision predating the tracing PR would be treated as new enough, get--shorebird-traceforwarded to flutter, and fail argparse hard.Flip
FlutterSupportConstraint.isSatisfiedByto acceptVersion?and require either a real version ≥ the floor or an allowlisted revision to satisfy it. Unknown + non-allowlisted now leaves the trace session off. Addedc2c56ab2d5483bdf86152725342f55ca6faed946(the current pin) to the allowlist — previously it was carried implicitly by #3708's runtime probe, which this PR also removes.2.
Revert "fix: only pass --shorebird-trace to build commands that support it (#3708)"#3708 probed each
flutter build <cmd> -hat runtime to decide whether to pass--shorebird-trace. That was a belt-and-suspenders guard around two real bugs:flutter build aar/ios-framework/linux/macos/windowsdidn't register the option (fixed by the companion Extend --shorebird-trace to aar, ios-framework, and desktop builds flutter#136).With both fixes in place, every subcommand this CLI invokes accepts the flag when the gate admits tracing, and every subcommand refuses it when the gate rejects tracing — so the per-subcommand runtime probe (plus its 8 per-command call-site detours and its help-output cache) is dead weight.
Landing order
This PR must land after shorebirdtech/flutter#136 is merged AND
bin/internal/flutter.versionis bumped to a revision that includes it. I reproduced a hard crash onshorebird release macoswhen this simplification is applied without the Flutter side (see Scenario C counter-factual below).Test plan
dart test packages/shorebird_cli/test/src/artifact_builder/artifact_builder_test.dart— 92 tests pass.End-to-end on a fresh test project (
/tmp/shorebird-trace-test/demo):shorebird release android --dry-run --flutter-version be0b343f(a pre-trace Shorebird-fork dev hash, not on anyflutter_release/*branch).Command: flutter build appbundle --release --target-platform=android-arm,android-arm64,android-x64— no--shorebird-trace.git checkout 2d0f8919~1, same command):Could not find an option named "--shorebird-trace". Command: flutter build appbundle ... --shorebird-trace=/…/build-trace-android.json.shorebird release android --dry-run(pin is allowlisted): builds AAB end-to-end, writesbuild/shorebird/debug/build-trace-android.json(17.8KB of valid Chrome Trace Event Format JSON).shorebird release macos --dry-run:Could not find an option named "--shorebird-trace". Command: flutter build macos --release --shorebird-trace=…. This is the crash that motivates the landing-order note.✓ Built build/macos/Build/Products/Release/demo.app (48.6MB)/No issues detected.