Skip to content
This repository was archived by the owner on Jun 13, 2024. It is now read-only.

Use Synthetic Packages for localizations#282

Merged
shihaohong merged 6 commits intoflutter:masterfrom
shihaohong:synthetic-l10n
Sep 1, 2020
Merged

Use Synthetic Packages for localizations#282
shihaohong merged 6 commits intoflutter:masterfrom
shihaohong:synthetic-l10n

Conversation

@shihaohong
Copy link

@shihaohong shihaohong commented Sep 1, 2020 •
edited
Loading

With synthetic package support in the gen_l10n tool (flutter/flutter#62395), we no longer need to check in generated localizations .dart files into any Flutter project. Here is the laundry list of changes involved in the PR:

  1. This PR introduces an l10n.yaml file to set up localizations generation when flutter test, flutter pub get, flutter analyze are invoked. It also generates localizations on flutter run, hot reload and hot restart so that direct modifications to the arb files can be made and observed while in development.
  2. Removal of all generated .dart localizations files from source control, since it's no longer needed.
  3. Updated all imports to point to the synthetic package location.
  4. Slight update to the grind.dart tool that formats the generated code. I left the rest of the grind.dart command in there because it seems like we running flutter pub get in ${FLUTTER}/dev/tools is unfortunately still a necessity. My hope is to get rid of this requirement altogether, but I plan on doing this in a subsequent PR.
  5. Edit: Add flutter update-packages as a step in the CI temporarily since without it (issue filed [gen_l10n] pub get required in dev/tools when switching between flutter versions flutter#65023), flutter pub get does not work as expected since the gen_l10n tool's dependencies are not always in sync. This is particularly observed when switching between versions of flutter (ie. upgrading from stable to master branch, or vice versa)

rami-a
rami-a approved these changes Sep 1, 2020
View reviewed changes
Copy link
Contributor

@rami-a rami-a left a comment

Choose a reason for hiding this comment

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

LGTM, look at all those deleted lines!

@shihaohong
Copy link
Author

shihaohong commented Sep 1, 2020 •
edited
Loading

The tests are failing because the gen_l10n tool's dependencies aren't being properly created unless you run flutter update-packages, which is similar to the issue seen in flutter/samples#331. I added a step to the tests where flutter update-packages to avoid the is run before flutter pub get, which now generates synthetic packages.

However, since flutter update-packages is not meant to be used by Flutter users, my next goal is to update the gen_l10n tool itself so that this no longer becomes a problem.

perclasson
perclasson approved these changes Sep 1, 2020
View reviewed changes
Copy link
Contributor

@perclasson perclasson left a comment

Choose a reason for hiding this comment

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

LGTM, this is great!

tool/grind.dart
@@ -44,7 +44,6 @@ Future<void> generateLocalizations() async {
'--preferred-supported-locales=["en"]',
'--use-deferred-loading',
Copy link
Contributor

@perclasson perclasson Sep 1, 2020

Choose a reason for hiding this comment

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

Are these arguments still needed now with the l10n.yaml file? Or is it simply enough to just run gen_l10n.dart?

Copy link
Author

@shihaohong shihaohong Sep 1, 2020

Choose a reason for hiding this comment

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

The command itself shouldn't be necessary at all anymore, since localizations generation is baked into Flutter run, restart, reload, pub get, analyze and test.

The only reason I've left it in for now is because there's an issue with the gen_l10n tool's having the incorrect dependencies when switching between flutter versions that I still need to fix. Running the grinder command will always work, whereas the synthetic package generation may fail unless flutter update-packages is run.

@shihaohong shihaohong merged commit 00c8404 into flutter:master Sep 1, 2020
@clocksmith
Copy link
Contributor

clocksmith commented Sep 2, 2020 •
edited
Loading

I am having some trouble running the Gallery from the latest commit.

flutter upgrade is at

Flutter is already up to date on channel master
Flutter 1.22.0-10.0.pre.39 • channel master • https://github.com/flutter/flutter.git
Framework • revision a8281e31af (23 hours ago) • 2020-09-01 10:57:59 -0700
Engine • revision 165abef0a2
Tools • Dart 2.10.0 (build 2.10.0-77.0.dev)

and after running flutter update-packages, GalleryLocalizations is unable to be resolved:

Screen Shot 2020-09-02 at 12 23 01 PM

@clocksmith
Copy link
Contributor

clocksmith commented Sep 2, 2020

flutter clean, then flutter pub get recreated .dart_tool with the flutter_gen inside, so it works now. Thank you @Renzo-Olivares !

@xster
Copy link
Member

xster commented Sep 3, 2020

Is flutter gallery expected to run on flutter stable too?

I'm getting

Error detected in pubspec.yaml:
Unexpected child "generate" found under "flutter".
Please correct the pubspec.yaml file at /Users/xster/development/flutter-open/gallery/pubspec.yaml

if I run on stable.

@rami-a
Copy link
Contributor

rami-a commented Sep 3, 2020

Nope, gallery targets master https://github.com/flutter/gallery#running-flutter-gallery-on-flutters-master-channel so its expected that things won't always work on stable.

@xster
Copy link
Member

xster commented Sep 3, 2020

Understood, thanks for clarifying.

@xster
Copy link
Member

xster commented Sep 4, 2020

I've been getting this LUCI test issue when I sync flutter/flutter past this commit.

https://logs.chromium.org/logs/flutter/buildbucket/cr-buildbucket.appspot.com/8870127907410516896/+/steps/run_test.dart_for_hostonly_devicelab_tests_shard_and_subshard_3_last/0/stdout

Do we need to modify dev/bots/test.dart to unblock flutter/flutter?

This was referenced Sep 8, 2020
Closed
Merged
627171314
627171314 reviewed Sep 18, 2020
View reviewed changes
Copy link

@627171314 627171314 left a comment

Choose a reason for hiding this comment

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

image

@guidezpl
Copy link
Member

guidezpl commented Sep 18, 2020

@627171314 please follow the steps at #282 (comment), if that doesn't work, please file a separate issue.

@shihaohong shihaohong deleted the synthetic-l10n branch November 5, 2020 02:15
@pd4d10 pd4d10 mentioned this pull request Feb 3, 2021
Closed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

3 more reviewers

@627171314 627171314 627171314 left review comments

@perclasson perclasson perclasson approved these changes

@rami-a rami-a rami-a approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@shihaohong @clocksmith @xster @rami-a @guidezpl @perclasson @627171314