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

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

Closed
brodycj opened this issue Nov 12, 2018 · 19 comments
Labels

Comments

@brodycj
Copy link
Contributor

brodycj commented Nov 12, 2018

From #542 (comment) & #542 (comment):

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 #542 (comment):

Example file types to deal with here:

  • .jar - generally has target-dir="lib" in old plugins
  • .aar - also generally has target-dir="lib" in old plugins
  • .so - generally has a "lib" subdirectory in old plugins

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

@brodycj
Copy link
Contributor Author

brodycj commented Nov 12, 2018

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

@brodycj
Copy link
Contributor Author

brodycj commented Nov 12, 2018

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

  • cordova plugin add cordova-sqlite-storage@2.1.5
  • cordova platform add android@7.1.2
  • cordova build android

Then I get errors such as:

  • /Users/brodybits/Documents/cordova/cordova-sqlite-test-app/platforms/android/app/src/main/java/io/sqlc/SQLiteConnectorDatabase.java:29: error: package io.liteglue does not exist
  • /Users/brodybits/Documents/cordova/cordova-sqlite-test-app/platforms/android/app/src/main/java/io/sqlc/SQLiteConnectorDatabase.java:41: error: cannot find symbol

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):

  • cordova plugin add cordova-sqlite-storage@2.1.5
  • cordova platform add https://github.com/apache/cordova-android
  • cordova build android

@brodycj brodycj added the bug label Nov 12, 2018
brodycj pushed a commit to brodycj/cordova-android that referenced this issue Nov 12, 2018
brodycj pushed a commit to brodycj/cordova-android that referenced this issue Nov 12, 2018
brodycj pushed a commit to brodycj/cordova-android that referenced this issue Nov 13, 2018
brodycj pushed a commit to brodycj/cordova-android that referenced this issue Nov 13, 2018
@thibaud-sanchez
Copy link

thibaud-sanchez commented Nov 13, 2018

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.

@brodycj
Copy link
Contributor Author

brodycj commented Nov 13, 2018

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 apla/me.apla.cordova.app-preferences#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 apla/me.apla.cordova.app-preferences#142).

@brodycj
Copy link
Contributor Author

brodycj commented Nov 13, 2018

@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
Copy link

j3k0 commented Nov 13, 2018

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?

brodycj pushed a commit to brodycj/cordova-android that referenced this issue Nov 13, 2018
brodycj pushed a commit to brodycj/cordova-android that referenced this issue Nov 13, 2018
(source-file entries)

including aidl, aar, jar, and so files
brodycj pushed a commit to brodycj/cordova-android that referenced this issue Nov 13, 2018
(source-file entries)

including aidl, aar, jar, and so files
brodycj pushed a commit to brodycj/cordova-android that referenced this issue Nov 13, 2018
brodycj pushed a commit to brodycj/cordova-android that referenced this issue Nov 14, 2018
@brodycj brodycj mentioned this issue Nov 14, 2018
7 tasks
brodycj pushed a commit to brodycj/cordova-android that referenced this issue Nov 14, 2018
(source-file entries)

including aidl, aar, jar, and so files
brodycj pushed a commit to brodycj/cordova-android that referenced this issue Nov 14, 2018
(source-file entries)

including aidl, aar, jar, and so files
brodycj pushed a commit to brodycj/cordova-android that referenced this issue Nov 14, 2018
brodycj pushed a commit to brodycj/cordova-android that referenced this issue Nov 14, 2018
(source-file entries)

including aidl, aar, jar, and so files
brodycj pushed a commit that referenced this issue Nov 14, 2018
Fix for old plugins with non-Java sources (GH-547)
@nagthgr8

This comment has been minimized.

brodycj pushed a commit to brodycj/cordova-android that referenced this issue Nov 15, 2018
(source-file entries)

including aar, jar, and so files

NOTE: target-lib mapping for aidl is NOT included in 7.1.x
in order to avoid an already "exists" installation error on
https://github.com/j3k0/cordova-plugin-purchase#master
(v7.2.4) in a patch release of cordova-android.
@brodycj
Copy link
Contributor Author

brodycj commented Nov 15, 2018

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.

brodycj pushed a commit to brodycj/cordova-android that referenced this issue Nov 15, 2018
avoid breaking plugins such as cordova-plugin-purchase
brodycj pushed a commit to brodycj/cordova-android that referenced this issue Nov 15, 2018
apacheGH-547 update for aidl files was breaking installation of
cordova-plugin-purchase@latest (7.2.4)
@brodycj
Copy link
Contributor Author

brodycj commented Nov 15, 2018

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 j3k0/cordova-plugin-purchase#759, would rather discuss these ideas here or in another Cordova issue.

brodycj pushed a commit to brodycj/cordova-android that referenced this issue Nov 15, 2018
@brodycj
Copy link
Contributor Author

brodycj commented Nov 15, 2018

From j3k0/cordova-plugin-purchase#759 (comment):

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
Copy link

j3k0 commented Nov 15, 2018

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="..." />

@brodycj
Copy link
Contributor Author

brodycj commented Nov 15, 2018

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.

brodycj pushed a commit to brodycj/cordova-android that referenced this issue Nov 15, 2018
@jcesarmobile
Copy link
Member

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.

@brodycj
Copy link
Contributor Author

brodycj commented Nov 15, 2018

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
Copy link
Member

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
Copy link

ippeiukai commented Nov 16, 2018

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.

vaenow/cordova-plugin-app-update#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

@brodycj
Copy link
Contributor Author

brodycj commented Nov 16, 2018

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 apache/cordova#47 with the information and would like to continue discussion there.

brodycj pushed a commit to brodycj/cordova-android that referenced this issue Nov 16, 2018
(source-file entries)

including aidl, aar, jar, and so files
brodycj pushed a commit to brodycj/cordova-android that referenced this issue Nov 16, 2018
brodycj pushed a commit to brodycj/cordova-android that referenced this issue Nov 16, 2018
(source-file entries)

including aidl, aar, jar, and so files
brodycj pushed a commit to brodycj/cordova-android that referenced this issue Nov 16, 2018
brodycj pushed a commit to brodycj/cordova-android that referenced this issue Nov 16, 2018
(source-file entries)

including aidl, aar, jar, and so files
@brodycj brodycj changed the title Compatibility of old plugins with non-Java source-file entries Compatibility of old plugins with non-Java source-file entries (individual files) Nov 18, 2018
@brodycj
Copy link
Contributor Author

brodycj commented Nov 18, 2018

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.

@brodycj
Copy link
Contributor Author

brodycj commented Nov 21, 2018

Now fixed in the latest patch release

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

No branches or pull requests

6 participants