in `flutter run`, throw tool exit when `--flavor` is provided but is not supported on the target device #139045

andrewkolos · 2023-11-27T07:23:56Z

Fixes #134197

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.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

packages/flutter_tools/lib/src/commands/run.dart

andrewkolos · 2023-12-07T01:52:04Z

fyi @christopherfujino, this is now ready for review again (assuming my last commit doesn't break test)

jmagman · 2023-12-07T16:43:44Z

packages/flutter_tools/lib/src/commands/run.dart

Was macOS flavor support removed while I was gone?

I previously assumed macOS was supported, but I don't think this is the case?

macOS support is not documented. I also think flutter build macos --flavor results in a ToolExit saying Could not find an option named "flavor".

Does it work with flutter run -d macos? If so, fixing flutter build macos might be a one line fix?

Then we should document flavors support for macOS (assuming it works). IIRC the setup process in Xcode would be similar but not identical. I'll do more research.

Looks like we deleted the test in april. Not sure what we should do moving forward... #125581

If

macOS support is not documented,

macOS support is not under test, and most importantly

users can't even build with flavors on macOS (unless they are publishing builds produced by flutter run),

then it's clear to me that macOS is not supported. If support is desired, then it should go through the normal feature request process, no?

#119564
#64088
cc @cbracken

Chatted with @christopherfujino. I still think it's fair for this PR to merge as is, but it's arguably worth seeing if macOS flavor support can be easily fixed before making it officially unsupported.

I will see what the current state of macOS flavor support is (i.e. does flutter run --flavor even work properly for macOS?). Then, I'll check in with Hans and see if they have any opinions on the matter.

Important correction: flutter build macos --flavor does work. I was accidentally doing flutter run -d macos --flavor instead. Still, the other two points are valid, so I will file an issue and get Hans' input.

I had some offline conversations about this, so I'm adding an update here for posterity's sake. I'm allotting myself some time to investigate #139774. If getting flavors for macOS back under test proves simple enough, I'll do that and get support for it publicly documented.

…oes-not-support-flavor

christopherfujino · 2024-01-03T19:35:27Z

packages/flutter_tools/test/commands.shard/hermetic/run_test.dart

+        // Useful for test readability.
+        // ignore: avoid_redundant_argument_values
+        final FakeDevice device = FakeDevice(supportsFlavors: false);
+        testDeviceManager.devices = <Device>[device];


can we have two devices, one of which IS supported, and another that is NOT supported, and then pass the --all flag? I'm guessing in that case we should throw tool exit, right?

christopherfujino · 2024-01-03T19:35:36Z

packages/flutter_tools/lib/src/commands/run.dart

+    final bool flavorsSupportedOnEveryDevice = devices!
+      .every((Device device) => device.supportsFlavors);
+    if (flavor != null && !flavorsSupportedOnEveryDevice) {
+      throwToolExit('--flavor is only supported for Android, macOS, and iOS devices.');


A few thoughts:

The API here of List<Device>? devices here is unfortunate, but I won't ask you to refactor this in this PR. I'm guessing late final Future<List<Devices>> devices = findAllTargetDevices(); would probably be more readable (although it's unfortunate that findAllTargetDevices() has hidden dependencies, but alas)

I'm guessing that here devices will be only devices that: are available AND we have determined we will be running the app on, right? In which case, it seems like this validation logic is correct.

Do we also need validation logic for flutter test

2. I'm guessing that here devices will be only devices that: are available AND we have determined we will be running the app on, right? In which case, it seems like this validation logic is correct.

This was my understanding from the last time I read the code. It also makes sense with the name of findAllTargetDevices.

3. Do we also need validation logic for flutter test

This seems like a good idea. I first need to figure out how device selection works for flutter test.

I've added the validation for flutter test. It is unclear to me whether or not this validation should itself be under test. Given its simplicity and that most of the neighboring throwToolExits also do not seem to be under test, I've elected to not create an integration test for this (perhaps I'm succumbing to broken window theory here). I did manually test this.

jmagman

The iOS and macOS parts LGTM, I'll let @christopherfujino finish his review.

Thank you so much for tracking down the macOS flavor issues. Just today I was able to implement something in packages tooling for iOS and macOS and it's so much nicer when the tool works the same for both!

christopherfujino

lgtm

… but is not supported on the target device (flutter/flutter#139045)

mauriziopinotti · 2024-02-17T14:53:51Z

What's the sense of this change? Now developers have to duplicate all run configurations, one for Web (without flavor option) and one for Mobile (with flavor option), causing massive inconvenience for no gain!

andrewkolos · 2024-02-20T02:24:50Z

What's the sense of this change? Now developers have to duplicate all run configurations, one for Web (without flavor option) and one for Mobile (with flavor option), causing massive inconvenience for no gain!

When questioning the purpose of a PR, I recommend first reading both the PR and the issue associated with it. One or both of these should include the reason for the change. In this case, the original issue, #134197, includes the reasoning.

Regardless, a new issue has been opened to discuss this further, #143574.

josehigroup · 2024-02-20T17:04:28Z

It would be excellent if we could continue using the flavor flag without breaking the build. that way we can continue using our run configuration for mobile and web.

mauriziopinotti · 2024-02-21T10:06:03Z

What's the sense of this change? Now developers have to duplicate all run configurations, one for Web (without flavor option) and one for Mobile (with flavor option), causing massive inconvenience for no gain!

When questioning the purpose of a PR, I recommend first reading both the PR and the issue associated with it. One or both of these should include the reason for the change. In this case, the original issue, #134197, includes the reasoning.

Regardless, a new issue has been opened to discuss this further, #143574.

It's a rhetorical question: I did read the PR and I just think the solution is causing more problems than it's supposed to fix.

andrewkolos added the tool Affects the "flutter" command-line tool. See also t: labels. label Nov 27, 2023

andrewkolos requested a review from christopherfujino November 27, 2023 19:01

navaronbracke mentioned this pull request Nov 27, 2023

[Proposal] Conditional bundling of assets #96514

Closed

andrewkolos force-pushed the throw-if-device-does-not-support-flavor branch from 210b8cb to 5ee78c6 Compare November 29, 2023 23:33

jmagman reviewed Nov 30, 2023

View reviewed changes

packages/flutter_tools/lib/src/commands/run.dart Outdated Show resolved Hide resolved

andrewkolos force-pushed the throw-if-device-does-not-support-flavor branch from 5ee78c6 to 84e55e1 Compare December 6, 2023 23:55

github-actions bot added the platform-ios iOS applications specifically label Dec 7, 2023

jmagman reviewed Dec 7, 2023

View reviewed changes

andrewkolos requested a review from jmagman December 7, 2023 19:13

andrewkolos mentioned this pull request Dec 7, 2023

It is unclear if the Flavors feature is meant to be supported for macOS #139774

Closed

andrewkolos force-pushed the throw-if-device-does-not-support-flavor branch from f346d48 to 4565874 Compare December 18, 2023 06:12

andrewkolos added 9 commits January 2, 2024 17:46

fix

82dfc55

fix some tests

1d56968

remove macOS support, move declaration of flavor support to devices

affd018

update exit text to reflect macOS not being supported

27d23fb

oops

4cf1b76

fix more tests

ddeb70b

fix variable name

56827c3

simplify

d70b5f3

add macOS support

6188b2e

andrewkolos force-pushed the throw-if-device-does-not-support-flavor branch from 4565874 to 6188b2e Compare January 3, 2024 01:47

github-actions bot added the a: desktop Running on desktop label Jan 3, 2024

andrewkolos added 2 commits January 2, 2024 19:05

doh

9e6921e

Merge remote-tracking branch 'upstream/master' into throw-if-device-d…

28c7f30

…oes-not-support-flavor

christopherfujino reviewed Jan 3, 2024

View reviewed changes

andrewkolos added 3 commits January 3, 2024 12:34

include device with flavor support in test

415fcb7

add flavors support to more devices

3f7c3f1

check device flavor support for flutter test

90b3378

fix test

353ab9f

gspencergoog requested a review from cbracken January 4, 2024 19:14

jmagman approved these changes Jan 5, 2024

View reviewed changes

andrewkolos requested a review from christopherfujino January 5, 2024 21:00

christopherfujino approved these changes Jan 5, 2024

View reviewed changes

andrewkolos added the autosubmit Merge PR when tree becomes green via auto submit App label Jan 5, 2024

auto-submit bot merged commit e90e488 into flutter:master Jan 5, 2024

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 16, 2024

e90e488 in flutter run, throw tool exit when --flavor is provided…

b89034a

… but is not supported on the target device (flutter/flutter#139045)

huycozy mentioned this pull request Feb 19, 2024

flutter web not allow --flavor while launching since Flutter 3.19.0 #143574

Closed

andrewkolos deleted the throw-if-device-does-not-support-flavor branch April 29, 2024 18:18

in flutter run, throw tool exit when --flavor is provided but is not supported on the target device #139045

in flutter run, throw tool exit when --flavor is provided but is not supported on the target device #139045

Uh oh!

Conversation

andrewkolos commented Nov 27, 2023

Pre-launch Checklist

Uh oh!

Uh oh!

andrewkolos commented Dec 7, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmagman left a comment

Choose a reason for hiding this comment

Uh oh!

christopherfujino left a comment

Choose a reason for hiding this comment

Uh oh!

mauriziopinotti commented Feb 17, 2024

Uh oh!

andrewkolos commented Feb 20, 2024

Uh oh!

josehigroup commented Feb 20, 2024

Uh oh!

mauriziopinotti commented Feb 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

in `flutter run`, throw tool exit when `--flavor` is provided but is not supported on the target device #139045

in `flutter run`, throw tool exit when `--flavor` is provided but is not supported on the target device #139045

mauriziopinotti commented Feb 21, 2024 •

edited

Loading