Skip to content

Migrate Runner project base configuration #54691

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

Jump to bottom
Merged
jmagman merged 2 commits into from
Apr 14, 2020
Merged

Migrate Runner project base configuration #54691

jmagman merged 2 commits into from
Apr 14, 2020

Conversation

@jmagman
Copy link
Member

@jmagman jmagman commented Apr 13, 2020 •
edited
Loading

Description

Remove Runner project base configuration from existing projects, and from template.

The Runner project was inheriting the build settings from the generated xcconfig file. As of #51453, that includes OTHER_LDFLAGS=$(inherited) -framework Flutter, which tells the linker to link on the Flutter framework. However, this was being inherited by all targets, not just Flutter or even iOS targets.

This migration removes the base configuration for the Runner project. It keeps it for the Runner target and any test targets, which is what actually needs to link on Flutter.

Screen Shot 2020-04-13 at 11 55 21 AM
becomes
Screen Shot 2020-04-13 at 11 57 12 AM

I ran flutter build ios on all example and integration test projects to prove this works. Also tested on the new devicelab test at #51126.

However, I had to manually migrate the flavors project, since that had custom build configurations. Users with custom build configurations that hit this issue (because they add a watchOS project, for example) will need to manually migrate their project base configuration.

Related Issues

Fixes #54680

Tests

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

Breaking Change

  • No, no existing tests failed, so this is not a breaking change.
  • Yes, this is a breaking change. If not, delete the remainder of this section.

@jmagman jmagman added tool Affects the "flutter" command-line tool. See also t: labels. t: xcode "xcodebuild" on iOS and general Xcode project management labels Apr 13, 2020
@jmagman jmagman self-assigned this Apr 13, 2020
@fluttergithubbot fluttergithubbot added d: examples Sample code and demos c: contributor-productivity Team-specific productivity, code health, technical debt. labels Apr 13, 2020
@jmagman
Copy link
Member Author

jmagman commented Apr 13, 2020

@tauu I would love your input on this solution since you diagnosed the original issue, but no pressure at all if you don't have time!

@tauu
Copy link
Contributor

tauu commented Apr 13, 2020

Sounds good :-) Just a small suggestion for an improvement. For the specific use case of adding a watchOS target, it would be convenient to access the settings FLUTTER_BUILD_NAME and
FLUTTER_BUILD_NUMBER. The version number of the watch target and the Runner target have to be synchronized and these settings would be handy. This would involve separating these settings (or all FLUTTER_* settings ?) from the rest. Though this might require quite a lot of work just for this use case.

christopherfujino
christopherfujino approved these changes Apr 13, 2020
View reviewed changes
Copy link
Contributor

@christopherfujino christopherfujino left a comment

Choose a reason for hiding this comment

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

LGTM

packages/flutter_tools/lib/src/ios/migrations/project_base_configuration_migration.dart Outdated
Copy link
Contributor

@christopherfujino christopherfujino Apr 13, 2020

Choose a reason for hiding this comment

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

nit: trailing comma

@jmagman
Copy link
Member Author

jmagman commented Apr 13, 2020

Sounds good :-) Just a small suggestion for an improvement. For the specific use case of adding a watchOS target, it would be convenient to access the settings FLUTTER_BUILD_NAME and
FLUTTER_BUILD_NUMBER. The version number of the watch target and the Runner target have to be synchronized and these settings would be handy. This would involve separating these settings (or all FLUTTER_* settings ?) from the rest. Though this might require quite a lot of work just for this use case.

We could set OTHER_LDFLAGS[sdk=iphone*] in the generated file so you have the option to inherit the build configuration in the watch target, if you want access to the other build settings. That would actually fix the original linker error in your PR, but I think this migration is still the right thing to do in case they have iOS targets that they don't want to link on Flutter.

@tauu
Copy link
Contributor

tauu commented Apr 14, 2020

We could set OTHER_LDFLAGS[sdk=iphone*] in the generated file so you have the option to inherit the build configuration in the watch target, if you want access to the other build settings.

That would be very helpful. I also agree that this migration is the right way to go. :-)

@jmagman jmagman mentioned this pull request Apr 14, 2020
Closed
@jmagman
Copy link
Member Author

jmagman commented Apr 14, 2020

We could set OTHER_LDFLAGS[sdk=iphone*] in the generated file so you have the option to inherit the build configuration in the watch target, if you want access to the other build settings.

That would be very helpful. I also agree that this migration is the right way to go. :-)

👍 Created #54769, will track separately.

@jmagman jmagman merged commit a6b39a2 into flutter:master Apr 14, 2020
@jmagman jmagman deleted the migrate-config branch April 14, 2020 20:01
This was referenced May 19, 2020
Closed
Closed
This was referenced Jun 15, 2020
Closed
Closed
@jmagman jmagman added the platform-ios iOS applications specifically label Aug 21, 2020
@tp tp mentioned this pull request Oct 11, 2020
Closed
@jmagman jmagman mentioned this pull request May 17, 2021
Closed
@jmagman jmagman mentioned this pull request Jun 24, 2021
Merged
11 tasks
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 21, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer

@christopherfujino christopherfujino christopherfujino approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@jmagman jmagman

Labels

c: contributor-productivity Team-specific productivity, code health, technical debt. d: examples Sample code and demos platform-ios iOS applications specifically t: xcode "xcodebuild" on iOS and general Xcode project management 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.

Adding non-iOS targets to Xcode project fails to build: Flutter not found

5 participants

@jmagman @tauu @christopherfujino @googlebot @fluttergithubbot