Skip to content

Allow FLUTTER_APPLICATION_PATH to be null for misconfigured Xcode projects #57701

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
May 27, 2020
Merged

Allow FLUTTER_APPLICATION_PATH to be null for misconfigured Xcode projects #57701

jmagman merged 2 commits into from
May 27, 2020

Conversation

@jmagman
Copy link
Member

@jmagman jmagman commented May 20, 2020 •
edited
Loading

Description

Some users have misconfigured Xcode projects base build configurations and incorrectly swapped to Pods-Runner.debug.xcconfig instead of Debug.xcconfig. They were getting around this by following Stack Overflow-quality advice and manually adding a FLUTTER_ROOT build setting.

#51453 started calling the embed function in xcode_backend, which checks if FLUTTER_APPLICATION_PATH exists. That variable isn't set for these misconfigured users.

flutter/packages/flutter_tools/bin/xcode_backend.sh

Line 267 in 2a5690f

AssertExists "${FLUTTER_APPLICATION_PATH}"

As a workaround, copy the build check behavior that falls back to SOURCE_ROOT. Keep the same .ios and ios dance to make this as minimum a change as possible.

flutter/packages/flutter_tools/bin/xcode_backend.sh

Lines 42 to 45 in 2a5690f

local project_path="${SOURCE_ROOT}/.."
if [[ -n "$FLUTTER_APPLICATION_PATH" ]]; then
project_path="${FLUTTER_APPLICATION_PATH}"
fi

They still have misconfigured Xcode projects, but at least they won't be worse off than before #51453.

Related Issues

Fixes #56507
Would have been never been needed with #12749 or #12751.

Tests

I tested this by misconfiguring my base configuration to the Pod variants, and adding a FLUTTER_ROOT build setting. I get the The path does not exist before this change, and a successful build after. The existing integration tests will prove that the default and correctly configured flutter create project still works.

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 May 20, 2020
@jmagman jmagman self-assigned this May 20, 2020
dnfield
dnfield approved these changes May 20, 2020
View reviewed changes
Copy link
Contributor

@dnfield dnfield left a comment

Choose a reason for hiding this comment

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

This LGTM and seems to make perfect sense, is there some way we can test it?

@jmagman jmagman mentioned this pull request May 20, 2020
Closed
@jmagman
Copy link
Member Author

jmagman commented May 21, 2020

This LGTM and seems to make perfect sense, is there some way we can test it?

I can create a misconfigured integration test that tests this exact scenario. But, I hate adding them to the Flutter repo since it bloats everyone's install every so slightly each time. More importantly, there are infinite ways to mess up an Xcode project (and enough bad advice on SO to keep us busy forever) and we can't test them all.

Any of the below are better fixes than this, but I wanted something focused so it's hot-fixable.

  1. A version of Emit an actionable error message if FLUTTER_* not defined in Xcode run-script phases #12751 that checks for necessary FLUTTER_ variables in the build script. This wouldn't work for FLUTTER_ROOT since you need that to get to the script, but it would have worked for FLUTTER_APPLICATION_PATH or FLUTTER_BUILD_NUMBER or other ones that are expected.
  2. A version of Flutter iOS builds should verify that Generated.xcconfig is included in the configuration #12749 that checks the variables are present in the flutter tool instead of xcode_backend. Wouldn't work from Xcode, but sure would from flutter run. We could give them a nicer error message and instructions and a link to documentation (that doesn't yet exist).
  3. Remove FLUTTER_APPLICATION_PATH in this spot. I don't see why we need to go up to the flutter application to back down to the SRCROOT (like it could just be $(SRCROOT)/Flutter/Flutter.framework. I suspect it's to handle Flutter modules that have been made editable, but we want to deprecate that.
  4. Don't copy the frameworks around 10 times.

@dnfield
Copy link
Contributor

dnfield commented May 21, 2020

Could we just directly invoke xcode_backend.sh with some flag that tests this path?

@jmagman
Copy link
Member Author

jmagman commented May 26, 2020

Could we just directly invoke xcode_backend.sh with some flag that tests this path?

Calling it directly from an integration test.

@jmagman jmagman merged commit d9adfe3 into flutter:master May 27, 2020
@jmagman jmagman deleted the assert-present branch May 27, 2020 17:08
pcsosinski pushed a commit to pcsosinski/flutter that referenced this pull request May 28, 2020
@jmagman
Allow FLUTTER_APPLICATION_PATH to be null for misconfigured Xcode pro…
2a2fbfd 
…jects (flutter#57701)
pcsosinski pushed a commit that referenced this pull request May 28, 2020
@LongCatIsLooong @pedromassango
@christopherfujino @jason-simmons @stuartmorgan-g @jmagman
Flutter 1.17.2 cherrypicks (#58050)
5f21edf 
* fix segment hit test behavior (#57461)

* Making DropdownButtonFormField to re-render if parent widget changes (#57037)

* Update DropdownButtonFormField's state if widget updates

Co-authored-by: Shi-Hao Hong <[email protected]>

* throw more specific toolexit when git fails during upgrade (#57162)

* [flutter_tools] Refresh VM state before executing hot reload (#53960)

* Update engine hash for 1.17.2

* Remove MaterialControls from examples/flutter_view (#57621)

Co-authored-by: Jenn Magder <[email protected]>

* Prevent building non-android plugins in build aar (#58018)

* Allow FLUTTER_APPLICATION_PATH to be null for misconfigured Xcode projects (#57701)

* Don't import plugins that don't support android in settings.gradle (#54407)

Co-authored-by: LongCatIsLooong <[email protected]>
Co-authored-by: Pedro Massango <[email protected]>
Co-authored-by: Shi-Hao Hong <[email protected]>
Co-authored-by: Christopher Fujino <[email protected]>
Co-authored-by: Jason Simmons <[email protected]>
Co-authored-by: stuartmorgan <[email protected]>
Co-authored-by: Jenn Magder <[email protected]>
Co-authored-by: Emmanuel Garcia <[email protected]>
@jmagman jmagman added the platform-ios iOS applications specifically label Aug 21, 2020
@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

@christopherfujino christopherfujino Awaiting requested review from christopherfujino

1 more reviewer

@dnfield dnfield dnfield approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@jmagman jmagman

Labels

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.

"Path does not exist" building for iOS [Crash report]

3 participants

@jmagman @dnfield @googlebot