Set the LANG when invoking cocoapods by dnfield · Pull Request #45710 · flutter/flutter

dnfield · 2019-11-27T17:31:04Z

Description

When invoked from the command line, this is fine, but when invoked from an IDE the $LANG env var isn't set by the IDE. We could remove this is Coco

Related Issues

google/flutter-desktop-embedding#586
CocoaPods/CocoaPods#9347

Tests

I added the following tests:

Updated tests to expect this variable is set.

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 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.

Breaking Change

Does your PR require Flutter developers to manually update their apps to accommodate your change?

Yes, this is a breaking change (Please read Handling breaking changes). Replace this with a link to the e-mail where you asked for input on this proposed change.
No, this is not a breaking change.

DanTup

Thanks! 🙂

Added one question about a fallback (since I saw localeName return null in a Flutter app, but I don't know if it was specifically being in a Flutter app that was the reason).

DanTup · 2019-11-27T17:39:28Z

dev/bots/test.dart

      <String>['install'],
      workingDirectory: podfile.parent.path,
+      environment: <String, String>{
+        'LANG': '${Platform.localeName}.UTF8',


Do we need a fallback if Platform.localeName is not set? I don't know if it'd happen for a CLI app, but when I was testing I noticed it wasn't set when I printed it inside a Flutter app, though it was set when running a CLI app (even when there was no LANG var set invoking the CLI app).

https://github.com/dart-lang/sdk/blob/master/runtime/bin/platform_macos.cc#L190 Is the impl for macOS/iOS. https://github.com/dart-lang/sdk/blob/master/runtime/bin/platform_android.cc#L121 android.

These look like they shouldn't return null... In fact,the main wrapper for it throws an error if they do.

/cc @zanderso who probably knows more about that

This is running on the iOS Simulator (iPhone), it gets null.

If it's only when running on-device, it might not matter (I can't imagine the flutter tool will be running pod on a device), but if there's another reason it's null (eg. that might occur for a real flutter_tool user on their host OS) we might need to guard against it (defaulting to en_US would presumably be fine).

Platform.localeName can give null if the embedder overrides _Platform._localeClosure with a function that returns null. See: https://github.com/dart-lang/sdk/blob/master/sdk/lib/io/platform_impl.dart#L48.

The command line Dart VM doesn't override it, so Platform.localeName won't give null.

The engine does override it here: https://github.com/flutter/engine/blob/32ca0cc7d347235d8ae1ed53e49c4f0de97bd03c/lib/ui/dart_runtime_hooks.cc#L143

With: https://github.com/flutter/engine/blob/32ca0cc7d347235d8ae1ed53e49c4f0de97bd03c/lib/ui/hooks.dart#L68

Got it - sounds like it'll always be set in for contexts where this code runs then.

Actually, there are test failures are because this is null somewhere that's being called while testing:

00:06 +58: test/general.shard/persistent_tool_state_test.dart: state can be set and persists NoSuchMethodError: The method 'replaceAll' was called on null. Receiver: null Tried calling: replaceAll("-", "_") #0 Object.noSuchMethod (dart:core-patch/object_patch.dart:53:5) #1 CocoaPods.cocoaPodsVersionText (package:flutter_tools/src/macos/cocoapods.dart:83:40)

DanTup · 2019-11-27T18:04:58Z

Ok, so I tested this with:

cd examples/flutter_gallery
flutter clean
LANG= flutter run -d macos -v

But it didn't work... After some fiddling, I found that it has to be UTF-8 (note the dash) and that the first part needs to be en_GB and not en-GB (as given by Dart).

So this worked:

'${platform.localeName.replaceAll('-', '_')}.UTF-8'

I can't find any info on these formats (eg. why it needs to be an underscore there, yet Dart has a dash) 🤷‍♂️

zanderso · 2019-11-27T18:33:02Z

On macOS, the command line VM doesn't use LANG to compute Platform.localeName. See: https://github.com/dart-lang/sdk/blob/master/runtime/bin/platform_macos.cc#L190

dnfield · 2019-11-27T18:43:02Z

Is there any chance that we just want to always force this to en_US.UTF-8?

DanTup · 2019-11-27T21:04:58Z

Is there any chance that we just want to always force this to en_US.UTF-8?

🤷🏼‍♂️

I'm not all that familiar with CocoaPods. I can't think of anything obvious it might do differently based on language, and if we used the existing env variable first and only set that if it wasn't already set, I think that limits the scope (and gives a way for people to force it back if required).

jmagman · 2019-11-27T21:09:23Z

I'm not all that familiar with CocoaPods. I can't think of anything obvious it might do differently based on language, and if we used the existing env variable first and only set that if it wasn't already set, I think that limits the scope (and gives a way for people to force it back if required).

It's using https://docs.ruby-lang.org/en/2.1.0/Encoding.html to check for a Encoding::UTF_8 ("UTF-8") match.

dnfield · 2019-11-27T23:57:48Z

I'll keep using the (massaged) platform encoding. I'm just thinking the Podfile is always going to be en_US encoded.

DanTup · 2019-11-28T09:01:28Z

Tested with the update and it seems to work fine, so LGTM once the tests pass (which seems to be because of a null for localeName somewhere).

jmagman

Is there any chance that we just want to always force this to en_US.UTF-8?

That would be my vote. Just hard-code it.

dnfield · 2019-12-02T19:31:56Z

That will probably resolve my test issue as well. I'm going to do that for now. We can fix it later if we determine that you ever want a non en_US locale for pod.

jmagman

LGTM

jmagman · 2019-12-02T20:26:15Z

packages/flutter_tools/test/general.shard/macos/cocoapods_test.dart

        'FLUTTER_FRAMEWORK_DIR': 'engine/path',
        'COCOAPODS_DISABLE_STATS': 'true',
-        'CP_REPOS_DIR': cocoapodsRepoDir,
+        'LANG': 'en_US.UTF8',


Did you mean to replace CP_REPOS_DIR?

It should be back now.

dnfield · 2019-12-02T20:35:07Z

As long as this passes CI, going to look to land on red since it seems like it should fix the redness.

DanTup · 2019-12-03T08:06:37Z

@dnfield Thanks! Tested this on master and it now works both from CLI and VS Code 👍

Set the LANG when invoking cocoapods

66b1b7b

dnfield requested review from DanTup and jmagman November 27, 2019 17:31

fluttergithubbot added c: contributor-productivity Team-specific productivity, code health, technical debt. tool Affects the "flutter" command-line tool. See also t: labels. labels Nov 27, 2019

googlebot added the cla: yes label Nov 27, 2019

DanTup reviewed Nov 27, 2019

View reviewed changes

massage the platform name

e685365

dnfield added 2 commits November 28, 2019 01:10

localeNAme

94b542e

Merge remote-tracking branch 'upstream/master' into cocoapods_lang

69378b4

jmagman reviewed Dec 2, 2019

View reviewed changes

dnfield added 2 commits December 2, 2019 12:23

Hard code to en_US

87dfbc0

Merge remote-tracking branch 'upstream/master' into cocoapods_lang

7b90866

dnfield force-pushed the cocoapods_lang branch from d097625 to 7b90866 Compare December 2, 2019 20:24

whups

e2fd2fd

jmagman approved these changes Dec 2, 2019

View reviewed changes

one more

400a9d1

jmagman approved these changes Dec 2, 2019

View reviewed changes

dnfield merged commit 088fa24 into flutter:master Dec 2, 2019

dnfield deleted the cocoapods_lang branch December 2, 2019 21:26

This was referenced Dec 3, 2019

[macOS]: failed to launch from VSCode: ArgumentError - invalid byte sequence in US-ASCII google/flutter-desktop-embedding#586

Closed

[macOS]: failed to launch from VSCode: ArgumentError - invalid byte sequence in US-ASCII Dart-Code/Dart-Code#2035

Closed

jmagman mentioned this pull request Dec 3, 2019

iOS run error with cocoapods #45853

Closed

github-actions bot locked as resolved and limited conversation to collaborators Aug 3, 2021

Conversation

dnfield commented Nov 27, 2019

Description

Related Issues

Tests

Checklist

Breaking Change

Uh oh!

DanTup left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DanTup commented Nov 27, 2019

Uh oh!

zanderso commented Nov 27, 2019

Uh oh!

dnfield commented Nov 27, 2019

Uh oh!

DanTup commented Nov 27, 2019

Uh oh!

jmagman commented Nov 27, 2019

Uh oh!

dnfield commented Nov 27, 2019

Uh oh!

DanTup commented Nov 28, 2019

Uh oh!

jmagman left a comment

Choose a reason for hiding this comment

Uh oh!

dnfield commented Dec 2, 2019

Uh oh!

jmagman left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dnfield commented Dec 2, 2019

Uh oh!

DanTup commented Dec 3, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants