Skip to content

Pass environment variables through to xcodebuild#43553

Merged
jmagman merged 4 commits into
flutter:masterfrom
jmagman:env-xcodebuild
Oct 29, 2019
Merged

Pass environment variables through to xcodebuild#43553
jmagman merged 4 commits into
flutter:masterfrom
jmagman:env-xcodebuild

Conversation

@jmagman

@jmagman jmagman commented Oct 26, 2019 •
edited
Loading

Copy link
Copy Markdown
Member

Description

Allow developers to pass in arbitrary Xcode build settings via environment variables prefixed with FLUTTER_XCODE_. This will prevent the need for a new flutter flag for every build setting a developer may want to change.

We can use this in our device lab to override code signing Xcode settings without needing to edit every integration test Xcode project, for example.

Removes the need for patterns like FLUTTER_DEVICELAB_XCODE_PROVISIONING_CONFIG that inject xcconfig files into Generated files (see https://github.com/flutter/flutter/pull/10736/files)

Related Issues

See proposal in #37231 (comment).

Tests

Added a few xcodebuild commands tests.

Checklist

  • 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

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

@jmagman jmagman added c: contributor-productivity Team-specific productivity, code health, technical debt. platform-ios iOS applications specifically tool Affects the "flutter" command-line tool. See also t: labels. t: xcode "xcodebuild" on iOS and general Xcode project management team-infra Owned by Infrastructure team labels Oct 26, 2019
@jmagman

jmagman commented Oct 26, 2019

Copy link
Copy Markdown
Member Author

Do you guys have suggestions for where to document this?

@jmagman

jmagman commented Oct 26, 2019

Copy link
Copy Markdown
Member Author

\cc @godofredoc

@jmagman jmagman force-pushed the env-xcodebuild branch 2 times, most recently from 3a17c4b to b7c98b6 Compare October 28, 2019 21:58
christopherfujino
christopherfujino reviewed Oct 28, 2019
View reviewed changes
Comment thread packages/flutter_tools/lib/src/ios/xcodeproj.dart Outdated

@christopherfujino christopherfujino Oct 28, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Could you refactor this a little? I know you have tests for this, but it's difficult to read now, and I worry that later on contributors might make one or two little tweaks to it...

christopherfujino
christopherfujino reviewed Oct 28, 2019
View reviewed changes
Comment thread packages/flutter_tools/test/general.shard/ios/xcodeproj_test.dart Outdated

@christopherfujino christopherfujino Oct 28, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Cool, I didn't know about testUsingOsxContext

@christopherfujino

christopherfujino commented Oct 28, 2019

Copy link
Copy Markdown
Contributor

Do you guys have suggestions for where to document this?

Good question. Maybe a new wiki page for Xcode configuration?

jmagman
jmagman commented Oct 28, 2019
View reviewed changes
Comment thread packages/flutter_tools/lib/src/ios/xcodeproj.dart Outdated

@jmagman jmagman Oct 28, 2019

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@christopherfujino Is this more or less readable than what I had before?

@christopherfujino christopherfujino Oct 28, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It's glorious.

christopherfujino
christopherfujino approved these changes Oct 28, 2019
View reviewed changes

@christopherfujino christopherfujino left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

Comment thread packages/flutter_tools/lib/src/ios/xcodeproj.dart Outdated

@christopherfujino christopherfujino Oct 28, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It's glorious.

jonahwilliams
jonahwilliams reviewed Oct 29, 2019
View reviewed changes
Comment thread packages/flutter_tools/lib/src/ios/xcodeproj.dart Outdated

@jonahwilliams jonahwilliams Oct 29, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit: I would chain all of these Iterable methods into a single expression. There is a lot of lhs real-estate dedicated to very temporary identifiers.

@jmagman jmagman Oct 29, 2019

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@christopherfujino said:
#43553 (comment)

and then even claimed:
#43553 (comment)

Let me split the difference.

jonahwilliams
jonahwilliams reviewed Oct 29, 2019
View reviewed changes
Comment thread packages/flutter_tools/lib/src/ios/xcodeproj.dart Outdated

@jonahwilliams jonahwilliams Oct 29, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It's perfectly fine to return a list here, but note that all of the contexts you are using this in can accept an Iterable as well.

jonahwilliams
jonahwilliams reviewed Oct 29, 2019
View reviewed changes
Comment thread packages/flutter_tools/test/general.shard/ios/xcodeproj_test.dart Outdated

@jonahwilliams jonahwilliams Oct 29, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

At runtime the map used by platform.environment will be unmodifiable. For tests that mock the platform, its good to use a const map or Map.unmodifiable to ensure there is no accidental setting of values, since this will throw during the normal runtime.

@jmagman jmagman Oct 29, 2019

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Good idea.

jonahwilliams
jonahwilliams approved these changes Oct 29, 2019
View reviewed changes

@jonahwilliams jonahwilliams left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM with nit

@jmagman jmagman merged commit 01dc19b into flutter:master Oct 29, 2019
@jmagman jmagman deleted the env-xcodebuild branch October 29, 2019 18:15
@jmagman jmagman mentioned this pull request Nov 6, 2019
Closed
@digiter digiter mentioned this pull request Nov 16, 2019
Closed
Inconnu08 pushed a commit to Inconnu08/flutter that referenced this pull request Nov 26, 2019
@jmagman @Inconnu08
Pass environment variables through to xcodebuild (flutter#43553)
99992ee
@jmagman jmagman mentioned this pull request Jan 23, 2020
Open
@jmagman jmagman mentioned this pull request Dec 7, 2020
Merged
@renancaraujo renancaraujo mentioned this pull request Apr 28, 2021
Closed
@github-actions github-actions Bot locked as resolved and limited conversation to collaborators Aug 3, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

2 more reviewers
@christopherfujino christopherfujino christopherfujino approved these changes
@jonahwilliams jonahwilliams jonahwilliams approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

c: contributor-productivity Team-specific productivity, code health, technical debt. platform-ios iOS applications specifically t: xcode "xcodebuild" on iOS and general Xcode project management team-infra Owned by Infrastructure team 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.

4 participants

@jmagman @christopherfujino @jonahwilliams @googlebot