Skip to content

[ci] Run Swift formatter and linter during CI formatting #5928

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

Merged
auto-submit merged 5 commits into from
Jan 19, 2024
Merged

[ci] Run Swift formatter and linter during CI formatting #5928

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
6316bd9
[ci] Run Swift formatter during CI formatting
jmagman Jan 16, 2024
8a4da67
Review edits
jmagman Jan 19, 2024
918285e
Remove --swift
jmagman Jan 19, 2024
cd50880
Review edits
jmagman Jan 19, 2024
a5a6250
Skip swift during pigeon generation
jmagman Jan 19, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 13 additions & 1 deletion .ci.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1071,7 +1071,11 @@ targets:
properties:
add_recipes_cq: "true"
version_file: flutter_master.version
target_file: macos_check_podspecs.yaml
target_file: macos_repo_checks.yaml
dependencies: >
[
{"dependency": "swift_format", "version": "build_id:8797338980206841409"}
]

### macOS desktop tasks ###
# macos-platform_tests builds all the packages on ARM, so this build is run
Expand Down Expand Up @@ -1143,6 +1147,10 @@ targets:
{
"CHANNEL": "master"
}
dependencies: >
[
{"dependency": "swift_format", "version": "build_id:8797338979890974865"}
Copy link
Member Author

@jmagman jmagman Jan 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

The arm64 and x64 build_id numbers are different, but it's pointing to the same version.

arm: https://chrome-infra-packages.appspot.com/p/infra/3pp/tools/swift-format/mac-arm64/+/RLmOtWwcNQeEUBqiZdpqEtJVh-cdm95Bbcz0W-wC124C
x64: https://chrome-infra-packages.appspot.com/p/infra/3pp/tools/swift-format/mac-amd64/+/xG4XM0suWSr-M7wBW_zojQ83hgZRnRVO2qVYy46d2PgC

#5928 (comment)

]

- name: Mac_arm64 custom_package_tests stable
recipe: packages/packages
Expand All @@ -1156,6 +1164,10 @@ targets:
{
"CHANNEL": "stable"
}
dependencies: >
[
{"dependency": "swift_format", "version": "build_id:8797338979890974865"}
]

### iOS tasks ###
# ios_platform_tests builds all the packages on ARM, so this build is run
Expand Down
7 changes: 7 additions & 0 deletions .ci/targets/macos_check_podspecs.yaml → .ci/targets/macos_repo_checks.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,13 @@ tasks:
- name: update pods repo
script: .ci/scripts/update_pods.sh
infra_step: true # Note infra steps failing prevents "always" from running.
- name: Swift format
script: script/tool_runner.sh
# Non-Swift languages are formatted on Linux builders.
# Skip them on Mac builder to avoid duplication.
args: ["format", "--fail-on-change", "--no-dart", "--no-clang-format", "--no-kotlin", "--no-java" ]
always: true
- name: validate iOS and macOS podspecs
script: script/tool_runner.sh
args: ["podspec-check"]
always: true
3 changes: 2 additions & 1 deletion .ci/targets/repo_checks.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,8 @@ tasks:
infra_step: true # Note infra steps failing prevents "always" from running.
- name: format
script: script/tool_runner.sh
args: ["format", "--fail-on-change"]
# Skip Swift formatting on Linux builders.
args: ["format", "--fail-on-change", "--no-swift"]
always: true
- name: license validation
script: script/tool_runner.sh
Expand Down
2 changes: 1 addition & 1 deletion packages/file_selector/file_selector_macos/example/macos/RunnerTests/RunnerTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ class TestViewProvider: NSObject, ViewProvider {
var window: NSWindow? = NSWindow()
}

class exampleTests: XCTestCase {
class ExampleTests: XCTestCase {
Copy link
Member Author

@jmagman jmagman Jan 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

linter (not format) error:

RunnerTests.swift:43:7: warning: [TypeNamesShouldBeCapitalized] rename the class 'exampleTests' using UpperCamelCase; for example, 'ExampleTests'

No CHANGELOG or version bump needed based on #5933 (comment)


func testOpenSimple() throws {
let panelController = TestPanelController()
Expand Down
1 change: 1 addition & 0 deletions packages/pigeon/tool/shared/generation.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -242,6 +242,7 @@ Future<int> formatAllFiles({required String repositoryRoot}) {
'run',
'script/tool/bin/flutter_plugin_tools.dart',
'format',
'--no-swift',
'--packages=pigeon',
],
workingDirectory: repositoryRoot,
Expand Down
42 changes: 33 additions & 9 deletions script/tool/lib/src/format_command.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,8 @@ class FormatCommand extends PackageCommand {
argParser.addFlag(_kotlinArg,
help: 'Format Kotlin files', defaultsTo: true);
argParser.addFlag(_javaArg, help: 'Format Java files', defaultsTo: true);
argParser.addFlag(_swiftArg, help: 'Format Swift files');
argParser.addFlag(_swiftArg,
help: 'Format and lint Swift files', defaultsTo: true);
argParser.addOption(_clangFormatPathArg,
defaultsTo: 'clang-format', help: 'Path to "clang-format" executable.');
argParser.addOption(_javaPathArg,
Expand Down Expand Up @@ -105,7 +106,7 @@ class FormatCommand extends PackageCommand {
await _formatCppAndObjectiveC(files);
}
if (getBoolArg(_swiftArg)) {
await _formatSwift(files);
await _formatAndLintSwift(files);
}

if (getBoolArg('fail-on-change')) {
Expand Down Expand Up @@ -177,16 +178,32 @@ class FormatCommand extends PackageCommand {
}
}

Future<void> _formatSwift(Iterable<String> files) async {
final String swiftFormat = await _findValidSwiftFormat();
final Iterable<String> swiftFiles =
_getPathsWithExtensions(files, <String>{'.swift'});
Future<void> _formatAndLintSwift(Iterable<String> files) async {
// TODO(jmagman): Remove generated file filter when pigeon Swift generation matches swift-format.
// https://github.com/flutter/flutter/issues/141799
final Iterable<String> swiftFiles = _filterGeneratedFiles(
_getPathsWithExtensions(files, <String>{'.swift'}));
if (swiftFiles.isNotEmpty) {
final String swiftFormat = await _findValidSwiftFormat();
print('Formatting .swift files...');
final int exitCode =
final int formatExitCode =
await _runBatched(swiftFormat, <String>['-i'], files: swiftFiles);
if (exitCode != 0) {
printError('Failed to format Swift files: exit code $exitCode.');
if (formatExitCode != 0) {
printError('Failed to format Swift files: exit code $formatExitCode.');
throw ToolExit(_exitSwiftFormatFailed);
}

print('Linting .swift files...');
final int lintExitCode = await _runBatched(
swiftFormat,
<String>[
'lint',
'--parallel',
'--strict',
],
files: swiftFiles);
if (lintExitCode != 0) {
printError('Failed to lint Swift files: exit code $lintExitCode.');
throw ToolExit(_exitSwiftFormatFailed);
}
}
Expand Down Expand Up @@ -342,6 +359,13 @@ class FormatCommand extends PackageCommand {
(String filePath) => extensions.contains(path.extension(filePath)));
}

Iterable<String> _filterGeneratedFiles(Iterable<String> files) {
Copy link
Collaborator

@stuartmorgan-g stuartmorgan-g Jan 19, 2024

Choose a reason for hiding this comment

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

Rather than filtering out generated files, we just need Pigeon to do Swift formatting in CI; this code should include Swift formatting. If we changed the default then it'll just work, otherwise we need to add --swift there.

Copy link
Collaborator

@stuartmorgan-g stuartmorgan-g Jan 19, 2024

Choose a reason for hiding this comment

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

Oh, but that's not going to work on the Linux machine where we run this.

Hm, do we have a clang-format CIPD package for macOS already? If so, we could change this to macOS instead of Linux, and adjust the dependencies in ci.yaml. If not I'll need to think more about how we can handles this sanely, and we can land with the exclusion in the meantime.

Copy link
Collaborator

@stuartmorgan-g stuartmorgan-g Jan 19, 2024

Choose a reason for hiding this comment

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

Maybe it's easier to change the Pigeon validator to check specific languages for changes instead of checking everything for changes, and then run it for different languages on different CI hosts the way we do the format check itself. I'll play with that today.

Copy link
Collaborator

@stuartmorgan-g stuartmorgan-g Jan 19, 2024

Choose a reason for hiding this comment

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

Maybe it's easier to change the Pigeon validator to check specific languages for changes instead of checking everything for changes, and then run it for different languages on different CI hosts the way we do the format check itself.

I think #5944 should make this work.

Copy link
Member Author

@jmagman jmagman Jan 19, 2024

Choose a reason for hiding this comment

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

I changed my comment about removing it into a TODO. How about I land this, then you rebase #5944 onto ToT and remove the _filterGeneratedFiles in that PR?

Copy link
Collaborator

@stuartmorgan-g stuartmorgan-g Jan 19, 2024

Choose a reason for hiding this comment

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

👍🏻

Copy link
Collaborator

@stuartmorgan-g stuartmorgan-g Jan 19, 2024

Choose a reason for hiding this comment

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

Rather than filtering out generated files, we just need Pigeon to do Swift formatting in CI; this code should include Swift formatting.

Oops, now that swift is enabled by default, that same code will need (until my PR lands) --no-swift.

return files.where((String filePath) {
final String basename = path.basename(filePath);
return !basename.contains('.gen.') && !basename.contains('.g.');
});
}

Future<String> _getJavaFormatterPath() async {
final String javaFormatterPath = path.join(
path.dirname(path.fromUri(platform.script)),
Expand Down
76 changes: 70 additions & 6 deletions script/tool/test/format_command_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -551,15 +551,45 @@ void main() {
processRunner.recordedCalls,
orderedEquals(<ProcessCall>[
const ProcessCall(
'/path/to/swift-format', <String>['--version'], null),
'/path/to/swift-format',
<String>['--version'],
null,
),
ProcessCall(
'/path/to/swift-format',
<String>['-i', ...getPackagesDirRelativePaths(plugin, files)],
packagesDir.path),
'/path/to/swift-format',
<String>['-i', ...getPackagesDirRelativePaths(plugin, files)],
packagesDir.path,
),
ProcessCall(
'/path/to/swift-format',
<String>[
'lint',
'--parallel',
'--strict',
...getPackagesDirRelativePaths(plugin, files),
],
packagesDir.path,
),
]));
});

test('skips Swift if --swift flag is not provided', () async {
test('skips generated Swift files', () async {
const List<String> files = <String>[
'macos/foo.gen.swift',
'macos/foo.g.swift',
];
createFakePlugin(
'a_plugin',
packagesDir,
extraFiles: files,
);

await runCapturingPrint(runner, <String>['format', '--swift']);

expect(processRunner.recordedCalls, orderedEquals(<ProcessCall>[]));
});

test('skips Swift if --no-swift flag is provided', () async {
const List<String> files = <String>[
'macos/foo.swift',
];
Expand All @@ -569,7 +599,7 @@ void main() {
extraFiles: files,
);

await runCapturingPrint(runner, <String>['format']);
await runCapturingPrint(runner, <String>['format', '--no-swift']);

expect(processRunner.recordedCalls, orderedEquals(<ProcessCall>[]));
});
Expand Down Expand Up @@ -601,6 +631,40 @@ void main() {
]));
});

test('fails if swift-format lint fails', () async {
const List<String> files = <String>[
'macos/foo.swift',
];
createFakePlugin('a_plugin', packagesDir, extraFiles: files);

processRunner.mockProcessesForExecutable['swift-format'] =
<FakeProcessInfo>[
FakeProcessInfo(MockProcess(),
<String>['--version']), // check for working swift-format
FakeProcessInfo(MockProcess(), <String>['-i']),
FakeProcessInfo(MockProcess(exitCode: 1), <String>[
'lint',
'--parallel',
'--strict',
]),
];
Error? commandError;
final List<String> output = await runCapturingPrint(runner, <String>[
'format',
'--swift',
'--swift-format-path=swift-format'
], errorHandler: (Error e) {
commandError = e;
});

expect(commandError, isA<ToolExit>());
expect(
output,
containsAllInOrder(<Matcher>[
contains('Failed to lint Swift files: exit code 1.'),
]));
});

test('fails if swift-format fails', () async {
const List<String> files = <String>[
'macos/foo.swift',
Expand Down