Skip to content

[all] fix for UserAgent.h compilation failures #2099

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.

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

Closed
wants to merge 26 commits into from

Conversation

collinjackson
Copy link
Contributor

@collinjackson collinjackson commented Mar 2, 2020

Right now the development cocoapod is named the same thing on iOS and MacOS. I think this may be why the podspec's prepare_command is not running consistently for both iOS and MacOS.

The attached change points both platforms at a pregenerated shared UserAgent.h, ensuring that it always exists. With this change, the version should always be set but we'll be able to detect if it's not by an "unknown" version showing up as the version on the server side.

Fixes #2087
Fixes #1858
Fixes #2084

@collinjackson collinjackson changed the title fix for UserAgent.h compilation failures [all] fix for UserAgent.h compilation failures Mar 2, 2020
@collinjackson
Copy link
Contributor Author

@franciscojma86 @jmagman while I think this fix will work for now, I'm wondering if Flutter plugins should be using a different pod name for the iOS and Mac development pods.

@cbenhagen cbenhagen mentioned this pull request Mar 2, 2020
13 tasks
@jmagman
Copy link
Contributor

jmagman commented Mar 2, 2020

#1858 (comment)

Those missing files should be coming from the firebase* podspec prepare_command which only get run when the pod is first downloaded. I don't know how this works when the pod isn't being downloaded via pod but instead via a symlink to the pub-cache...
I was able to reproduce this by adding this to a pubspec:

  firebase_auth: 
  firebase_storage: 
  firebase_core: 

Running flutter pub get then running:

flutter pub cache repair

which deleted the UserAgent.h files.

Essentially: s.prepare_command shouldn't be relied on in a podspec that is distributed via pub.

https://github.com/FirebaseExtended/flutterfire/blob/00c0c8707fcced86faec0d752b6c8bf43b3fcd7c/packages/cloud_firestore/cloud_firestore/ios/cloud_firestore.podspec#L28

Instead of generating a header file to define these macros instead you can use GCC_PREPROCESSOR_DEFINITIONS to define them (I didn't test this, should work in theory)

s.pod_target_xcconfig = { 'GCC_PREPROCESSOR_DEFINITIONS' => '$(inherited) LIBRARY_VERSION=unknown LIBRARY_NAME=flutter-fire-fst' }

See #1858 (comment).

@collinjackson collinjackson requested a review from jmagman March 2, 2020 19:31
@collinjackson
Copy link
Contributor Author

Thanks! I'll update the PR.

@googlebot
Copy link

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@googlebot googlebot added cla: no and removed cla: yes labels Mar 9, 2020
@franciscojma86 franciscojma86 requested a review from jmagman March 9, 2020 17:34
Copy link
Contributor

@jmagman jmagman left a comment

Choose a reason for hiding this comment

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

Why are the ipa tests failing?

@franciscojma86
Copy link
Contributor

Those are existing failures :( I believe @collinjackson is looking into those, but we don't think those are related to this PR.

Copy link
Contributor

@jmagman jmagman left a comment

Choose a reason for hiding this comment

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

This change LGTM, assuming @collinjackson is fine with this landing on build errors.
I don't know why @googlebot thinks there are commits from authors who haven't signed the cla...

@collinjackson
Copy link
Contributor Author

@jmagman @franciscojma86 Thank you both SO much for improving this PR -- this approach is simpler, more reliable, and easy to maintain.

@collinjackson
Copy link
Contributor Author

collinjackson commented Mar 10, 2020

I'm not sure if this will fix the Crashlytics CI failure but we should probably land a change similar to flutter/plugins#2592 to remove this file:

https://github.com/FirebaseExtended/flutterfire/blob/master/packages/firebase_crashlytics/example/ios/Runner.xcworkspace/xcshareddata/WorkspaceSettings.xcsettings

I'll open a PR for that now. Hopefully then we can land on green (modulo the CLA issue)

@danysz
Copy link

danysz commented Mar 13, 2020

Someone can please fix the CLA of google and merge this issue. Is a huge waiting time this UserAgent.h missing

@cbenhagen
Copy link
Contributor

@danysz while you are waiting you can use this branch by adding entries like the following to your pubspec.yaml for every package you need:

dependency_overrides:
  firebase_core:
    git:
      url: https://github.com/collinjackson/flutterfire
      path: packages/firebase_core/firebase_core
      ref: user_agent_mac

Or you can use a local git checkout where you merge the PRs you are waiting for.

@jmagman
Copy link
Contributor

jmagman commented Mar 13, 2020

This PR got messy with multiple usernames and a merge conflict, and it was missing the firebase_crashlytics/pubspec.yaml version bump. Closing in favor of #2169.

@jmagman
Copy link
Contributor

jmagman commented Mar 16, 2020

There are still UserAgent headers in firebase_remote_config, firebase_database, firebase_dynamic_links, firebase_messaging, firebase_ml_vision, firebase_analytics, firebase_storage, firebase_performance.

@collinjackson @franciscojma86 is there a follow up PR for them?

@danysz

This comment has been minimized.

@firebase firebase locked and limited conversation to collaborators Aug 2, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
7 participants