Skip to content
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

Makes scheme and target optional parameter when getting universal lin… #134571

Merged
merged 4 commits into from
Sep 15, 2023
Merged

Makes scheme and target optional parameter when getting universal lin… #134571

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
9b7a1fd
Makes scheme and target optional parameter when getting universal lin…
chunhtai Sep 12, 2023
50c6a83
remove schemes from analyze API
chunhtai Sep 13, 2023
8c353e5
Apply suggestions from code review
chunhtai Sep 14, 2023
4264323
Merge branch 'master' into xcode-optional
chunhtai Sep 15, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
10 changes: 4 additions & 6 deletions packages/flutter_tools/lib/src/commands/analyze.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -152,7 +152,8 @@ class AnalyzeCommand extends FlutterCommand {
argParser.addFlag('output-universal-link-settings',
negatable: false,
help: 'Output a JSON with iOS Xcode universal link settings into a file. '
'The "--configuration", "--scheme", and "--target" must also be set.',
'The "--configuration", and one of "--scheme" or "--target" must also '
'be set.',
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: grammar

Suggested change
'The "--configuration", and one of "--scheme" or "--target" must also '
'be set.',
'The "--configuration" and either "--scheme" or "--target" must also '
'be set.',

Copy link
Contributor

Choose a reason for hiding this comment

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

On second thought, "one of" is fine too. Feel free to ignore.

hide: !verboseHelp,
);

Expand Down Expand Up @@ -280,12 +281,9 @@ class AnalyzeCommand extends FlutterCommand {
throwToolExit('"--configuration" must be provided');
}
target = argResults!['target'] as String?;
if (target == null) {
throwToolExit('"--target" must be provided');
}
scheme = argResults!['scheme'] as String?;
if (scheme == null) {
throwToolExit('"--scheme" must be provided');
if ((scheme == null) == (target == null)) {
throwToolExit('Only one of "--target" or "--scheme" must be provided but not both.');
}
} else {
throwToolExit('No argument is provided to analyze. Use -h to see available commands.');
Expand Down
7 changes: 4 additions & 3 deletions packages/flutter_tools/lib/src/commands/ios_analyze.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,8 @@ class IOSAnalyze {
this.target,
required this.logger,
}) : assert(option == IOSAnalyzeOption.listBuildOptions ||
(configuration != null && scheme != null && target != null));
(configuration != null &&
((scheme != null) != (target != null))));

final FlutterProject project;
final IOSAnalyzeOption option;
Expand All @@ -61,8 +62,8 @@ class IOSAnalyze {
}
logger.printStatus(jsonEncode(result));
case IOSAnalyzeOption.outputUniversalLinkSettings:
await project.ios.outputsUniversalLinkSettings(configuration: configuration!, scheme: scheme!, target: target!);
final String filePath = await project.ios.outputsUniversalLinkSettings(configuration: configuration!, scheme: scheme!, target: target!);
await project.ios.outputsUniversalLinkSettings(configuration: configuration!, scheme: scheme, target: target);
final String filePath = await project.ios.outputsUniversalLinkSettings(configuration: configuration!, scheme: scheme, target: target);
logger.printStatus('result saved in $filePath');
}
}
Expand Down
9 changes: 7 additions & 2 deletions packages/flutter_tools/lib/src/xcode_project.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.

import 'base/common.dart';
import 'base/error_handling_io.dart';
import 'base/file_system.dart';
import 'base/utils.dart';
Expand Down Expand Up @@ -221,9 +222,12 @@ class IosProject extends XcodeBasedProject {
/// The return future will resolve to string path to the output file.
Future<String> outputsUniversalLinkSettings({
required String configuration,
required String scheme,
required String target,
String? scheme,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually I am not sure if we should support scheme at all. My understanding of scheme is that it contains a set of targets. I am not sure what does it mean to get build settings for a scheme.

Copy link
Member

Choose a reason for hiding this comment

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

@andrewkolos can you review this PR? I'm still fuzzy on the relationship between targets and schemes.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hah, I hardly know the difference myself, but—no worries—I'll take a look.

Copy link
Contributor

Choose a reason for hiding this comment

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

As I understand, projects and targets can have Build Settings, while schemes can have a Build Configuration. Settings are applied in this order: project settings, build configuration settings, then target settings1.

Footnotes

  1. https://developer.apple.com/documentation/xcode/adding-a-build-configuration-file-to-your-project#Map-build-settings-to-a-build-configuration ↩

Copy link
Contributor

Choose a reason for hiding this comment

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

the show build settings xcode command can only accept one of the target or scheme flag. Therefore I make them optional.

Running xcodebuild -scheme iosapp -configuration jk -target iosapp -showBuildSettings in one of my local projects doesn't result in any error. Are you sure that the command only accepts exactly one of -scheme and -target?

Copy link
Contributor

@vashworth vashworth Sep 13, 2023 •
edited

Choose a reason for hiding this comment

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

So getBuildSettings specifies -project, which if -project is specified, you can only use either -scheme or -target. However, if you were to exclude the -project flag and use -scheme and -target together, it basically just ignores the -target flag.

So @andrewkolos xcodebuild -scheme iosapp -configuration jk -target iosapp -showBuildSettings works because -project is not specified.

Also, Build Configuration is not per scheme but per project, although in flutter tooling we use the scheme to determine the configuration for flavors.

When -scheme is specified, it shows the build settings for all runnable targets. This can be seen if you edit the scheme and check the Run column for more than one targets.

Screenshot 2023-09-13 at 10 44 01 AM

So if I ran xcodebuild -project xxx -configuration xxx -scheme Runner -showBuildSettings, it would show something like:

Build settings for action build and target Runner:
...
Build settings for action build and target RunnerTests:
...

When -target is set, it will only show the build settings for only the set target.

Copy link
Contributor

@vashworth vashworth Sep 13, 2023 •
edited

Choose a reason for hiding this comment

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

Looking at our code, seems like we don't really handle multiple targets per scheme. It operates under the assumption that there's only one target per scheme.

I think there are improvements that could be made here, but probably out of scope for this PR. I filed an issue to follow up on that: #134669

For your PR, since -configuration is required, I think it's okay to only use -target.

If -configuration was not set, you would have needed -scheme to get the correct -configuration for flavors.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good, thanks for explanation. I will update the code.

String? target,
}) async {
if ((scheme != null) == (target != null)) {
throwToolExit('Only one of scheme or target must be provide but not both');
}
final XcodeProjectBuildContext context = XcodeProjectBuildContext(
configuration: configuration,
scheme: scheme,
Expand All @@ -233,6 +237,7 @@ class IosProject extends XcodeBasedProject {
.childDirectory('deeplink_data')
.childFile('universal-link-settings-$configuration-$scheme-$target.json')
.create(recursive: true);

await file.writeAsString(jsonEncode(<String, Object?>{
'bundleIdentifier': await _productBundleIdentifierWithBuildContext(context),
'teamIdentifier': await _getTeamIdentifier(context),
Expand Down
50 changes: 46 additions & 4 deletions packages/flutter_tools/test/commands.shard/hermetic/ios_analyze_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,26 +64,44 @@ void main() {
}
});

testWithoutContext('can output json file', () async {
testWithoutContext('can output json file with scheme', () async {
final MockIosProject ios = MockIosProject();
final MockFlutterProject project = MockFlutterProject(ios);
const String expectedConfig = 'someConfig';
const String expectedScheme = 'someScheme';
const String expectedTarget = 'someConfig';
const String expectedOutputFile = '/someFile';
ios.outputFileLocation = expectedOutputFile;
await IOSAnalyze(
project: project,
option: IOSAnalyzeOption.outputUniversalLinkSettings,
configuration: expectedConfig,
scheme: expectedScheme,
target: expectedTarget,
logger: logger,
).analyze();
expect(logger.statusText, contains(expectedOutputFile));
expect(ios.outputConfiguration, expectedConfig);
expect(ios.outputScheme, expectedScheme);
expect(ios.outputTarget, null);
});

testWithoutContext('can output json file with target', () async {
final MockIosProject ios = MockIosProject();
final MockFlutterProject project = MockFlutterProject(ios);
const String expectedConfig = 'someConfig';
const String expectedTarget = 'someTarget';
const String expectedOutputFile = '/someFile';
ios.outputFileLocation = expectedOutputFile;
await IOSAnalyze(
project: project,
option: IOSAnalyzeOption.outputUniversalLinkSettings,
configuration: expectedConfig,
target: expectedTarget,
logger: logger,
).analyze();
expect(logger.statusText, contains(expectedOutputFile));
expect(ios.outputConfiguration, expectedConfig);
expect(ios.outputTarget, expectedTarget);
expect(ios.outputScheme, null);
});

testWithoutContext('can list build options', () async {
Expand Down Expand Up @@ -132,6 +150,30 @@ void main() {
),
);
});

testUsingContext('throws if too much parameters', () async {
final Directory tempDir = fileSystem.systemTempDirectory.createTempSync('someTemp');
await expectLater(
runner.run(
<String>[
'analyze',
'--ios',
'--output-universal-link-settings',
'--configuration=config',
'--target=target',
'--scheme=scheme',
tempDir.path,
],
),
throwsA(
isA<Exception>().having(
(Exception e) => e.toString(),
'description',
contains('Only one of "--target" or "--scheme" must be provided but not both.'),
),
),
);
});
});
}

Expand All @@ -150,7 +192,7 @@ class MockIosProject extends Fake implements IosProject {
late XcodeProjectInfo expectedProjectInfo;

@override
Future<String> outputsUniversalLinkSettings({required String configuration, required String scheme, required String target}) async {
Future<String> outputsUniversalLinkSettings({required String configuration, String? scheme, String? target}) async {
outputConfiguration = configuration;
outputScheme = scheme;
outputTarget = target;
Expand Down
32 changes: 26 additions & 6 deletions packages/flutter_tools/test/general.shard/project_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -733,7 +733,6 @@ apply plugin: 'kotlin-android'

const XcodeProjectBuildContext buildContext = XcodeProjectBuildContext(
target: 'Runner',
scheme: 'Debug',
configuration: 'config',
);
xcodeProjectInterpreter.buildSettingsByBuildContext[buildContext] = <String, String>{
Expand All @@ -753,7 +752,6 @@ apply plugin: 'kotlin-android'
);
final String outputFilePath = await project.ios.outputsUniversalLinkSettings(
target: 'Runner',
scheme: 'Debug',
configuration: 'config',
);
final File outputFile = fs.file(outputFilePath);
Expand Down Expand Up @@ -781,7 +779,6 @@ apply plugin: 'kotlin-android'

const XcodeProjectBuildContext buildContext = XcodeProjectBuildContext(
target: 'Runner',
scheme: 'Debug',
configuration: 'config',
);
xcodeProjectInterpreter.buildSettingsByBuildContext[buildContext] = <String, String>{
Expand All @@ -802,7 +799,6 @@ apply plugin: 'kotlin-android'

final String outputFilePath = await project.ios.outputsUniversalLinkSettings(
target: 'Runner',
scheme: 'Debug',
configuration: 'config',
);
final File outputFile = fs.file(outputFilePath);
Expand All @@ -827,7 +823,6 @@ apply plugin: 'kotlin-android'

const XcodeProjectBuildContext buildContext = XcodeProjectBuildContext(
target: 'Runner',
scheme: 'Debug',
configuration: 'config',
);
xcodeProjectInterpreter.buildSettingsByBuildContext[buildContext] = <String, String>{
Expand All @@ -838,7 +833,6 @@ apply plugin: 'kotlin-android'
testPlistUtils.setProperty(PlistParser.kCFBundleIdentifierKey, r'$(PRODUCT_BUNDLE_IDENTIFIER)');
final String outputFilePath = await project.ios.outputsUniversalLinkSettings(
target: 'Runner',
scheme: 'Debug',
configuration: 'config',
);
final File outputFile = fs.file(outputFilePath);
Expand All @@ -847,6 +841,32 @@ apply plugin: 'kotlin-android'
expect(json['bundleIdentifier'], 'io.flutter.someProject');
expect(json['associatedDomains'], unorderedEquals(<String>[]));
});

testWithMocks('throws when both scheme and target are provided', () async {
final FlutterProject project = await someProject();
project.ios.xcodeProject.createSync();
project.ios.defaultHostInfoPlist.createSync(recursive: true);
expect(
() => project.ios.outputsUniversalLinkSettings(
target: 'Runner',
scheme: 'scheme',
configuration: 'config',
),
throwsToolExit(),
);
});

testWithMocks('throws when non of the scheme and target is provided', () async {
final FlutterProject project = await someProject();
project.ios.xcodeProject.createSync();
project.ios.defaultHostInfoPlist.createSync(recursive: true);
expect(
() => project.ios.outputsUniversalLinkSettings(
configuration: 'config',
),
throwsToolExit(),
);
});
});

group('product bundle identifier', () {
Expand Down