Skip to content
This repository has been archived by the owner on Feb 22, 2023. It is now read-only.

[in_app_purchase] Add macOS support #5854

Closed
wants to merge 0 commits into from

Conversation

alihassan143
Copy link

@alihassan143 alihassan143 commented May 28, 2022

macos os support added also the example app also added

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I read and followed the relevant style guides and ran the auto-formatter. (Unlike the flutter/flutter repo, the flutter/plugins repo does use dart format.)
  • I signed the CLA.
  • The title of the PR starts with the name of the plugin surrounded by square brackets, e.g. [shared_preferences]
  • I listed at least one issue that this PR fixes in the description above.
  • I updated pubspec.yaml with an appropriate new version according to the pub versioning philosophy, or this PR is exempt from version changes.
  • I updated CHANGELOG.md to add a description of the change, following repository CHANGELOG style.
  • I updated/added relevant documentation (doc comments with ///).
  • I added new tests to check the change I am making, or this PR is test-exempt.
  • All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

@flutter-dashboard
Copy link

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 (don't just cc him here, he won't see it! He's on Discord!).

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

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

@stuartmorgan
Copy link
Contributor

Thanks for the submission! Since this currently doesn't pass any of the CI tests, and the checklist is missing critical elements like tests, I'm going to mark this as a draft. Please also read flutter/flutter#84391 (comment) as it looks like you are duplicating code, which isn't an approach we would land.

Once this PR is complete, please mark it as ready for review and we'll take a look. Thanks!

@stuartmorgan stuartmorgan marked this pull request as draft May 28, 2022 14:01
@alihassan143 alihassan143 marked this pull request as ready for review May 30, 2022 05:25
@stuartmorgan stuartmorgan changed the title Macos Support for in app purchase [in_app_purchase] Add macOS support May 30, 2022
@stuartmorgan stuartmorgan removed the request for review from blasten May 30, 2022 16:19
@stuartmorgan
Copy link
Contributor

This still has a number of high-level issues that need to be addressed before we can even start reviewing this:

  • As mentioned above:

    Please also read [in_app_purchase] support for macOS platform (same as iOS) flutter#84391 (comment) as it looks like you are duplicating code, which isn't an approach we would land.

    This doesn't appear to have been resolved; the diff is showing duplication of all of the files. They need to be symlinked from a shared location.

  • The plugin does not build. See failures in macos-build_all_plugins and macos-platform_tests. It needs to build and pass tests.

  • You have a lot of unrelated changes. You've made pubspec changes to packages with no changes, for reasons that aren't clear, and add a lot of files to the iOS and Android examples. All the changes that aren't related to this PR need to be reverted.

  • You need to follow the process described here so that CI can build the changes. You should also not be disabling publishing of in_app_purchase; there's no step in this process that would require that, and it disables important checks.

Some other issues as well:

  • This doesn't appear to have been run; see failures in the format task. The license check there is also failing; you'll need to add the license to all new files.

  • You've checked these boxes, but haven't actually done the steps. As I said in your previous PR, please don't check boxes when you haven't done the steps. We have this checklist for a reason; these steps are part of our development process.

@stuartmorgan stuartmorgan marked this pull request as draft May 30, 2022 16:37
@alihassan143
Copy link
Author

thank you for clearing the confusion i will change the things and resend for review again

@alihassan143
Copy link
Author

@stuartmorgan i am running tests for the macos error: "Runner" has entitlements that require signing with a development certificate. Enable development signing in the Signing & Capabilities editor. (in target 'Runner' from project 'Runner') showing this error but in test is it required to sign in with capabilities

@stuartmorgan
Copy link
Contributor

I'm not sure where you are seeing that error, but the errors in CI are from the build not being set up correctly in the first place. For instance:

Errno::ENOENT - No such file or directory @ rb_check_realpath_internal - /private/var/folders/2f/dq81klt17l3g6yg3_nf0t2gh0000gn/T/cirrus-ci-build/packages/in_app_purchase/in_app_purchase_storekit/example/macos/Flutter/ephemeral/.symlinks/plugins/in_app_purchase_storekit/macos/Classes/FIAObjectTranslator.h

@alihassan143
Copy link
Author

@stuartmorgan i tried the ci test for ios also but they showing the same error also

@stuartmorgan
Copy link
Contributor

I don't see that error anywhere in the CI output for iOS, so again I'm not sure where you are seeing this error.

@alihassan143
Copy link
Author

@stuartmorgan can you try to run again the ci test i am getting this error for ios also
in native tests

@stuartmorgan
Copy link
Contributor

@stuartmorgan can you try to run again the ci test i am getting this error for ios also in native tests

I'm not sure what you are asking for here; CI tests are run automatically every time you push changes. Please link to the specific line in the Cirrus logs that you are referring to, because I've looked at the logs and don't see the errors you are describing.

@alihassan143
Copy link
Author

I'm not sure what you are asking for here; CI tests are run automatically every time you push changes. Please link to the specific line in the Cirrus logs that you are referring to, because I've looked at the logs and don't see the errors you are describing.

you can run the test locally and i am running flutter ci test locally that mentioned in documentation

@alihassan143
Copy link
Author

@stuartmorgan can you try to run again the ci test i am getting this error for ios also in native tests

I'm not sure what you are asking for here; CI tests are run automatically every time you push changes. Please link to the specific line in the Cirrus logs that you are referring to, because I've looked at the logs and don't see the errors you are describing.

@stuartmorgan also in the automatic ci test getting the same error

@stuartmorgan
Copy link
Contributor

@stuartmorgan also in the automatic ci test getting the same error

Again, I'm not seeing that, so please provide the specific link (by opening the Cirrus log view, clicking the relevant line, and then copying the resulting address).

@alihassan143
Copy link
Author

@stuartmorgan For more information, see https://blog.cocoapods.org and the CHANGELOG for this version at https://github.com/CocoaPods/CocoaPods/releases/tag/1.11.3

  CDN: trunk Relative path: CocoaPods-version.yml exists! Returning local because checking is only performed in repo update
[!] CocoaPods could not find compatible versions for pod "in_app_purchase_storekit":
  In Podfile:
    in_app_purchase_storekit (from `Flutter/ephemeral/.symlinks/plugins/in_app_purchase_storekit/macos`)

Specs satisfying the `in_app_purchase_storekit (from `Flutter/ephemeral/.symlinks/plugins/in_app_purchase_storekit/macos`)` dependency were found, but they required a higher minimum deployment target.
can you help how to resolve this error we need macos 10.15 for most of the in app purchase apis

@stuartmorgan
Copy link
Contributor

That error is completely different than the error you quoted earlier and said that you were seeing in the CI. It's very hard to help when you are providing conflicting information.

You can resolve that error by changing the minimum deployment target of the targets in the Runner project and the Podfile.

@alihassan143
Copy link
Author

@stuartmorgan i changed the build setting and when i am running the examples in real device and all the things working perfectly but the ci is taking so much time in the building the application and stop the task after 1 hour

@stuartmorgan
Copy link
Contributor

A build timeout almost always means that pub get resolution is failing; there's an issue with pub get when running in CI where it times out instead of failing with output. I see you've done something strange with git references (instead of following our contributing guide's instructions for how to do multi-package changes). It's very likely that the problem is that you've made package resolution impossible with how you made that change.

@alihassan143
Copy link
Author

@stuartmorgan i am completely stuck can you give some time its totally takes your some time to resolve this and merged this pr

@stuartmorgan
Copy link
Contributor

https://github.com/flutter/flutter/wiki/Contributing-to-Plugins-and-Packages#changing-federated-plugins has the instructions for the process I was referring to in my last comment. You should follow the instructions in step 1 instead of using git references in pubspecs.

IVLIVS-III added a commit to IVLIVS-III/flutter_plugins_fork that referenced this pull request Sep 29, 2022
IVLIVS-III added a commit to IVLIVS-III/flutter_plugins_fork that referenced this pull request Sep 29, 2022
IVLIVS-III added a commit to IVLIVS-III/flutter_plugins_fork that referenced this pull request Sep 29, 2022
IVLIVS-III added a commit to IVLIVS-III/flutter_plugins_fork that referenced this pull request Oct 7, 2022
IVLIVS-III added a commit to IVLIVS-III/flutter_plugins_fork that referenced this pull request Oct 23, 2022
IVLIVS-III added a commit to IVLIVS-III/flutter_plugins_fork that referenced this pull request Oct 26, 2022
auto-submit bot pushed a commit that referenced this pull request Nov 30, 2022
* Initial commit of adding MacOS support.

Heavily inspired by #5854

* Updated version and changelog.

* Updated documentation.

* Added missing license comments.

* Fixed podspec lint issue.

* Moved native tests to a shared location.

* Decreased minimum macOS version from 10.15 to 10.11.

This seems wrong to me, since StoreKit is only properly supported on MacOS 10.15+.
However, now all existing tests should pass.

* Added RunnerTests target to macos example.

* Unified macOS capitalization.

* Deleted generated macOS icon assets.

* Removed empty function overrides.

* Enabled tests for macOS.

* Added OCMock to RunnerTests target.

* Adapted tests for macOS.

* Reverted relative path dependencies.

* Updated to a proper version bump.

* Make macOS no-op tests run only on iOS.

* Replace directory symlinks with file symlinks.

* Marked iOS-only API as iOS-only.

Previously they were no-ops on macOS.

* Re-worded doc-comment.

* Formatted code.
@Hixie
Copy link
Contributor

Hixie commented Dec 14, 2022

@alihassan143 Thanks for your contribution! Can you post an update describing the state of this PR and the related PRs? I'm just trying to understand the state of our review queue. Thanks!

@alihassan143
Copy link
Author

@Hixie @IVLIVS-III make changes in his pr that needed to be added now his pr is in final reviews my pr contains some error so i am closing this pr

mauricioluz pushed a commit to mauricioluz/plugins that referenced this pull request Jan 26, 2023
* Initial commit of adding MacOS support.

Heavily inspired by flutter#5854

* Updated version and changelog.

* Updated documentation.

* Added missing license comments.

* Fixed podspec lint issue.

* Moved native tests to a shared location.

* Decreased minimum macOS version from 10.15 to 10.11.

This seems wrong to me, since StoreKit is only properly supported on MacOS 10.15+.
However, now all existing tests should pass.

* Added RunnerTests target to macos example.

* Unified macOS capitalization.

* Deleted generated macOS icon assets.

* Removed empty function overrides.

* Enabled tests for macOS.

* Added OCMock to RunnerTests target.

* Adapted tests for macOS.

* Reverted relative path dependencies.

* Updated to a proper version bump.

* Make macOS no-op tests run only on iOS.

* Replace directory symlinks with file symlinks.

* Marked iOS-only API as iOS-only.

Previously they were no-ops on macOS.

* Re-worded doc-comment.

* Formatted code.
engine-flutter-autoroll pushed a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 22, 2023
* Initial commit of adding MacOS support.

Heavily inspired by flutter/plugins#5854

* Updated version and changelog.

* Updated documentation.

* Added missing license comments.

* Fixed podspec lint issue.

* Moved native tests to a shared location.

* Decreased minimum macOS version from 10.15 to 10.11.

This seems wrong to me, since StoreKit is only properly supported on MacOS 10.15+.
However, now all existing tests should pass.

* Added RunnerTests target to macos example.

* Unified macOS capitalization.

* Deleted generated macOS icon assets.

* Removed empty function overrides.

* Enabled tests for macOS.

* Added OCMock to RunnerTests target.

* Adapted tests for macOS.

* Reverted relative path dependencies.

* Updated to a proper version bump.

* Make macOS no-op tests run only on iOS.

* Replace directory symlinks with file symlinks.

* Marked iOS-only API as iOS-only.

Previously they were no-ops on macOS.

* Re-worded doc-comment.

* Formatted code.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
3 participants