search

apache / cordova-android

Apache Cordova Android
https://cordova.apache.org/
Apache License 2.0
3.59k stars 1.52k forks source link

Compatibility of old plugins with non-Java source-file entries (individual files) #547

Closed brodybits closed 5 years ago

brodybits commented 5 years ago

From https://github.com/apache/cordova-android/pull/542#issuecomment-437709608 & https://github.com/apache/cordova-android/pull/542#issuecomment-437922322:

Some old plugins with non-Java source-file entries still do not seem to work since version 7.0.0. Some examples, including examples from https://github.com/apache/cordova-android/pull/542#issuecomment-437922322:

Example file types to deal with here:

Purpose is that a massive number of plugins should be able to just work on both old and new Android project structure.

brodybits commented 5 years ago

P.S. This may affect https://github.com/apache/cordova-docs/issues/902: Document how to use Android NDK libraries in plugins

brodybits commented 5 years ago

Reproduction steps in https://github.com/brodybits/cordova-sqlite-test-app:

Then I get errors such as:

It worked when I did cordova plugin add cordova-sqlite-storage@2.1.5 and cordova platform add android@6.

Note that newer versions of cordova-sqlite-storage do not show this issue. My solution was to use lib-file with JARs. But countless other plugins are likely affected by this issue.

P.S. The following reproduction steps lead to the same result on the master branch of cordova-android (this project):

thibaud-sanchez commented 5 years ago

Seems to have a similar issue with plugin cordova-plugin-app-preferences

Got the following error when I make a cordova prepare after adding plugin :

PS E:> cordova prepare Android Studio project detected will push strings array {"name":"lang","titles":["English (US)","English (UK)"],"values":["en-us","en-gb"]} unhandled exception { [Error: ENOENT: no such file or directory, mkdir 'E:...\platforms\android\res\xml'] errno: -4058, code: 'ENOENT', syscall: 'mkdir', path: 'E:\...\platforms\android\res\xml' } ENOENT: no such file or directory, mkdir 'E:...\platforms\android\res\xml'

I've rollback to Android 6.4.0 and it works.

brodybits commented 5 years ago

Seems to have a similar issue with plugin cordova-plugin-app-preferences

This is due to their use of hook scripts and I just raised https://github.com/apla/me.apla.cordova.app-preferences/issues/142. I would like to keep it outside the scope of this issue (please feel free to raise a new issue and link it to https://github.com/apla/me.apla.cordova.app-preferences/issues/142).

brodybits commented 5 years ago

@j3k0 I was able to get https://github.com/j3k0/cordova-plugin-purchase#v7.1.0 (version before you started making adaptations and workarounds for cordova-android@7) to build with a few changes to the cordova-android JavaScript. I will add a unit test and raise this in a PR later today. I added your plugin to the OP description, now closing #546 as a duplicate.

j3k0 commented 5 years ago

I was able to get https://github.com/j3k0/cordova-plugin-purchase#v7.1.0 to build

Great! I'll prepare a new version of the plugin without the installation hook.

BTW, you might have noticed that we install the .aidl file in two different locations, this was necessary to have the build succeed on all systems.

<!-- In-app Billing Library -->
<source-file src="src/android/com/android/vending/billing/IInAppBillingService.aidl" target-dir="src/com/android/vending/billing" />
<source-file src="src/android/com/android/vending/billing/IInAppBillingService.aidl" target-dir="app/src/main/aidl/com/android/vending/billing" />

Is there any better way to do this?

nagthgr8 commented 5 years ago

Guys, can anyone help me understand why does build succeed on such breaking changes, the runtime failures are more disastrous! my app stopped taking payments since few days with the new cordova update

brodybits commented 5 years ago

Payments plugin is not supported here. For payments problem please raise a new issue on the payments plugin and explain why it is not a duplicate.

brodybits commented 5 years ago

Updates as proposed in PR #550 are now on the master branch. But I discovered that if I try adding https://github.com/j3k0/cordova-plugin-purchase#master (v7.2.4) with these updates then it fails due to a duplicated .aidl file. This is a breaking change which is not wanted in patch release and not desired in the near future. The legacy target-dir mapping for .aidl files will be removed in an upcoming PR.

I had an interesting discussion with @j3k0, with some good ideas in https://github.com/j3k0/cordova-plugin-purchase/issues/759, would rather discuss these ideas here or in another Cordova issue.

brodybits commented 5 years ago

From https://github.com/j3k0/cordova-plugin-purchase/issues/759#issuecomment-439008473:

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

cordova-android@7 project structure seems to be based on Android Studio project structure now, don't see how this could make a difference. Any other input would be appreciated.

More longer term, I wonder if a definite solution wouldn't be to support .aidl natively on cordova-android?

+1

Example <aidl-file> entry: [...]

Cordova seems to use more portable tags such as header-file, source-file, lib-file, framework, etc. Maybe we could add something like an interface tag or idl tag.

j3k0 commented 5 years ago

When I see this proposed patch:

    } else if (obj.src.endsWith('.java')) {
        return path.join(APP_MAIN_PREFIX, 'java', obj.targetDir.substring(4), path.basename(obj.src));
    } else if (obj.src.endsWith('.aidl')) {
        return path.join(APP_MAIN_PREFIX, 'aidl', obj.targetDir.substring(4), path.basename(obj.src));
    } else if (obj.targetDir.includes('libs')) {
        if (obj.src.endsWith('.so')) {
            return path.join(APP_MAIN_PREFIX, 'jniLibs', obj.targetDir.substring(5), path.basename(obj.src));
        } else {
            return path.join('app', obj.targetDir, path.basename(obj.src));
        }

I wonder if the "fileType" part of path.join calls shouldn't be an extra attribute of <source-file>. It could default to this kind of heuristic, but allow more flexibility.

Example:

<source-file src="path/libfoo.so" type="jniLibs" />
<source-file src="path/IFoo.aidl" type="aidl" target-dir="..." />
<source-file src="path/Foo.bar" type="barFiles" target-dir="..." />
brodybits commented 5 years ago

I wonder if the "fileType" part of path.join calls shouldn't be an extra attribute of <source-file>.

I think a much better solution is that we move away from using source-file elements for .aidl, library, and framework files. For a followup issue.

jcesarmobile commented 5 years ago

The duplicate problem is not just for the .aidl, it will happen on any extension if the plugin maintainer added different source-file tags for the same file but different paths for different cordova-android versions. I don't think we should fix this, we should keep with the remapping, and ask plugin authors to update the plugins, as this will only happen on plugins that got updated, not for old unmaintained plugins.

Other than that, all we can do is copy the file even if it exists instead of failing, but that could be even a bigger breaking change.

brodybits commented 5 years ago

Interesting. So some remapping was introduced in 7.0.0, and some modified remapping was introduced in master, as was merged from #550.

We know that the changes to the project structure and file remapping from 7.0.0 could be considered breaking changes, which evidently broke some plugins. This is valid from according to semver rules, but I did not see much in the way of notifying the community that plugins would be broken, testing of workaround solutions, etc.

From this discussion I have a major concern that the modified remapping as introduced in master may prove to be a breaking change, which should definitely not be part of a patch release and also should not be part of a minor release.

I think the modified remapping in master could really benefit the user community. I am now starting to wonder if we should consider proposing a major release, with the modified remapping, in the near future?

jcesarmobile commented 5 years ago

The problem is people is using source-file for everything, while they should be using resource-file and lib-file in most cases.

source-file should only be used for .java files, so that was the only case taken into account resource-file and lib-file also got the proper remapping

but as the use of source-file for non source files seems to be massive we had to do this patches to handle other file types different from .java.

So trying to fix this is definitely a patch, not a breaking change, the purpose of the fix is to make old non updated plugins compatible. If it will break plugins that got "fixed" with some workaround, they will have to fix it again.

ippeiukai commented 5 years ago

I've just submitted a fix to another plugin. This one is a bit different because this behaviour of source-file is not documented, but it was working with cordova-android@6 but no longer with cordova-android@7.

https://github.com/vaenow/cordova-plugin-app-update/pull/119 (Specifying a directory with many .java files as source-file src no longer works.)

FYI: though not documented officially, it is a known behaviour: https://stackoverflow.com/q/28042385

brodybits commented 5 years ago

So trying to fix this is definitely a patch

@jcesarmobile I see your point now. I will update PR #555 for the patch release.

[...] behaviour of source-file is not documented, [...] it was working with cordova-android@6 but no longer with cordova-android@7.

@ippeiukai thanks for reporting, I just raised https://github.com/apache/cordova/issues/47 with the information and would like to continue discussion there.

brodybits commented 5 years ago

Closing as this issue is resolved in both master and 7.1.x branches. 7.1.3 patch release with this issue resolved should be available this week.

brodybits commented 5 years ago

Now fixed in the latest patch release