Skip to content

Conversation

@jmagman
Copy link
Member

@jmagman jmagman commented Jun 9, 2020

Description

Move Podfile logic into the tool so changes can be easily made without the user needing to migrate or regenerate their Podfile.

Related Issues

Fixes #45197
Will make #39659 easier

Tests

Add checks to plugin_lint_mac.

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 platform-ios iOS applications specifically tool Affects the "flutter" command-line tool. See also t: labels. labels Jun 9, 2020
@jmagman jmagman self-assigned this Jun 9, 2020
@fluttergithubbot fluttergithubbot added c: contributor-productivity Team-specific productivity, code health, technical debt. work in progress; do not review labels Jun 9, 2020
@jmagman jmagman marked this pull request as ready for review June 9, 2020 18:54
@fluttergithubbot
Copy link
Contributor

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

@jmagman
Copy link
Member Author

jmagman commented Jun 9, 2020

generated_key_values
def ios_application_path
ios_application_path = File.dirname(defined_in_file.realpath) if self.respond_to?(:defined_in_file)
ios_application_path ||= File.dirname(File.realpath(__FILE__))
Copy link
Member Author

Choose a reason for hiding this comment

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

Avoid __dir__, I used it in add-to-app version of this and people have had issues.

end
generated_key_values
def ios_application_path
ios_application_path = File.dirname(defined_in_file.realpath) if self.respond_to?(:defined_in_file)
Copy link
Member Author

Choose a reason for hiding this comment

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

Hopefully CocoaPods never removes defined_in_file (the path to the Podfile) but if they do, fallback to... this file.

Copy link
Contributor

Choose a reason for hiding this comment

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

should we log a warning if it doesn't repond to defined_in_file?

Copy link
Member Author

Choose a reason for hiding this comment

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

I was sensitive about this due to #42513 but we know the location of the Podfile--it's this file.
I think I'm over-engineering this a bit. I just need to pass in this file. We can keep the fallback to defined_in_file and subsequent raise in the podhelper.

# end
# @param [XCBuildConfiguration] build_configuration Build configuration for Pod target.
def flutter_additional_ios_build_settings(build_configuration)
build_configuration.build_settings['ENABLE_BITCODE'] = 'NO'
Copy link
Member Author

Choose a reason for hiding this comment

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

This gives us flexibility to support bitcode (someday).

end
end

def flutter_parse_plugins_file(file, separator='=')
Copy link
Member Author

@jmagman jmagman Jun 9, 2020

Choose a reason for hiding this comment

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

This logic will be updated to do #39659.

matches = line.match(/FLUTTER_ROOT\=(.*)/)
return matches[1].strip if matches
end
raise 'FLUTTER_ROOT not found in Generated.xcconfig. Try deleting Generated.xcconfig, then run flutter pub get'
Copy link
Contributor

Choose a reason for hiding this comment

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

nit. including generated_xcode_build_settings_path in the message would be helpful

Copy link
Member Author

Choose a reason for hiding this comment

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

Changed it to include the path

[!] Invalid `Podfile` file: /Users/m/Projects/test_ruby/ios/Flutter/Generated.xcconfig must exist. If you're running pod install manually, make sure flutter pub get is executed first.

and

[!] Invalid `Podfile` file: FLUTTER_ROOT not found in /Users/m/Projects/test_ruby/ios/Flutter/Generated.xcconfig. Try deleting Generated.xcconfig, then run flutter pub get.

end

# Plugin Pods
require File.expand_path(File.join('packages', 'flutter_tools', 'bin', 'podhelper'), flutter_root)
Copy link
Member

Choose a reason for hiding this comment

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

\o/

@@ -0,0 +1,126 @@
# Copyright 2014 The Flutter Authors. All rights reserved.
Copy link
Member

Choose a reason for hiding this comment

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

what's the transition plan for add-to-app? Can we change this line in https://flutter.dev/docs/development/add-to-app/ios/project-setup#option-a---embed-with-cocoapods-and-the-flutter-sdk to load File.join(ENV["FLUTTER_ROOT"], '.ios', 'Flutter', 'podhelper.rb') or some such?

Copy link
Member Author

@jmagman jmagman Jun 9, 2020

Choose a reason for hiding this comment

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

Currently the CocoaPods add-to-app module lays down a similar but not quite the same podhelper in the ephemeral directory, which gets recreated on a flutter clean. As long as we didn't rename those methods and break existing host Podfiles (unfortunately the names don't reference iOS or the module: install_all_flutter_pods, install_flutter_engine_pod, install_flutter_plugin_pods) we could transition them to this bin podhelper, but I don't know what it would really buy us (other than they would get updates instantly instead on a clean). I'm concerned it would make the CI story more complicated because flutter would need to be available on every machine, even if the entire module and podspecs and binaries were already placed in the right spot.

P.S. We wouldn't want them to need FLUTTER_ROOT on their path, it would have to be parsed from the Generated files the same way this one is.

Copy link
Member

Choose a reason for hiding this comment

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

ya, it should be a separate task.

What I had in mind actually may or may not work. One small ergonomic annoyance now is if someone checks out the iOS and flutter projects, it currently takes 3 steps to build again. Go to flutter, flutter pub get to make the .ios folder with podhelper.rb, then go to iOS, call pod install, then open xcode, run.

Perhaps we can save one step if the various flutter_install_* code called flutter pub get to make the underlying xcodeproj if needed etc?

Copy link
Member Author

Choose a reason for hiding this comment

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

That would be cool. Want to create a new issue?

# target 'Runner' do
# ...
# end
def flutter_ios_podfile_setup
Copy link
Member Author

Choose a reason for hiding this comment

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

Added this hook for future use (could have been used for the old install! 'cocoapods', :disable_input_output_paths => true, for example)

@jmagman jmagman added this to the 1.21 - July 2020 milestone Jun 9, 2020
# end
# end
# @param [PBXAggregateTarget] target Pod target.
def flutter_additional_ios_build_settings(target)
Copy link
Member Author

Choose a reason for hiding this comment

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

One more change to make more future proof: pass in the target instead of the build configuration. This will give us access to the target name so in the future we can just make changes to the Flutter-specific targets (like adding a Flutter linker flag).

@jmagman jmagman changed the title Move Podfile logic into tool Move iOS Podfile logic into tool Jun 10, 2020
@jmagman jmagman merged commit 319a61f into flutter:master Jun 10, 2020
@jmagman jmagman deleted the podhelper branch June 10, 2020 21:12
zljj0818 pushed a commit to zljj0818/flutter that referenced this pull request Jun 22, 2020
mingwandroid pushed a commit to mingwandroid/flutter that referenced this pull request Sep 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

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.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Move logic from user project's Podfile to a Ruby script in Flutter tools

5 participants