[tool] Building flavors with native tooling #157871

Sameri11 · 2024-10-30T17:22:21Z

fixes #155951

This PR enables building Flutter apps with flavors using native build tooling (Xcode/Gradle).

Tool part

Implementation is fully based on #155951 (comment) (thanks, @andrewkolos !)

Android part

Android has native support of flavors on the build system level, so no need to any modifications in gradle part.

It's added to assemble here:

flutter/packages/flutter_tools/gradle/src/main/groovy/flutter.groovy

Lines 1762 to 1764 in c9923ca

    
           if (flavor != null) { 
        
               args("-dFlavor=${flavor}") 
        
           }

and parsed from current running variant, meaning that running ./gradlew app:assemblePaidDebug will choose Paid flavor.

iOS part

There is no existing way to parse flavor from Xcode configuration (as far as I know). And FLAVOR is never passed to build if no additional actions performed. My suggestion here is to support 2 ways:

Add FLAVOR variable to xcconfig file (should be done by user). By doing that, FLAVOR variable will be passed to build and parsed in xcode_backend.dart (or macOS_assemble.sh). This way was supported before, obviously.
Parse flavor name from configuration variable. I expect configuration to be named as $CONFIGURATION-$FLAVOR as suggested in Creating flavors in iOS and macOS documentation.

Having these 2 methods, parsing logic is become similar to one used for parsing buildMode here:

flutter/packages/flutter_tools/bin/xcode_backend.dart

Line 135 in c9923ca

String parseFlutterBuildMode() {

Testing strategy

I do not know any other method than test this in integration tests:

Build with native tooling
Run flavor test with flutter drive --use-application-binary

Hence tests added only for iOS and Android since --use-application-binary is not supported for another platforms (as per documentation for flutter drive).

⚠️ WARNING ⚠️: .ci.yaml has been modified to enable new tests in presubmit. Then they become green, latest commit in the branch DO_NOT_MERGE! try run tests should be deleted. In any case, this commit must be deleted before merging

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.
I followed the breaking change policy and added Data Driven Fixes where supported.
All existing and new tests are passing.

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

Sameri11 · 2025-01-14T16:09:58Z

Mac_ios flavors_test_ios test is broken now due to #161402. In the meantime, I suggest to discuss implementation in general.

andrewkolos · 2025-01-30T21:26:33Z

I think it's okay to not test this on presubmit CI. In the worst case scenario, flavors_test and flavors_test_ios will fail in postsubmit, and this PR will get reverted, which isn't a huge deal. When I get back to my desk, I can try running these locally to get some confidence that these will still work. After that, I can revert the ci.yaml changes and get this merged 🙂

andrewkolos

I think the new _testFlavorsWhenBuildStartsWithXcode doesn't work. I tested this on a physical iphone by running

dart bin/test_runner.dart test -t flavors_test_ios --exit

within the devicelab directory.

I think the xcodebuild command might be incorrect (for starters, I think '-scheme paid' needs to be '-scheme', '-paid').

Logs: https://gist.github.com/andrewkolos/2786de8d5fd521ea697b151b3fb54906

Perhaps the xcodebuild calls in dev/devicelab/bin/tasks/module_test_ios.dart could be used for inspiration? Unfortunately, I don't think I have the time to debug this myself.

jmagman · 2025-01-31T00:10:17Z

packages/flutter_tools/bin/xcode_backend.dart

+    if (flavor != null && flavor.isNotEmpty) {
+      return flavor;
+    }
+    if (environment['CONFIGURATION'] != null) {


I don't think we should duplicate the existing (more complicated) flavor parsing logic that's already in the tool (this is a stand-alone script). For example this doesn't handle the difference in casing between configurations and flavors #161455

In #155951 I was trying to say that the logic in the assemble build target should reuse the same logic as passed into --flavor. I don't want anything extra to be set by the developer in a build configuration, and I don't want it to be possible for setting an extra build setting to get out of sync with the parsed schemes and build configurations.

I agree that this is the most fragile part of PR. I just want to clarify the intention here: the idea was to provide some default for the case when a developer didn't provide a custom configuration through which FLAVOR can be obtained inside an Xcode-started build. By custom configuration, I mean .xcconfig or any other method that will "inject" this variable.

Imagine the situation:

A developer adds flavors as described in [documentation](https://docs.flutter.dev/deployment/flavors#creating-flavors-in-ios-and-macos).

Builds the app with flutter build ... – everything works.

Builds the app with Xcode – everything works too, but app_flavor at runtime is still null.

I thought this flow was kind of confusing when I implemented this PR and decided to add some default in such a way that it will work for the default case described above. For developers who have already created a custom way to provide FLAVOR from Xcode itself, this shouldn't break anything since there's an early return if FLAVOR is already set. This also won't break flutter build ... because the FLAVOR environment variable would be set, and everything will stay in sync.

Now, again, it's fragile, and I won't fight for this change, obviously :) Those changes to the xcode_backend scripts can be reverted, and everything will work, with one exception: it will be the developer's responsibility to provide FLAVOR for the build when it's invoked with Xcode.

So yeah, I need an confirmation (after this explanation) that this change is still undesired.

jmagman · 2025-01-31T00:30:26Z

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


    final Map<String, Object?> defineConfigJsonMap = extractDartDefineConfigJsonMap();
    final List<String> dartDefines = extractDartDefines(defineConfigJsonMap: defineConfigJsonMap);
+    if (results.containsKey(kFlavor) && results[kFlavor]!.isNotEmpty) {


I think this should be done instead in the ios and macos assemble target steps based on the same logic used to parse configuration and schemes that already exist in xcodeproj, and then from there sent it into the GenSnapshot (if I'm remembering how this works).

should be done instead in the ios and macos assemble target steps

Could you please elaborate on the advantages of this move? I am obviously not an expert here, so I can't see any for now :( But if it is done, I believe there will be code duplication since it will require moving Android's implementation somewhere. Doing the same thing in 2-3 places when it can be done in a single location (as in the current PR state) – I believe that was the main motivation to implement parsing here.

Or maybe I completely misunderstood your comment?

Sameri11 · 2025-01-31T05:28:29Z

@andrewkolos Thanks for your effort and time you spend dealing with new tests. It didn't occur to me that I can test it locally – will try and fix those.

Sameri11 · 2025-01-31T05:33:12Z

@bkonyi changes to .ci.yaml are subject to being reverted/reset – the idea was to make tests green, check them in presubmit, and then revert those changes before merging.

Co-authored-by: Andrew Kolos <[email protected]>

Sameri11 · 2025-02-04T06:25:44Z

dev/integration_tests/flavors/ios/Runner.xcodeproj/project.pbxproj

+				LD_RUNPATH_SEARCH_PATHS = (
+					"$(inherited)",
+					"@executable_path/Frameworks",
+				);


Seems like these changes have been formatted by Xcode when I opened project in it. Should it be reverted or it's ok ?

Sameri11 · 2025-02-04T06:26:43Z

flavors_test_ios should work now. At least, it worked locally in my environment :)

vashworth · 2025-02-11T16:38:50Z

@Sameri11 Thanks for working on this issue. I agree with your approach of attempting to parse the flavor/scheme from the configuration, however, I also agree with @jmagman that it should be moved into the assemble targets. Since this would require many changes and is difficult to explain, I went ahead and put together a PR to handle the iOS/macOS/Xcode side of things (#162907).

I will defer to @andrewkolos and @bkonyi on the Android side of things

bkonyi · 2025-02-11T16:50:48Z

@reidbaker do you have any thoughts on this WRT the Android side of things?

Sameri11 · 2025-02-11T17:30:54Z

@vashworth thanks for letting me know.

reidbaker · 2025-02-12T16:01:06Z

@Sameri11 did you still want review on the android side?

Sameri11 · 2025-02-12T17:28:02Z

@reidbaker Not in this PR, no. Current implementation is incompatible with #162907 so I guess it is must be completely new PR after #162907 successfully merged. And maybe even another issue, since #155951 is initially is about Xcode only.

…#163737)  This request is a subset of #157871 and follows up #162907. #162907 actually fixed behaviour of building with native tooling not only for iOS/macOS, but also for Android builds too: now on master, if you start build with `gradle` instead of `flutter build ...` – you also get the correct behaviour and `appFlavor` won't be null in runtime. Tests in this PR will check exactly this behavior. Another diff is the change in the method of obtaining flavor at runtime inside test project. Before this change, method channels were used for this, after – `appFlavor` constant. Both methods do more or less the same thing right now, but they may diverge in future, so I guess this is the right way to check correctness here. Also, this change was requested and approved by Andrew in #157871 (comment) issue for checklist: #155951 New test was tested here: https://ci.chromium.org/ui/p/flutter/builders/try/Linux_pixel_7pro%20flavors_test/63/overview ## Pre-launch Checklist - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [x] I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement]. - [x] I signed the [CLA]. - [x] I listed at least one issue that this PR fixes in the description above. - [x] I updated/added relevant documentation (doc comments with `///`). - [x] I added new tests to check the change I am making, or this PR is [test-exempt]. - [x] I followed the [breaking change policy] and added [Data Driven Fixes] where supported. - [x] All existing and new tests are passing. If you need help, consider asking for advice on the #hackers-new channel on [Discord].  [Contributor Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#overview [Tree Hygiene]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md [test-exempt]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#tests [Flutter Style Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md [Features we expect every widget to implement]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md#features-we-expect-every-widget-to-implement [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [breaking change policy]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/blob/main/docs/contributing/Chat.md [Data Driven Fixes]: https://github.com/flutter/flutter/blob/main/docs/contributing/Data-driven-Fixes.md

github-actions bot added the tool Affects the "flutter" command-line tool. See also t: labels. label Oct 30, 2024

Sameri11 force-pushed the app-flavors-native-tooling branch 2 times, most recently from 182d313 to afd40e8 Compare October 31, 2024 18:20

Sameri11 force-pushed the app-flavors-native-tooling branch 21 times, most recently from 88edece to fe0c6a3 Compare January 14, 2025 16:06

Sameri11 marked this pull request as ready for review January 14, 2025 16:11

Sameri11 changed the title ~~[tool] Building flavours with native tooling~~ [tool] Building flavors with native tooling Jan 16, 2025

Sameri11 force-pushed the app-flavors-native-tooling branch 3 times, most recently from 2673458 to f5c1677 Compare January 17, 2025 16:35

andrewkolos requested a review from bkonyi January 30, 2025 21:41

andrewkolos requested changes Jan 30, 2025

View reviewed changes

jmagman requested changes Jan 31, 2025

View reviewed changes

jmagman requested a review from vashworth January 31, 2025 00:32

Sameri11 and others added 3 commits February 3, 2025 17:23

Build flavors with native tooling on android/ios/macos

dc608dd

Apply suggestions from code review

0abd7f9

Co-authored-by: Andrew Kolos <[email protected]>

Cleanup for flavor tests

ef9ee57

Sameri11 force-pushed the app-flavors-native-tooling branch from a215fe0 to 0d25ed2 Compare February 3, 2025 18:37

github-actions bot added platform-android Android applications specifically a: desktop Running on desktop labels Feb 3, 2025

Sameri11 force-pushed the app-flavors-native-tooling branch 2 times, most recently from 6d74885 to c72b49d Compare February 3, 2025 18:55

github-actions bot removed platform-android Android applications specifically a: desktop Running on desktop labels Feb 3, 2025

fix ios driver test

932a10a

Sameri11 force-pushed the app-flavors-native-tooling branch from c72b49d to 932a10a Compare February 4, 2025 06:23

Sameri11 commented Feb 4, 2025

View reviewed changes

Sameri11 requested a review from andrewkolos February 4, 2025 06:26

Sameri11 closed this Feb 11, 2025

Sameri11 mentioned this pull request Feb 20, 2025

Add integration test for Gradle-initiated android builds with flavors #163737

Merged

9 tasks

[tool] Building flavors with native tooling #157871

[tool] Building flavors with native tooling #157871

Uh oh!

Conversation

Sameri11 commented Oct 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Tool part

Android part

iOS part

Testing strategy

Pre-launch Checklist

Uh oh!

Sameri11 commented Jan 14, 2025

Uh oh!

andrewkolos commented Jan 30, 2025

Uh oh!

andrewkolos left a comment

Choose a reason for hiding this comment

Uh oh!

jmagman Jan 31, 2025

Choose a reason for hiding this comment

Uh oh!

Sameri11 Feb 3, 2025

Choose a reason for hiding this comment

Uh oh!

jmagman Jan 31, 2025

Choose a reason for hiding this comment

Uh oh!

Sameri11 Feb 3, 2025

Choose a reason for hiding this comment

Uh oh!

Sameri11 commented Jan 31, 2025

Uh oh!

Sameri11 commented Jan 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Sameri11 Feb 4, 2025

Choose a reason for hiding this comment

Uh oh!

Sameri11 commented Feb 4, 2025

Uh oh!

vashworth commented Feb 11, 2025

Uh oh!

bkonyi commented Feb 11, 2025

Uh oh!

Sameri11 commented Feb 11, 2025

Uh oh!

reidbaker commented Feb 12, 2025

Uh oh!

Sameri11 commented Feb 12, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Sameri11 commented Oct 30, 2024 •

edited

Loading

Sameri11 commented Jan 31, 2025 •

edited

Loading