Skip to content

Conversation

@cbenhagen
Copy link
Contributor

@cbenhagen cbenhagen commented Dec 23, 2019

Description

Add macOS support.

Related Issues

#1653

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • If the pull request affects only one plugin, the PR title starts with the name of the plugin in brackets (e.g. [cloud_firestore])
  • My PR includes unit or integration tests for all changed/updated/fixed behaviors (See Contributor Guide).
  • All existing and new tests are passing.
  • I updated/added relevant documentation (doc comments with ///).
  • The analyzer (flutter analyze) does not report any problems on my PR.
  • I read and followed the Flutter Style Guide.
  • I updated pubspec.yaml with an appropriate new version according to the pub versioning philosophy.
  • I updated CHANGELOG.md to add a description of the change.
  • I signed the CLA.
  • I am willing to follow-up on review comments in a timely manner.

Breaking Change

Does your PR require plugin users to manually update their apps to accommodate your change?

  • Yes, this is a breaking change (please indicate a breaking change in CHANGELOG.md and increment major revision).
  • No, this is not a breaking change.

@collinjackson collinjackson self-requested a review January 8, 2020 21:26
Copy link
Contributor

@collinjackson collinjackson left a comment

Choose a reason for hiding this comment

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

Discussed with @amirh and @franciscojma86. I think it is OK to use symlinks to share implementation between iOS and MacOS (since they're the same). I've updated the PR to do this.

@cbenhagen
Copy link
Contributor Author

Thanks @collinjackson and @franciscojma86 for working on this. I guess I will leave this PR to you as you have write access to my branch. Let me know if you want me to work on this again.

@flowhorn
Copy link

I tried this package and get the following issue:
CompileC myPath/app/build/macos/Build/Intermediates.noindex/Pods.build/Debug/firebase_core.build/Objects-normal/x86_64/FLTFirebaseCorePlugin.o /Users/felixweuthen/Development/flutter/.pub-cache/git/flutterfire-9034c88d8797e6ea7f8e4a21460bd08871d35c4b/packages/firebase_core/firebase_core/darwin/Classes/FLTFirebaseCorePlugin.m normal x86_64 objective-c com.apple.compilers.llvm.clang.1_0.compiler
so that the mac_os app doesnt compile? Do you know why this happens?

@cbenhagen
Copy link
Contributor Author

@flowhorn I don't know. Do you have more output than this? Please provide the output of flutter doctor -v.

@collinjackson / @franciscojma86 What about testing? Should I write XCTests?

@flowhorn
Copy link

It's the lastest master, here is the output:

[✓] Flutter (Channel master, v1.15.4-pre.26, on Mac OS X 10.15.3 19D76, locale de-DE)
• Flutter version 1.15.4-pre.26 at /Users/username/Development/flutter
• Framework revision b9ecebf101 (45 minutes ago), 2020-02-12 10:58:02 -0800
• Engine revision e4f46f32f1
• Dart version 2.8.0 (build 2.8.0-dev.8.0 fd992e423e)

[✓] Android toolchain - develop for Android devices (Android SDK version 29.0.2)
• Android SDK at /Users/username/Library/Android/sdk
• Android NDK location not configured (optional; useful for native profiling
support)
• Platform android-29, build-tools 29.0.2
• Java binary at: /Applications/Android
Studio.app/Contents/jre/jdk/Contents/Home/bin/java
• Java version OpenJDK Runtime Environment (build
1.8.0_202-release-1483-b49-5587405)
• All Android licenses accepted.

[✓] Xcode - develop for iOS and macOS (Xcode 11.3.1)
• Xcode at /Applications/Xcode.app/Contents/Developer
• Xcode 11.3.1, Build version 11C504
• CocoaPods version 1.8.4

[✓] Chrome - develop for the web
• Chrome at /Applications/Google Chrome.app/Contents/MacOS/Google Chrome

[✓] Android Studio (version 3.5)
• Android Studio at /Applications/Android Studio.app/Contents
• Flutter plugin version 41.0.2
• Dart plugin version 191.8593
• Java version OpenJDK Runtime Environment (build
1.8.0_202-release-1483-b49-5587405)

[✓] VS Code (version 1.42.0)
• VS Code at /Applications/Visual Studio Code.app/Contents
• Flutter extension version 3.8.1

[✓] Connected device (3 available)
• macOS • macOS • darwin-x64 • Mac OS X 10.15.3 19D76
• Chrome • chrome • web-javascript • Google Chrome 79.0.3945.130
• Web Server • web-server • web-javascript • Flutter Tools

• No issues found!

Do I need to setup anything? What about the GoogleServices? I tried to use them from the ios side.

@collinjackson
Copy link
Contributor

collinjackson commented Feb 13, 2020

@collinjackson / @franciscojma86 What about testing? Should I write XCTests?

Per our discussion on #1707 the integration test is sufficient.

@cbenhagen
Copy link
Contributor Author

cbenhagen commented Feb 13, 2020

@collinjackson || @franciscojma86 do you know why this task thinks that there is no macOS desktop project configured? There is a example/macos directory and running ./script/incremental_build.sh build-examples --macos --no-ipa locally works fine. What did I forget?

@franciscojma86
Copy link
Contributor

I think that's because you need to include your .xcworkspace as well, but it's being gitignored. Sent #1996 to hopefully fix this and then your PR should work

@@ -0,0 +1,6 @@
# Flutter-related
Copy link
Contributor

Choose a reason for hiding this comment

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

You don't need a gitignore at this level. The root gitignore should be enough

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's what flutter create . did. Maybe it should be removed from there then.

Copy link
Contributor

Choose a reason for hiding this comment

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

Well flutter create doesn't know there's a root level .gitignore :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True :) But this one also ignores **/Flutter/ephemeral/ and maybe more which is missing from the root level one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should I add **/Flutter/ephemeral/ to the root .gitignore or do we leave it as it is?

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's leave it as is right now. I'll add it to the root ignore and remove from here if needed in another PR

@cbenhagen
Copy link
Contributor Author

cbenhagen commented Feb 13, 2020

@franciscojma86 the failing test seems unrelated. Maybe you can re-run it? As soon as this is merged and released I will update the other PRs so they pass their tests.

@collinjackson collinjackson self-requested a review February 13, 2020 20:34
@franciscojma86 franciscojma86 merged commit 7a312ce into firebase:master Feb 13, 2020
@franciscojma86
Copy link
Contributor

Thank you for your patience @cbenhagen ! Looking forward to reviewing the other PRs as well. 🎉

@cbenhagen
Copy link
Contributor Author

Thank you for the review! Do you release the package manually or will it be released automatically at some point?

@franciscojma86
Copy link
Contributor

Done!

@cbenhagen cbenhagen deleted the firebase_core_macos branch February 14, 2020 06:42
@firebase firebase locked and limited conversation to collaborators Aug 5, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants