Skip to content

Build command dependency injection #114383

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 7 commits into from
Nov 8, 2022
Merged

Build command dependency injection #114383

merged 7 commits into from
Nov 8, 2022

Conversation

Jasguerrero
Copy link
Contributor

@Jasguerrero Jasguerrero commented Nov 1, 2022 •
edited
Loading

Part of: #107607

The missing BuildSubCommands will be added in a different PR to minimize breakeges with g3

Pre-launch Checklist

  • I read the [Contributor Guide] and followed the process outlined there for submitting PRs.
  • I read the [Tree Hygiene] wiki page, which explains my responsibilities.
  • I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement].
  • I signed the [CLA].
  • I listed at least one issue that this PR fixes in the description above.
  • I updated/added relevant documentation (doc comments with ///).
  • I added new tests to check the change I am making, or this PR is [test-exempt].
  • All existing and new tests are passing.

@flutter-dashboard flutter-dashboard bot added the tool Affects the "flutter" command-line tool. See also t: labels. label Nov 1, 2022
@Jasguerrero Jasguerrero marked this pull request as ready for review November 1, 2022 17:52
@Jasguerrero
Merge branch 'master' into build_command_dep_inj
40ce3e3 
# Conflicts:
#	packages/flutter_tools/test/commands.shard/hermetic/build_web_test.dart
christopherfujino
christopherfujino reviewed Nov 3, 2022
View reviewed changes
packages/flutter_tools/lib/src/commands/build_web.dart
@@ -15,8 +15,10 @@ import 'build.dart';

class BuildWebCommand extends BuildSubCommand {
BuildWebCommand({
required super.logger,
required FileSystem fileSystem,
required bool verboseHelp,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as a further cleanup, I think this can be required super.verboseHelp, and then we don't have to explicitly call super(verboseHelp: verboseHelp)

christopherfujino
christopherfujino reviewed Nov 3, 2022
View reviewed changes
packages/flutter_tools/lib/src/commands/build_macos.dart
@@ -16,6 +16,7 @@ import 'build.dart';
/// A command to build a macOS desktop target through a build shell script.
class BuildMacosCommand extends BuildSubCommand {
BuildMacosCommand({
required super.logger,
required bool verboseHelp,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this can be required super.verboseHelp,

christopherfujino
christopherfujino reviewed Nov 3, 2022
View reviewed changes
packages/flutter_tools/test/commands.shard/hermetic/build_ios_test.dart
final BuildCommand command = BuildCommand(
androidSdk: FakeAndroidSdk(),
buildSystem: TestBuildSystem.all(BuildResult(success: true)),
fileSystem: MemoryFileSystem.test(),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we need the filesystem override on line 199? And how does the global XcodeProjectInterpreter get called?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general, are we able to remove any of the context overrides from the tests after this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes we still need those overrides because BuildIOSCommand is not yet migrated. As for the XcodeProjectInterpreter comes all the way from lib/src/ios/mac.dart

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ahhh, ok

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there are no overrides throughout all the test files here that can be removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No 😕, wanted to remove the fs but it is used on FlutterCommand

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:'( ok, thanks for checking

christopherfujino
christopherfujino approved these changes Nov 3, 2022
View reviewed changes
Copy link
Contributor

@christopherfujino christopherfujino left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks for this one!

@Jasguerrero
Merge branch 'master' into build_command_dep_inj
65deee8 
# Conflicts:
#	packages/flutter_tools/test/commands.shard/hermetic/build_macos_test.dart
@Jasguerrero
Merge remote-tracking branch 'upstream/master' into build_command_dep…
b3f9121 
…_inj
@Jasguerrero Jasguerrero added the autosubmit Merge PR when tree becomes green via auto submit App label Nov 8, 2022
@auto-submit auto-submit bot merged commit 530324d into flutter:master Nov 8, 2022
This was referenced Nov 9, 2022
Merged
Merged
engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Nov 9, 2022
@engine-flutter-autoroll
530324d Build command dependency injection (flutter/flutter#114383)
ab04c85
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Nov 9, 2022
@engine-flutter-autoroll
530324d Build command dependency injection (flutter/flutter#114383)
d798a8f
auto-submit bot pushed a commit to flutter/packages that referenced this pull request Nov 9, 2022
@engine-flutter-autoroll
Roll Flutter from a1432a9 to d3dcd7d (8 revisions) (#2791)
4f7b952 
* a84e369 Revert "Allow Flutter golden file tests to be flaky (#114450)" (flutter/flutter#114902)

* 1725a26 Switch the way we retrieve the vm_service_port from /hub to iquery, on device. (flutter/flutter#114834)

* 7020f59 [tool] Support `--flavor` option for `flutter install`. (flutter/flutter#114048)

* 9797d5f [macOS] Refactor the `flutter run` macOS console output test (flutter/flutter#114645)

* 530324d Build command dependency injection (flutter/flutter#114383)

* 69a542d Apply multidex config in kotlin dsl gradle file (flutter/flutter#114660)

* 92f10ed match error statements without relying on volatile human-readable descriptions (flutter/flutter#114922)

* d3dcd7d Update `CircleAvatar` to support Material 3 (flutter/flutter#114812)
auto-submit bot pushed a commit to flutter/plugins that referenced this pull request Nov 9, 2022
@engine-flutter-autoroll
Roll Flutter from a1432a9 to d3dcd7d (8 revisions) (#6684)
5857920 
* a84e369 Revert "Allow Flutter golden file tests to be flaky (#114450)" (flutter/flutter#114902)

* 1725a26 Switch the way we retrieve the vm_service_port from /hub to iquery, on device. (flutter/flutter#114834)

* 7020f59 [tool] Support `--flavor` option for `flutter install`. (flutter/flutter#114048)

* 9797d5f [macOS] Refactor the `flutter run` macOS console output test (flutter/flutter#114645)

* 530324d Build command dependency injection (flutter/flutter#114383)

* 69a542d Apply multidex config in kotlin dsl gradle file (flutter/flutter#114660)

* 92f10ed match error statements without relying on volatile human-readable descriptions (flutter/flutter#114922)

* d3dcd7d Update `CircleAvatar` to support Material 3 (flutter/flutter#114812)
IVLIVS-III pushed a commit to IVLIVS-III/flutter_plugins_fork that referenced this pull request Nov 11, 2022
@engine-flutter-autoroll @IVLIVS-III
Roll Flutter from a1432a9 to d3dcd7d (8 revisions) (flutter#6684)
3fd0f14 
* a84e369 Revert "Allow Flutter golden file tests to be flaky (#114450)" (flutter/flutter#114902)

* 1725a26 Switch the way we retrieve the vm_service_port from /hub to iquery, on device. (flutter/flutter#114834)

* 7020f59 [tool] Support `--flavor` option for `flutter install`. (flutter/flutter#114048)

* 9797d5f [macOS] Refactor the `flutter run` macOS console output test (flutter/flutter#114645)

* 530324d Build command dependency injection (flutter/flutter#114383)

* 69a542d Apply multidex config in kotlin dsl gradle file (flutter/flutter#114660)

* 92f10ed match error statements without relying on volatile human-readable descriptions (flutter/flutter#114922)

* d3dcd7d Update `CircleAvatar` to support Material 3 (flutter/flutter#114812)
adam-harwood pushed a commit to adam-harwood/flutter_plugins that referenced this pull request Nov 21, 2022
@engine-flutter-autoroll
Roll Flutter from a1432a9 to d3dcd7d (8 revisions) (flutter#6684)
36727de 
* a84e369 Revert "Allow Flutter golden file tests to be flaky (#114450)" (flutter/flutter#114902)

* 1725a26 Switch the way we retrieve the vm_service_port from /hub to iquery, on device. (flutter/flutter#114834)

* 7020f59 [tool] Support `--flavor` option for `flutter install`. (flutter/flutter#114048)

* 9797d5f [macOS] Refactor the `flutter run` macOS console output test (flutter/flutter#114645)

* 530324d Build command dependency injection (flutter/flutter#114383)

* 69a542d Apply multidex config in kotlin dsl gradle file (flutter/flutter#114660)

* 92f10ed match error statements without relying on volatile human-readable descriptions (flutter/flutter#114922)

* d3dcd7d Update `CircleAvatar` to support Material 3 (flutter/flutter#114812)
shogohida pushed a commit to shogohida/flutter that referenced this pull request Dec 7, 2022
@Jasguerrero @shogohida
Build command dependency injection (flutter#114383)
287b69e 
* update flutter build command

* update tests

* fix analyze suggestions
gspencergoog pushed a commit to gspencergoog/flutter that referenced this pull request Jan 19, 2023
@Jasguerrero @gspencergoog
Build command dependency injection (flutter#114383)
70fd116 
* update flutter build command

* update tests

* fix analyze suggestions
mauricioluz pushed a commit to mauricioluz/plugins that referenced this pull request Jan 26, 2023
@engine-flutter-autoroll @mauricioluz
Roll Flutter from a1432a9 to d3dcd7d (8 revisions) (flutter#6684)
940ee33 
* a84e369 Revert "Allow Flutter golden file tests to be flaky (#114450)" (flutter/flutter#114902)

* 1725a26 Switch the way we retrieve the vm_service_port from /hub to iquery, on device. (flutter/flutter#114834)

* 7020f59 [tool] Support `--flavor` option for `flutter install`. (flutter/flutter#114048)

* 9797d5f [macOS] Refactor the `flutter run` macOS console output test (flutter/flutter#114645)

* 530324d Build command dependency injection (flutter/flutter#114383)

* 69a542d Apply multidex config in kotlin dsl gradle file (flutter/flutter#114660)

* 92f10ed match error statements without relying on volatile human-readable descriptions (flutter/flutter#114922)

* d3dcd7d Update `CircleAvatar` to support Material 3 (flutter/flutter#114812)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
1 more reviewer

@christopherfujino christopherfujino christopherfujino approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
autosubmit Merge PR when tree becomes green via auto submit App tool Affects the "flutter" command-line tool. See also t: labels.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@Jasguerrero @christopherfujino