Skip to content

Move gen_l10n into flutter_tools#65025

Merged
HansMuller merged 28 commits intoflutter:masterfrom
shihaohong:gen-l10n-flutter-tool
Sep 3, 2020
Merged

Move gen_l10n into flutter_tools#65025
HansMuller merged 28 commits intoflutter:masterfrom
shihaohong:gen-l10n-flutter-tool

Conversation

@shihaohong
Copy link
Contributor

@shihaohong shihaohong commented Sep 1, 2020 •
edited
Loading

Description

flutter pub get or flutter update-packages needs to be run in flutter/dev/tools for the localizations generator to work because the dependencies of the localizations script is not cached or updated correctly. This is an issue with CI that requires switching flutter versions, and usually becomes an issue when users upgrade their flutter SDK when they're already using the gen_l10n tool.

This PR moves the gen_l10n script from dev/tools to flutter_tools to ensure that its dependencies are regenerated every time a new flutter tool snapshot is generated.

List of changes:

  1. Move gen_l10n files from dev/tools to packages/flutter_tools.
  2. Update gen_subtag_registry.dart so that language_subtag_registry.dart in dev/tools and packages/flutter_tools stay in sync and updated when the script is run.
  3. Leave an executable in dev/tools/localization/bin/gen_l10n.dart that calls flutter gen-l10n <options> so that this change is backwards compatible.
  4. Add a flutter gen-l10n <options> command that works the same way as the previous dart ${FLUTTER}/dev/tools/localization/bin/gen_l10n.dart <option> did.
  5. Modifications to existing build_system and gen_l10n CLI workflows to account for the move to flutter_tools, such as wiring a Logger into LocalizationsGenerator and updating the points in the code where separate dart process was called to generate localizations and instead invoking LocalizationsGenerator directly.

Related Issues

Fixes #65023

Tests

I added the following tests:

  • Moved all existing gen_l10n tests from dev/tools to packages/flutter_tools
  • Updated resident_runner_test.dart
  • Tests verifying a few success modes and a failure mode for the gen-l10n command.

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.

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I signed the CLA.
  • I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I updated/added relevant documentation (doc comments with ///).
  • All existing and new tests are passing.
  • The analyzer (flutter analyze --flutter-repo) does not report any problems on my PR.
  • I am willing to follow-up on review comments in a timely manner.

@shihaohong shihaohong added tool Affects the "flutter" command-line tool. See also t: labels. a: internationalization Supporting other languages or locales. (aka i18n) work in progress; do not review has reproducible steps The issue has been confirmed reproducible and is ready to work on labels Sep 1, 2020
@flutter-dashboard flutter-dashboard bot added the c: contributor-productivity Team-specific productivity, code health, technical debt. label Sep 1, 2020
@shihaohong shihaohong removed the has reproducible steps The issue has been confirmed reproducible and is ready to work on label Sep 1, 2020
@fluttergithubbot
Copy link
Contributor

fluttergithubbot commented Sep 1, 2020

This pull request is not suitable for automatic merging in its current state.

  • Please get at least one approved review before re-applying this label. Reviewers: If you left a comment approving, please use the "approve" review action instead.
  • The status or check suite Linux tool_tests has failed. Please fix the issues identified (or deflake) before re-applying this label.
  • The status or check suite Windows tool_tests has failed. Please fix the issues identified (or deflake) before re-applying this label.
  • The status or check suite tool_tests-general-linux has failed. Please fix the issues identified (or deflake) before re-applying this label.
  • The status or check suite tool_tests-integration-linux has failed. Please fix the issues identified (or deflake) before re-applying this label.
  • The status or check suite tool_tests-general-macos has failed. Please fix the issues identified (or deflake) before re-applying this label.
  • The status or check suite tool_tests-integration-macos has failed. Please fix the issues identified (or deflake) before re-applying this label.
  • The status or check suite customer_testing-macos has failed. Please fix the issues identified (or deflake) before re-applying this label.

@shihaohong shihaohong requested review from HansMuller and jonahwilliams and removed request for jonahwilliams September 2, 2020 11:30
@shihaohong
Copy link
Contributor Author

shihaohong commented Sep 2, 2020

I'm requesting review on the existing parts of the PR for now because of the timezone differences. I'll address any code review comments and add the generate_localizations command tests on the next day.

jonahwilliams
jonahwilliams reviewed Sep 2, 2020
View reviewed changes
packages/flutter_tools/lib/src/commands/generate_localizations.dart Outdated
GenerateLocalizationsCommand({
FileSystem fileSystem,
}) {
_fileSystem = fileSystem;
Copy link
Contributor

@jonahwilliams jonahwilliams Sep 2, 2020

Choose a reason for hiding this comment

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

do this in the initializer and it can be final

jonahwilliams
jonahwilliams reviewed Sep 2, 2020
View reviewed changes
packages/flutter_tools/lib/src/commands/generate_localizations.dart
import '../localizations/localizations_utils.dart';
import '../runner/flutter_command.dart';

class GenerateLocalizationsCommand extends FlutterCommand {
Copy link
Contributor

@jonahwilliams jonahwilliams Sep 2, 2020

Choose a reason for hiding this comment

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

nit: add a brief doc comment and link to the correct documentation?

jonahwilliams
jonahwilliams reviewed Sep 2, 2020
View reviewed changes
packages/flutter_tools/lib/src/commands/generate_localizations.dart Outdated

precacheLanguageAndRegionTags();

final String inputPathString = argResults['arb-dir'] as String;
Copy link
Contributor

@jonahwilliams jonahwilliams Sep 2, 2020

Choose a reason for hiding this comment

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

there are some helpers like stringArg, stringsArg, et cetera that avoid the casting

jonahwilliams
jonahwilliams reviewed Sep 2, 2020
View reviewed changes
packages/flutter_tools/lib/src/commands/generate_localizations.dart Outdated
..loadResources()
..writeOutputFiles()
..outputUnimplementedMessages(untranslatedMessagesFile, globals.logger);
} on FileSystemException catch (e) {
Copy link
Contributor

@jonahwilliams jonahwilliams Sep 2, 2020

Choose a reason for hiding this comment

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

consider not catching all file system exceptions: some of these might be programming errors in the tool.

Copy link
Contributor Author

@shihaohong shihaohong Sep 3, 2020

Choose a reason for hiding this comment

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

I've converted any FileSystemExceptions I threw in the tool into L10nExceptions and then removed the code catching file system exceptions

jonahwilliams
jonahwilliams reviewed Sep 2, 2020
View reviewed changes
packages/flutter_tools/lib/src/commands/generate_localizations.dart Outdated
String get description => 'Generate localizations for the Flutter project.';

@override
String get name => 'generate_localizations';
Copy link
Contributor

@jonahwilliams jonahwilliams Sep 2, 2020

Choose a reason for hiding this comment

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

consider an short name alias for typing - gen-l10n ?

Copy link
Contributor

@HansMuller HansMuller Sep 2, 2020

Choose a reason for hiding this comment

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

Note also: maybe we should use a hyphenated name for the long form as well.

jonahwilliams
jonahwilliams approved these changes Sep 2, 2020
View reviewed changes
Copy link
Contributor

@jonahwilliams jonahwilliams left a comment

Choose a reason for hiding this comment

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

Overall change LGTM with some nits

@shihaohong shihaohong changed the title [WIP] Move gen_l10n into flutter_tools Move gen_l10n into flutter_tools Sep 2, 2020
@shihaohong
Copy link
Contributor Author

shihaohong commented Sep 3, 2020

I manually set Google testing to pass because the tests were failing due to the diffs not matching with the PR's diffs, likely due to the recent merge of #62395 that has not been rolled into Google yet.

@shihaohong
Copy link
Contributor Author

shihaohong commented Sep 3, 2020 •
edited
Loading

Linux web_smoke_test seems to be failing for reasons that I believe are unrelated to my PR. It was passing a few commits ago prior to adding a few integration tests and addressing code review nits.

@jonahwilliams
Copy link
Contributor

jonahwilliams commented Sep 3, 2020

seems to be OK now?

@HansMuller HansMuller merged commit b80b432 into flutter:master Sep 3, 2020
@HansMuller
Copy link
Contributor

HansMuller commented Sep 3, 2020

I re-ran the failing test and it appears to have healed itself.

mingwandroid pushed a commit to mingwandroid/flutter that referenced this pull request Sep 6, 2020
@mingwandroid
Move gen_l10n into flutter_tools (flutter#65025)
506797f
@edwardaux edwardaux mentioned this pull request Nov 1, 2020
Closed
@shihaohong shihaohong deleted the gen-l10n-flutter-tool branch December 3, 2020 11:33
@jmagman jmagman mentioned this pull request Mar 24, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@HansMuller HansMuller Awaiting requested review from HansMuller

1 more reviewer

@jonahwilliams jonahwilliams jonahwilliams approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

a: internationalization Supporting other languages or locales. (aka i18n) c: contributor-productivity Team-specific productivity, code health, technical debt. tool Affects the "flutter" command-line tool. See also t: labels.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[gen_l10n] pub get required in dev/tools when switching between flutter versions

5 participants

@shihaohong @fluttergithubbot @jonahwilliams @HansMuller @googlebot