Skip to content
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

Small update will be needed for proposed cordova-android patch release #759

Closed
brodycj opened this issue Nov 15, 2018 · 10 comments
Closed

Comments

@brodycj
Copy link

brodycj commented Nov 15, 2018

Changes proposed in apache/cordova-android#555 were made so that the same IInAppBillingService.aidl entry in plugin.xml will work in cordova-android@6 and cordova-android@latest. This is achieved by remapping of target-dir if it does not start with "app". But this means that you will need to remove the second IInAppBillingService.aidl entry from plugin.xml and have your cordova-android@7 users upgrade to the upcoming cordova-android patch release.

Right now you can test with the following cordova-android versions:

  • https://github.com/brodybits/cordova-android#7.1.3-patch-updates
  • cordova-android@nightly (or https://github.com/apache/cordova-android#master)
  • cordova-android@6

Right now https://github.com/j3k0/cordova-plugin-purchase#v7.1.0 (version before you added the extra aidl entry and hook script) will install and build with the proposed patch release as well as cordova-android@6. So it should be possible for you to get rid of the hook script as well.

I hope this does not prove to be too much of a problem for you. If this does prove to be a major problem then I would be happy to look for another solution.

@brodycj
Copy link
Author

brodycj commented Nov 15, 2018

You can now test with cordova-android@nightly. I just updated the description.

@j3k0
Copy link
Owner

j3k0 commented Nov 15, 2018

Thanks @brodybits - I'll revert the .aidl related part.

You don't mention it, but there was also this section that required change in our plugin.xml file:

        <source-file src="src/android/billing_key_param.xml" target-dir="res/values/" />
        <config-file target="res/values/billing_key_param.xml" parent="/*">
            <string name="billing_key_param">$BILLING_KEY</string>
        </config-file>

To this :

        <hook type="before_plugin_install" src="scripts/androidBeforeInstall.js" />
        <config-file target="*/billing_key_param.xml" parent="/*">
            <string name="billing_key_param">$BILLING_KEY</string>
        </config-file>

Where androidBeforeInstall.js was creating the billing_key_param.xml file at the right location.

This should be covered by cordova-android@7.1.3 as well, right?

@brodycj
Copy link
Author

brodycj commented Nov 15, 2018

Good point. I would probably rewrite that section as follows:

        <source-file src="src/android/billing_key_param.xml" target-dir="res/values/" />
        <config-file target="*/billing_key_param.xml" parent="/*">
            <string name="billing_key_param">$BILLING_KEY</string>
        </config-file>

Old way to install billing_key_param.xml, more flexible config-file target, no hook script.

And be sure to test on nightly and upcoming patch release of cordova-android.

With nightly and upcoming patch release, the old source-file element should install billing_key_param.xml into app/src/main/res/values. The hook script seems to install the xml into the same place. And on cordova-android@6 it should work just like it did before.

TBH I have not dealt with the config-file element so much, looks like you know what you are doing with it.

I hope this makes sense, please don't hesitate to ask in case of any questions or confusion.

@brodycj
Copy link
Author

brodycj commented Nov 15, 2018

Sorry about this, I am having some second thoughts about the aidl change. Removing the second aidl entry from this plugin should be considered a breaking change since it would no longer work on cordova-android@7.1.1. And remapping of aidl on cordova-android should be considered a breaking change there since it causes the existing version of this plugin (v7.2.4) to install with an error.

I have no idea how many other plugins may be using multiple aidl entries, as needed to support both cordova-android@6 and cordova-android@7.

I think the right thing to do is to undo the aidl change on the proposed patch release and maybe master branch of cordova-android. More work on our side, but probably worth it if we can avoid more broken plugins in the near future.

@j3k0
Copy link
Owner

j3k0 commented Nov 15, 2018

As for not breaking existing plugins. Maybe ignore <source-file> entries that start with app/src/main/aidl/ if you're not on a Android Studio project?

More longer term, I wonder if a definite solution wouldn't be to support .aidl natively on cordova-android? Was it (or is it) already discussed somewhere?

Example <aidl-file> entry:

<aidl-file src="src/android/com/android/vending/billing/IInAppBillingService.aidl" target-dir="com/android/vending/billing"  />

cordova-android knows it has to prefix the target-dir with just src/ or app/src/main/aidl/ depending on the case, and plugin developers don't have to care where .aidl files needs to be installed (who knows when this will change again...)

@j3k0
Copy link
Owner

j3k0 commented Nov 15, 2018

Idea 2.

Remove the ^src/ and ^app/src/main/aidl/ part of all <source-file> entries, then add the correct prefix.

For plugins that have 2 entries, will it cause a problem if the aidl is installed twice at the same location?

@brodycj
Copy link
Author

brodycj commented Nov 15, 2018

For plugins that have 2 entries, will it cause a problem if the aidl is installed twice at the same location?

Yes that is the whole problem, unfortunately. It would be possible to use --force as a workaround, I don't really like it though.

@brodycj
Copy link
Author

brodycj commented Nov 16, 2018

As discussed apache/cordova-android#547 I think we do want to keep the .aidl change in the upcoming patch release, which means you would have to remove second .aidl entry. I think you should wait for us to publish the patch before making the change. My apologies for all the confusion here.

@j3k0
Copy link
Owner

j3k0 commented Nov 16, 2018

No problems, I understand the reasons for the confusion ;-) So I'll wait and the patch is released I ask users to stick to cordova-android@7.1.1. Good luck with this!

@brodycj
Copy link
Author

brodycj commented Nov 25, 2018

@j3k0 you have to get rid of the second .aidl entry for this plugin to work with recent cordova-android releases 7.1.3 and 7.1.3.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants