[flutter_tools] fix build for projects with watchOS companion app #51126
Conversation
fluttergithubbot
added
the
tool
|
It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie. Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
|
We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google. ℹ️ Googlers: Go here for more info. |
|
@googlebot I fixed it. |
|
CLAs look good, thanks! ℹ️ Googlers: Go here for more info. |
|
FYI @jmagman |
|
|
Regarding 2. : flutter/packages/flutter_tools/lib/src/ios/mac.dart Lines 209 to 213 in 3cee8e0 I tried using the SDKROOT setting, but couldn't get rid of the -sdk parameter this way.
In addition just dropping the |
Codecov Report
@@ Coverage Diff @@
## master #51126 +/- ##
==========================================
- Coverage 69.52% 65.58% -3.95%
==========================================
Files 204 208 +4
Lines 22737 22201 -536
==========================================
- Hits 15809 14560 -1249
- Misses 6928 7641 +713
Continue to review full report at Codecov.
|
Sorry for being unclear, I meant could we get rid of Requiring a destination for apps with companions seems like a good compromise, so thanks for this solution! A few concerns:
An "easier" way to test this may be to add a new final Directory watchApp = Directory(path.join(tempDir.path, 'app_with_watch'));
mkdir(watchApp);
recursiveCopy(
Directory(path.join(flutterDirectory.path, 'dev', 'integration_tests', 'ios_app_with_watch_companion')),
watchApp,
);The rub here is that your change requires a real destination. So you'd have to add simctl code to your dart test as setup (this needs to be exec'd in dart, but I just tested this in bash as an example)
await inDirectory(watchApp, () async {
await flutter(
'build',
options: <String>[
'ios',
'--no-codesign',
'-d',
'2360C20E-86EC-454C-8BF2-9053AD6AAE93',
],
);
});Then needs to be cleaned up in a To be tested like: Then added to Line 939 in 3411129 if (Platform.isMacOS) () => _runDevicelabTest('app_with_watch_companion'),(And you can see why I haven't gotten around to fixing this yet 😄).
This is very concerning, and I saw this in my own tests, too I'm not sure what other issues this will cause. |
There was a problem hiding this comment.
Don't set a default, we need to handle null being passed in, anyway.
There was a problem hiding this comment.
ok, default has been removed
There was a problem hiding this comment.
- Remove space between
!andbuildForDevice. - Handle null
deviceID
There was a problem hiding this comment.
Ended up here looking for reasons why xcodebuild was throwing errors. Turn's out this comment was the key! Do you happen to have any resources describing why -sdk causes issues with projects with WatchKit dependencies?
In the end, bloody amazing comment!
There was a problem hiding this comment.
"build a flutter a flutter app" -> "build an app with a companion watch"
Can you mention that the ID should be passed in with the --devices flag?
There was a problem hiding this comment.
You need to check that this file actually exists. INFOPLIST_FILE can be moved.
There was a problem hiding this comment.
Ok, a check has been added.
|
Oh, another thing: |
|
Hi! However, I've tried using it to run the Flutter app on a physical device, and that failed: Flutter doctor output and version: Perhaps we could address this in some further issue? And given that we need to implement and test some interaction between the apps, another problem appears. |
|
@Henrik-glt yes, of course! I plan to implement it this / next week :-) Sorry for the delay, but recent world wide events kinda forced me to work on more immediate projects. :-( |
Great! Looking forward to getting this important fix to master. 😃 Great work mate, completely understand that people need to prioritise during this weird time. |
zanderso
added
work in progress; do not review
waiting for customer response
This comment has been minimized.
This comment has been minimized.
|
@jmagman Sorry that it took so long - hope the overall situation gets less stressful soon.
Build for a simulated device seems to use a different codepath than running on a simulated device, as
That is definately a good idea. Can confirm that in my test, the companion app inherited the One more thing: As running the devicelab tests with other locales than en_US fails (likely en_* also works, not tested), would it be possible to add the following text to https://github.com/flutter/flutter/wiki/Running-and-writing-tests in the section "Running device lab tests locally" between items 2. and 3.:
|
|
@tauu Thank you so much for working on this, I can tell you put in a lot of time and thought! I'll look at it in detail today!
Great idea, done. Also created an issue for the tests to do this for you. |
jmagman
left a comment
jmagman
left a comment
There was a problem hiding this comment.
- You can delete
dev/integration_tests/ios_app_with_watch_companion/android/anddev/integration_tests/ios_app_with_watch_companion/web/anddev/integration_tests/ios_app_with_watch_companion/testsince they aren't needed for the test. That will also make thedev/integration_tests/ios_app_with_watch_companion/android/app/src/main/res/values/styles.xml:13: trailing U+0020 space characteranalyzer issue go away. - I checked out this change and it cleanly rebases onto top of tree master, but unfortunately it doesn't build on top of #51444:
packages/flutter_tools/lib/src/project.dart:482:48: Error: Getter not found: 'instance'.
if (infoFile.existsSync() && PlistParser.instance.getValueFromFile(infoFile.path, 'WKCompanionAppBundleIdentifier') == bundleIdentifier) {
You can replace PlistParser.instance with globals.plistParser (we're migrating to be explicit about "global" state so we can start removing it to inject dependencies more explicitly, see #47161)
I'm also hitting a linker error when I flutter build ios in the new integration test directory. I'll investigate that more on Monday.
Thank you so much for this, it's awesome!
There was a problem hiding this comment.
You can leave off instantiating these and check if they are null in the finally.
String watchDeviceID;
String phoneDeviceID;There was a problem hiding this comment.
ok, that sounds better, done.
There was a problem hiding this comment.
I think you're missing a brace here.
There was a problem hiding this comment.
Indeed I missed a brace, fixed.
There was a problem hiding this comment.
I took the liberty of writing some unit tests for this since I know this file is really gross to test. You can put them in project_test.dart or I'd be happy to push it to your fork, if you prefer.
group('watch companion', () {
MemoryFileSystem fs;
MockPlistUtils mockPlistUtils;
MockXcodeProjectInterpreter mockXcodeProjectInterpreter;
FlutterProjectFactory flutterProjectFactory;
setUp(() {
fs = MemoryFileSystem.test();
mockPlistUtils = MockPlistUtils();
mockXcodeProjectInterpreter = MockXcodeProjectInterpreter();
flutterProjectFactory = FlutterProjectFactory();
});
testUsingContext('cannot find bundle identifier', () async {
final FlutterProject project = await someProject();
expect(await project.ios.containsWatchCompanion(<String>['WatchTarget']), isFalse);
}, overrides: <Type, Generator>{
FileSystem: () => fs,
ProcessManager: () => FakeProcessManager.any(),
PlistParser: () => mockPlistUtils,
XcodeProjectInterpreter: () => mockXcodeProjectInterpreter,
FlutterProjectFactory: () => flutterProjectFactory,
});
group('with bundle identifier', () {
setUp(() {
when(mockXcodeProjectInterpreter.getBuildSettings(any, any)).thenAnswer(
(_) {
return Future<Map<String,String>>.value(<String, String>{
'PRODUCT_BUNDLE_IDENTIFIER': 'io.flutter.someProject',
});
}
);
});
testUsingContext('no Info.plist in target', () async {
final FlutterProject project = await someProject();
expect(await project.ios.containsWatchCompanion(<String>['WatchTarget']), isFalse);
}, overrides: <Type, Generator>{
FileSystem: () => fs,
ProcessManager: () => FakeProcessManager.any(),
PlistParser: () => mockPlistUtils,
XcodeProjectInterpreter: () => mockXcodeProjectInterpreter,
FlutterProjectFactory: () => flutterProjectFactory,
});
testUsingContext('Info.plist in target does not contain WKCompanionAppBundleIdentifier', () async {
final FlutterProject project = await someProject();
project.ios.hostAppRoot.childDirectory('WatchTarget').childFile('Info.plist').createSync(recursive: true);
expect(await project.ios.containsWatchCompanion(<String>['WatchTarget']), isFalse);
}, overrides: <Type, Generator>{
FileSystem: () => fs,
ProcessManager: () => FakeProcessManager.any(),
PlistParser: () => mockPlistUtils,
XcodeProjectInterpreter: () => mockXcodeProjectInterpreter,
FlutterProjectFactory: () => flutterProjectFactory,
});
testUsingContext('target WKCompanionAppBundleIdentifier is not project bundle identifier', () async {
final FlutterProject project = await someProject();
project.ios.hostAppRoot.childDirectory('WatchTarget').childFile('Info.plist').createSync(recursive: true);
when(mockPlistUtils.getValueFromFile(any, 'WKCompanionAppBundleIdentifier')).thenReturn('io.flutter.someOTHERproject');
expect(await project.ios.containsWatchCompanion(<String>['WatchTarget']), isFalse);
}, overrides: <Type, Generator>{
FileSystem: () => fs,
ProcessManager: () => FakeProcessManager.any(),
PlistParser: () => mockPlistUtils,
XcodeProjectInterpreter: () => mockXcodeProjectInterpreter,
FlutterProjectFactory: () => flutterProjectFactory,
});
testUsingContext('has watch companion', () async {
final FlutterProject project = await someProject();
project.ios.hostAppRoot.childDirectory('WatchTarget').childFile('Info.plist').createSync(recursive: true);
when(mockPlistUtils.getValueFromFile(any, 'WKCompanionAppBundleIdentifier')).thenReturn('io.flutter.someProject');
expect(await project.ios.containsWatchCompanion(<String>['WatchTarget']), isTrue);
}, overrides: <Type, Generator>{
FileSystem: () => fs,
ProcessManager: () => FakeProcessManager.any(),
PlistParser: () => mockPlistUtils,
XcodeProjectInterpreter: () => mockXcodeProjectInterpreter,
FlutterProjectFactory: () => flutterProjectFactory,
});
});
});There was a problem hiding this comment.
Thanks a lot! Totally agree that tests for containsWatchCompanion should be included (... and that writing tests for the plist file is not exactly fun ;-) )
There was a problem hiding this comment.
(... and that writing tests for the plist file is not exactly fun ;-) )
Ugh I know, tell me about it. 😝
We're working on making tool tests easier to write, though mocking out a plist parser may never be anyone's favorite pastime.
There was a problem hiding this comment.
If you rebase onto master and run flutter build ios in this directory, it should automatically migrate this project to fix #50568. See https://flutter.dev/docs/development/ios-project-migration also for details.
There was a problem hiding this comment.
Again thanks for the information, done.
|
Thanks for merging #54787 so fast! :-) |
jmagman
removed
the
waiting for customer response
jmagman
left a comment
jmagman
left a comment
There was a problem hiding this comment.
LGTM, thanks again for doing this!
|
No problem, always a pleasure submitting PRs to the flutter project. Thank you for all the detailed feedback. |
|
@tauu Are you able to run using flutter run when your iOS app has a watch extension? When I use legacy build system I get the following error: This seems to be caused flutter specifying architecture arm64, but that is not a valid architecture for the watch extension. When I use the "new" build system I get an error saying that signing doesn't match parent app. Running flutter build ios works now however (great work! :) ) Am I missing something obvious? Errors produced by switching to master (flutter channel master), run flutter upgrade and then flutter run |
|
@Henrik-glt Can confirm that using
Sorry apparently I missed checking it this way. Running the app on a real device from XCode works though. So likely the build process employed by flutter run differs in some way from what XCode does, will investigate it further. |
|
@Henrik-glt: Found the issue and proposed a fix for it #54959. |
|
@tauu Works like a charm! Great work and many thanks for fixing this! |
|
Hello, Does anybody have a workaround in case WKCompanionAppBundleIdentifier is not the app bundle itself but a build variable like ${APP_BUNDLE}? We have different bundle identifiers in our app so we need to have it in this way. |
I have same issue. I use flavor like com.foo.bar and com.foo.bar.dev |
I reolved using hacky solution. Despite of additional fix for substituting variable (Skogsfrae@584801d), I failed substituting "com.foo.bar${BUNDLE_ID_SUFFIX}" However "$(PRODUCT_BUNDLE_IDENTIFIER)" success So I added the following to ../Runner/Info.plist $(PRODUCT_BUNDLE_IDENTIFIER) is what value I want (e.g., "com.foo.bar.dev", etc) only where in Runner target. |
…144607) flutter/engine@d514a30...17a4b66 2024-03-05 [email protected] Roll Skia from a577399ed6fb to 5839a94bf28b (1 revision) (flutter/engine#51194) 2024-03-05 [email protected] [Impeller] Turn off StC. (flutter/engine#51191) 2024-03-05 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Reland: [macOS] Use CVDisplayLink to drive repaint (#51126)" (flutter/engine#51192) 2024-03-05 [email protected] Roll Skia from 9c62e7b382cf to a577399ed6fb (1 revision) (flutter/engine#51190) 2024-03-05 [email protected] Reland "Remove migration flag and unused header files #50216" (flutter/engine#50259) 2024-03-05 [email protected] Shift git version fetching to tools/gn (flutter/engine#51175) 2024-03-05 [email protected] [fuchsia] Remove now unnecessary diagnostics directory (flutter/engine#51180) 2024-03-05 [email protected] [Impeller] Enable depth buffer clipping & Stencil-then-Cover path rendering. (flutter/engine#50856) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC [email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
12 participants
Description
A watchOS companion app can be added to a flutter app as described in the official documentation. But after doing
flutter buildandflutter runwill no longer work. The root cause of this is that flutter will always pass either '-sdk iphoneos' or '-sdk iphonesimulator' to xcodebuild for iOS builds. Neither of these may be used to build an app with a watchOS companion app. (see e.g. Stackoverflow )The changes in this pull request adjust the build settings if a watchOS companion app is detected in an iOS project and fix both
flutter runandflutter buildcommands for iOS projects with a watchOS companion app.Related Issues
This might be relevant for #28901.
Tests
I added the following tests:
Checklist
Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes (
[x]). This will ensure a smooth and quick review process.///).flutter analyze --flutter-repo) does not report any problems on my PR.Breaking Change
Did any tests fail when you ran them? Please read [Handling breaking changes].