search

apache / cordova-android

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

CB-11244: Studio Project Compatibility: Now with merge commit #389

Closed infil00p closed 6 years ago

infil00p commented 6 years ago

Platforms affected

Android

What does this PR do?

This PR is the new current working PR for the Studio Project work

What testing has been done on this change?

Tested against the CI tests, more tests need to be added.

Checklist

filmaj commented 6 years ago

Heh, with me merging in #391, you might need to rebase πŸ™ˆ

At least you'll get wider test feedback on Windows with it.

infil00p commented 6 years ago

@filmaj Github says otherwise.

filmaj commented 6 years ago

Just based on the last travis/appveyor run in the commits in this PR, the unit tests, end to end tests and the native unit tests are not running.

I don't see master HEAD in the commit list for this branch, either:

* a7304b9a - (HEAD -> StudioProjectCompat, infil00p/StudioProjectCompat) Finishing the linting (2 weeks ago) <Joe Bowser>
*   e456175a - Merge branch 'master' into StudioProjectCompat (2 weeks ago) <Joe Bowser>
|\
| * 55d7cf38 - CB-12895 : updated .eslintrc file in spec dir and set jasmine true and removed root is true (3 weeks ago) <Audrey So>
| * ac4ac935 - CB-12895 : added .eslintrc files to set up jasmine environment (3 weeks ago) <Audrey So>
| * d83d49d8 - CB-12895 : fixed eslint errors (3 weeks ago) <Audrey So>
| * e36158a0 - CB-12895 : added eslint and removed jshint (3 weeks ago) <Audrey So>
| * 5cc14b80 - CB-12605 In Windows get Android studio path from the registry (3 weeks ago) <Jesse MacFadyen>
* | b20028c4 - (infil00p/StudioProjectCleanup, StudioProjectCleanup) The prepare step was broken, which breaks the CLI workflow.  This was caused by hardcoding the Java directory, which is a very bad idea. (3 weeks ago) <Joe Bowser>
* | 1cda7a9d - CB-11244: Found bug in Api.js where xml/strings.xml is used instead of values/strings.xml (3 weeks ago) <Joe Bowser>
* | 49b76f5c - Fixing mangled commits that crept into this branch (4 weeks ago) <Joe Bowser>
* | c0474e81 - Bump for travis test (4 weeks ago) <Joe Bowser>
* | 40c97094 - OK, Going back to the old build.gradle for legacy projects (4 weeks ago) <Joe Bowser>
* | b67e9905 - This is probably a bad idea, but we need to split the gradle files into legacy and new style (4 weeks ago) <Joe Bowser>
* | c74192d5 - Adding conditional code into Gradle, this is a bit dirty since we can't explicitly test it but we'll just have to rely on jasmine (4 weeks ago) <Joe Bowser>
* | 33feb00e - Adding the if statement to see if we can support both structures with minimal editing, TODO: actually write tests for this somehow (4 weeks ago) <Joe Bowser>
* | 8f16df4c - Adding logic to upgrade both Classic and Android Studio style project structures (4 weeks ago) <Joe Bowser>
* | fb6cb51e - Fixing lint errors (4 weeks ago) <Joe Bowser>
* | 28ebbb8f - CB-11244: Setup Api.js to support multiple builders based on project structure (4 weeks ago) <Joe Bowser>
* | bd4ddcde - Updated AndroidStudio to only look for the app directory to determine studio status (4 weeks ago) <Joe Bowser>
* | e621edfb - Fixing the Android Studio detection and making it automatically pick the right builder, good for upgrading Cordova (4 weeks ago) <Joe Bowser>
* | 304a8991 - Fixed the specification of the builders in the run command by getting build to check what was being passed from run (4 weeks ago) <Joe Bowser>
* | 8391af2e - JsHint Fixes, deleting unused methods (4 weeks ago) <Joe Bowser>
* | 69ab6a0e - Changing the project to add the app directory as a dependency (4 weeks ago) <Joe Bowser>
* | a216f0db - CB-11244: Changing directory creation, will most likely hide this behind a flag for the next release of Cordova-Android, and then make it default in the next major pending feedback (4 weeks ago) <Joe Bowser>
* | 69260fb9 - Fix the overwriting of Fil's fix, blargh (4 weeks ago) <Joe Bowser>
* | db87e0ae - Made changes so cordova/build builds with the new project.  Need to work on plugin installation. (4 weeks ago) <Joe Bowser>
* | 8ead919f - Managed to get the project to mostly compile, still need to re-work the build command to add the app project (4 weeks ago) <Joe Bowser>
* | b73c04f3 - Updating gradle version in the build file (4 weeks ago) <Joe Bowser>
* | f790aeb8 - Setting up the create command so we actually have all the directories in the right place, and define default variables in the top level build.gradle (4 weeks ago) <Joe Bowser>
* | 7b17abc5 - Fixing linting issues (4 weeks ago) <Joe Bowser>
* | ffadf5dd - Changing this so we pass lint (4 weeks ago) <Joe Bowser>
* | 23d8d999 - Moving Android Manifest finding to the Gradle and Studio builders. (4 weeks ago) <Joe Bowser>
* | d88df59c - Adding the Studio Builder to build a project based on Android Studio, and deleting Ant, since Google does not support Ant Builds anymore. Sorry guys! (4 weeks ago) <Joe Bowser>
|/
* 3a6e898b - CB-12762 : pointed package.json repo items to github mirrors instead of apache repos site (5 weeks ago) <Audrey So>
infil00p commented 6 years ago

The merge commit that I did was e456175a

filmaj commented 6 years ago

Right and that is now 3 commits behind master's HEAD :P

Just saying latest master stuff will get you better testing / feedback via CI in this PR!

infil00p commented 6 years ago

@filmaj So, I should do another merge commit just to get more tests?

filmaj commented 6 years ago

Maybe? I don't know. Just beware that you have very few tests running in this PR (basically just the JS unit tests, no JS e2e nor Java native unit tests) so the green checkmark next to your PR is a bit of a false positive.

infil00p commented 6 years ago

@filmaj Given how old this branch is, those are the same tests that master was running until three commits ago.

infil00p commented 6 years ago

@filmaj Hard copied paths? SRSLY?

filmaj commented 6 years ago

Yes, seriously, because the code interpolates activity name / package name / etc. into hard-coded paths!

codecov-io commented 6 years ago

Codecov Report

Merging #389 into master will decrease coverage by 0.35%. The diff coverage is 37.62%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #389      +/-   ##
==========================================
- Coverage   43.77%   43.42%   -0.36%     
==========================================
  Files          17       17              
  Lines        1688     1734      +46     
  Branches      306      320      +14     
==========================================
+ Hits          739      753      +14     
- Misses        949      981      +32
Impacted Files Coverage Ξ”
bin/templates/cordova/lib/builders/builders.js 37.5% <ΓΈ> (ΓΈ) :arrow_up:
...n/templates/cordova/lib/builders/GenericBuilder.js 25.86% <ΓΈ> (-1.64%) :arrow_down:
bin/templates/cordova/lib/device.js 22.44% <0%> (ΓΈ) :arrow_up:
bin/templates/cordova/lib/build.js 13.04% <0%> (-0.39%) :arrow_down:
bin/templates/cordova/lib/emulator.js 48.44% <0%> (-0.58%) :arrow_down:
bin/templates/cordova/lib/AndroidStudio.js 94.73% <100%> (ΓΈ) :arrow_up:
...in/templates/cordova/lib/builders/GradleBuilder.js 20.25% <15.78%> (-0.47%) :arrow_down:
bin/templates/cordova/Api.js 41.88% <16.66%> (-2.57%) :arrow_down:
bin/lib/create.js 49.26% <45.45%> (-1.57%) :arrow_down:
bin/templates/cordova/lib/pluginHandlers.js 86.47% <84.61%> (-0.41%) :arrow_down:

Continue to review full report at Codecov.

Legend - Click here to learn more Ξ” = absolute <relative> (impact), ΓΈ = not affected, ? = missing data Powered by Codecov. Last update 3ad1ed7...c6cfeb1. Read the comment docs.

filmaj commented 6 years ago

Hey @infil00p, latest master has a tiny tweak to the appveyor build file that works around some troubles appveyor is experiencing with one of their VM images. I think if you rebase/merge master into this, you'll find that'll clear up the appveyor failures.

dpogue commented 6 years ago

So I ran this branch with the following:

cordova create androidTest
cd ./androidTest
cordova platform add git://github.com/infil00p/cordova-android.git#StudioProjectCompat

In my platforms/android I ended up with both an app folder and a src folder, and I have a MainActivity.java in both. Is that expected?

android
β”œβ”€β”€ app
β”‚Β Β  └── src
β”‚Β Β      └── main
β”‚Β Β          └── java
β”‚Β Β           Β Β  └── io
β”‚Β Β           Β Β   Β Β  └── cordova
β”‚Β Β           Β Β   Β Β      └── hellocordova
β”‚Β Β           Β Β   Β Β          └── MainActivity.java
└── src
    └── io
        └── cordova
            └── hellocordova
                └── MainActivity.java
infil00p commented 6 years ago

@dpogue No, that's definitely a bug. Only the app directory should exist, and it's clear that something else is copying over MainActivity.

filmaj commented 6 years ago

https://github.com/infil00p/cordova-android/blob/StudioProjectCompat/bin/templates/cordova/lib/prepare.js#L214-L216

infil00p commented 6 years ago

@dpogue @filmaj That should fix the CLI errors, but I think there's some CLI stuff that I'm missing, since I don't use the CLI. Want to take a quick look again?

dpogue commented 6 years ago

The generated project looks good now.

I did run into one issue with plugins that I tried to install. I installed the cordova-plugin-media-capture and it didn't throw any errors, but it also didn't add any permissions to AndroidManifest: https://github.com/apache/cordova-plugin-media-capture/blob/master/plugin.xml#L77-L82

Resulting ./app/src/main/AndroidManifest.xml:

<?xml version='1.0' encoding='utf-8'?>
<manifest android:hardwareAccelerated="true" android:versionCode="10000" android:versionName="1.0.0" package="io.cordova.hellocordova" xmlns:android="http://schemas.android.com/apk/res/android">
    <supports-screens android:anyDensity="true" android:largeScreens="true" android:normalScreens="true" android:resizeable="true" android:smallScreens="true" android:xlargeScreens="true" />
    <uses-permission android:name="android.permission.INTERNET" />
    <application android:hardwareAccelerated="true" android:icon="@mipmap/icon" android:label="@string/app_name" android:supportsRtl="true">
        <activity android:configChanges="orientation|keyboardHidden|keyboard|screenSize|locale" android:label="@string/activity_name" android:launchMode="singleTop" android:name="MainActivity" android:theme="@android:style/Theme.DeviceDefault.NoActionBar" android:windowSoftInputMode="adjustResize">
            <intent-filter android:label="@string/launcher_name">
                <action android:name="android.intent.action.MAIN" />
                <category android:name="android.intent.category.LAUNCHER" />
            </intent-filter>
        </activity>
    </application>
    <uses-sdk android:minSdkVersion="16" android:targetSdkVersion="25" />
</manifest>

I had other issues in one of my own plugins, where it threw an error about being unable to insert: https://github.com/AyogoHealth/cordova-plugin-push/blob/swift3.0/plugin.xml#L60-L69

Installing "cordova-plugin-ayogo-push" for android
Android Studio project detected
Failed to install 'cordova-plugin-ayogo-push': Error: Unable to graft xml at selector "/manifest/application" from "/Users/dpogue/androidTest/platforms/android/app/src/main/res/xml/config.xml" during config install
    at ConfigFile_graft_child [as graft_child] (/Users/dpogue/androidTest/platforms/android/cordova/node_modules/cordova-common/src/ConfigChanges/ConfigFile.js:120:19)
    at PlatformMunger_apply_file_munge [as apply_file_munge] (/Users/dpogue/androidTest/platforms/android/cordova/node_modules/cordova-common/src/ConfigChanges/ConfigChanges.js:83:34)
    at munge_helper (/Users/dpogue/androidTest/platforms/android/cordova/node_modules/cordova-common/src/ConfigChanges/ConfigChanges.js:238:14)
    at PlatformMunger.add_plugin_changes (/Users/dpogue/androidTest/platforms/android/cordova/node_modules/cordova-common/src/ConfigChanges/ConfigChanges.js:164:12)
    at /Users/dpogue/androidTest/platforms/android/cordova/node_modules/cordova-common/src/PluginManager.js:126:25
    at _fulfilled (/Users/dpogue/androidTest/platforms/android/cordova/node_modules/q/q.js:854:54)
    at self.promiseDispatch.done (/Users/dpogue/androidTest/platforms/android/cordova/node_modules/q/q.js:883:30)
    at Promise.promise.promiseDispatch (/Users/dpogue/androidTest/platforms/android/cordova/node_modules/q/q.js:816:13)
    at /Users/dpogue/androidTest/platforms/android/cordova/node_modules/q/q.js:877:14
    at runSingle (/Users/dpogue/androidTest/platforms/android/cordova/node_modules/q/q.js:137:13)
Error: Unable to graft xml at selector "/manifest/application" from "/Users/dpogue/androidTest/platforms/android/app/src/main/res/xml/config.xml" during config install

Was there intended to be some backward-compatible path aliasing or is this a known breaking change?

infil00p commented 6 years ago

@dpogue That's the exact issue I suspected was in prepare.js. Unfortunately all the CLI magic is done in prepare.js, and I'm having a very hard time figuring out where the code that determines where AndroidManifest.xml is supposed to be is, since it's clearly pointing at the wrong AndroidManifest.xml.

dpogue commented 6 years ago

I haven't dug into it particularly far, but a quick skim through the config munging stuff suggests that maybe a workaround could go here: https://github.com/apache/cordova-common/blob/master/src/ConfigChanges/ConfigKeeper.js#L40-L44

filmaj commented 6 years ago

Thanks for the repro steps there @dpogue.

I think after the long weekend we'll take a look at cordova-common and make sure it can support the two differing project structures in Android. Looks like we already have that in place for config.xml - we will probably need something similar for AndroidManifest.xml.

One thing at a time, moving closer!

janpio commented 6 years ago

Just a shoutout from the sidelines: Thanks for tackling this! ❀️ ❀️ ❀️
Working with the current generated Android projects turns out to be a lot harder than I expected, and that's how I found this PR. Each step that gets the Cordova Android "output" closer to a standard Android project is great.

infil00p commented 6 years ago

@dpogue Good find, but I can't find the aliasing that we were doing to maintain some backwards compatibility. We will be doing a breaking change eventually, but we're not torching everything just yet.

infil00p commented 6 years ago

This now depends on this: https://github.com/apache/cordova-common/pull/7

infil00p commented 6 years ago

@dpogue @filmaj This should be ready to review once the cordova-common code change is added.

filmaj commented 6 years ago

Sounds like this also depends on apache/cordova-common#8

stevengill commented 6 years ago

Common PRs have been merged.

When testing this locally, you have to go into package.json of cordova-android and change the version of cordova-common in your dependency object to point to your local version of cordova-common. Something like:

    "cordova-common": "../cordova-common",

This is because when you add android as a platform, it copies cordova-common over

infil00p commented 6 years ago

@stevengill Doesn't npm linking cordova-common do the same thing?

stevengill commented 6 years ago

I managed to npm link common into android but then when i added my local android to a cordova project, the way it was being added and copied made it so it wasn't using my linked common.

I think this is because when we add platforms to projects, we run npm install on them. So it would fetch the version that was listed in package.json instead of using the linked common i had.

dpogue commented 6 years ago

I ended up pointing cordova-android at the git master for cordova-common, but it all appears to be working now.

I see the permissions getting merged into the AndroidManifest as intended :+1:

infil00p commented 6 years ago

Bad news: There's issues with the installation of the File Plugin, and cordova clean doesn't work properly, so that's a major problem. I've created subtasks on the JIRA issue to track that work, since we've just been lumping everything on this one currently.

https://issues.apache.org/jira/browse/CB-11244

macdonst commented 6 years ago

@infil00p I've run the following commands:

cordova create androidTest
cd ./androidTest
cordova platform add git://github.com/infil00p/cordova-android.git#StudioProjectCompat
cordova plugin add phonegap-plugin-push@2.0.0

which resulted in the following error:

Installing "phonegap-plugin-push" for android
Android Studio project detected
Failed to install 'phonegap-plugin-push': Error: Unable to graft xml at selector "/manifest" from "/Users/smacdona/code/androidTest/platforms/android/app/src/main/res/xml/config.xml" during config install
    at ConfigFile_graft_child [as graft_child] (/Users/smacdona/code/androidTest/platforms/android/cordova/node_modules/cordova-common/src/ConfigChanges/ConfigFile.js:122:19)
    at PlatformMunger_apply_file_munge [as apply_file_munge] (/Users/smacdona/code/androidTest/platforms/android/cordova/node_modules/cordova-common/src/ConfigChanges/ConfigChanges.js:81:34)
    at munge_helper (/Users/smacdona/code/androidTest/platforms/android/cordova/node_modules/cordova-common/src/ConfigChanges/ConfigChanges.js:236:14)
    at PlatformMunger.add_plugin_changes (/Users/smacdona/code/androidTest/platforms/android/cordova/node_modules/cordova-common/src/ConfigChanges/ConfigChanges.js:157:12)
    at /Users/smacdona/code/androidTest/platforms/android/cordova/node_modules/cordova-common/src/PluginManager.js:123:29
    at _fulfilled (/Users/smacdona/code/androidTest/platforms/android/cordova/node_modules/q/q.js:854:54)
    at self.promiseDispatch.done (/Users/smacdona/code/androidTest/platforms/android/cordova/node_modules/q/q.js:883:30)
    at Promise.promise.promiseDispatch (/Users/smacdona/code/androidTest/platforms/android/cordova/node_modules/q/q.js:816:13)
    at /Users/smacdona/code/androidTest/platforms/android/cordova/node_modules/q/q.js:877:14
    at runSingle (/Users/smacdona/code/androidTest/platforms/android/cordova/node_modules/q/q.js:137:13)
Error: Unable to graft xml at selector "/manifest" from "/Users/smacdona/code/androidTest/platforms/android/app/src/main/res/xml/config.xml" during config install
infil00p commented 6 years ago

@macdonst Read @stevegill's note about changing package.json when testing locally. If you don't do this, you will definitely get this error.

macdonst commented 6 years ago

@infil00p okay, got it. Once I do that step I get a bit farther but then I run into other problems. The plugin doesn't compile because it is missing all of the frameworks it depends on. For instance the push plugin includes:

    <framework src="com.android.support:support-v13:26.+"/>
    <framework src="me.leolin:ShortcutBadger:1.1.17@aar"/>
    <framework src="com.google.firebase:firebase-messaging:$FCM_VERSION"/>
    <framework src="push.gradle" custom="true" type="gradleReference"/>

None of those things seem to get included in build.gradle. Previously I'd see:

// PLUGIN GRADLE EXTENSIONS START
apply from: "phonegap-plugin-push/app-push.gradle"
// PLUGIN GRADLE EXTENSIONS END

dependencies {
    compile fileTree(dir: 'libs', include: '*.jar')
    // SUB-PROJECT DEPENDENCIES START
    debugCompile(project(path: "CordovaLib", configuration: "debug"))
    releaseCompile(project(path: "CordovaLib", configuration: "release"))
    compile "com.android.support:support-v13:26.+"
    compile "me.leolin:ShortcutBadger:1.1.17@aar"
    compile "com.google.firebase:firebase-messaging:11.0.1"
    // SUB-PROJECT DEPENDENCIES END
}

But with this change I only see:

dependencies {
    compile fileTree(dir: 'libs', include: '*.jar')
    // SUB-PROJECT DEPENDENCIES START
    debugCompile(project(path: ":CordovaLib", configuration: "debug"))
    releaseCompile(project(path: ":CordovaLib", configuration: "release"))
    // SUB-PROJECT DEPENDENCIES END
}
janpio commented 6 years ago
infil00p commented 6 years ago
  1. The package.json's version is meaningless since this is a PR and not released code. It should read 6.5.0-dev
  2. The APKs are where they appear by default in an Android project. Given that this is a normal Gradle project, you can just edit the top-level Gradle as you would in a regular Android project: https://stackoverflow.com/questions/23008485/change-gradle-build-directory-in-android-studio
  3. I think this would be the correct answer for the generated project's build path, but I'm not certain. https://stackoverflow.com/questions/23008485/change-gradle-build-directory-in-android-studio

I think at this point, we should probably merge it so that these can get done on the repo before we go into a vote.

piotr-cz commented 6 years ago

Docs for 7.x should change to reflect new location for build-extras.gradle and gradle.properties as these are now included only when in /app subfolder or include path in app/build.gradle should traverse one folder up.

See https://github.com/apache/cordova-docs/pull/810