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

7.1.3 patch updates #555

Merged
merged 18 commits into from
Nov 18, 2018
Merged

7.1.3 patch updates #555

merged 18 commits into from
Nov 18, 2018

Conversation

brodycj
Copy link
Contributor

@brodycj brodycj commented Nov 14, 2018

Changes ported

How tested

npm test

  • npm run unit-tests succeeds
  • npm test succeeds in Travis CI and AppVeyor CI

cordova-sqlite-storage 2.1.5

In a clone of https://github.com/brodybits/cordova-sqlite-test-app

  • cordova platform add https://github.com/brodybits/cordova-android#7.1.3-patch-updates
  • cordova plugin add cordova-sqlite-storage@2.1.5
  • Able to build and run on a test device
  • Click on self test button results in an alert that the self test is OK (self test of SQLite CRUD operations in sqlite plugin)
  • cordova plugin remove cordova-sqlite-storage is successful

Other plugins tested

Add plugin, build, and remove plugin tested for:

  • cordova-plugin-deezer
  • https://github.com/adjust/cordova_sdk#master
  • https://github.com/adjust/cordova_sdk#cordova-8
  • https://github.com/dylearning/cordova-plugin-hikvisionVedio#master
  • https://github.com/j3k0/cordova-plugin-purchase#v7.1.0 (old version, without adaptations that were needed for cordova-android@7.0.0)

@brodycj brodycj changed the title [WIP] 7.1.3 patch updates - proposed 7.1.3 patch updates - proposed Nov 14, 2018
@brodycj brodycj changed the title 7.1.3 patch updates - proposed [WIP] 7.1.3 patch updates - proposed WIP Nov 14, 2018
@jcesarmobile
Copy link
Member

I don't see #551 fix in the resulting changes

@brodycj
Copy link
Contributor Author

brodycj commented Nov 14, 2018

I don't see #551 fix in the resulting changes

Just added it, thanks for catching it!

FYI This PR is still marked as WIP since I would like to test on a couple plugins before merging.

@brodycj brodycj changed the title [WIP] 7.1.3 patch updates - proposed WIP 7.1.3 patch updates Nov 15, 2018
@brodycj
Copy link
Contributor Author

brodycj commented Nov 15, 2018

I just finished testing the patch updates with some plugins listed in the OP description, quick review would be appreciated.

@brodycj
Copy link
Contributor Author

brodycj commented Nov 15, 2018

I just pushed a change to remove the GH-547 fix for aidl files. This means that plugins such as cordova-plugin-purchase would need to keep multiple aidl source-file entries to support both cordova-android@6 and cordova-android@latest.

This is needed to avoid breaking plugins such as cordova-plugin-purchase in a patch release. I suspect we should push this change to master to avoid breaking plugins such as cordova-plugin-purchase in the foreseeable future.

@jcesarmobile
Copy link
Member

This PR is to cherry-pick master changes to 7.1.x, if you push changes here before being on master, then it's not a cherry-pick anymore.
If the current patch for aidl files doesn't work well report an issue explaining it better (as by your comment I don't know what's wrong with this approach) and fix it in master first, and then we can cherry-pick on this branch.

@brodycj brodycj changed the title 7.1.3 patch updates [WIP] 7.1.3 patch updates Nov 15, 2018
@brodycj
Copy link
Contributor Author

brodycj commented Nov 15, 2018

Back to WIP for now

@brodycj
Copy link
Contributor Author

brodycj commented Nov 15, 2018

Waiting for final approval of #561, which is expected to be merged as a merge commit. I will include the merge commit in this proposal once it is available.

@brodycj brodycj changed the title [WIP] 7.1.3 patch updates 7.1.3 patch updates Nov 16, 2018
@brodycj
Copy link
Contributor Author

brodycj commented Nov 16, 2018

I just put the changes in almost the same order as they appear in master and removed "[WIP]" from the title, think it should be merged now. Any objections?

@janpio
Copy link
Member

janpio commented Nov 16, 2018

Maybe silly question, but what would be a way to compare master and this branch to see if the cherry-picked commits make sense and nothing is missing etc?

Second question: With 18 commits, wouldn't it make more sense to make this 7.2.0? Is this still just a patch release with so much content?

@jcesarmobile
Copy link
Member

No, because master have a lot more fixes that won't go into 7.1.3, so you can't directly compare

Also no to 7.2.0, this is supposedly a patch, and doing a minor bump will prevent the cli from getting this version that patches some bugs we caused in previous release

Will review later tonight

Darshan-Chauhan and others added 5 commits November 16, 2018 13:22
to work properly on Android Pie

was introduced in dc0bfeb (CB-11828)

Resolves apache#534

Co-authored-by: <pradiv-kumar@users.noreply.github.com>
Co-authored-by: Christopher J. Brody <chris.brody@gmail.com>
for Java source only (apacheGH-539)

Co-Authored-By: Christopher J. Brody <chris.brody@gmail.com>
Co-Authored-By: Kyle Kirbatski <kkirbatski@gmrmarketing.com>
Christopher J. Brody and others added 13 commits November 16, 2018 13:22
Fallback to old path mapping if no Android Studio path mapping exists

(with slight difference on 7.1.x branch)

Co-authored-by: Christopher J. Brody <chris.brody@gmail.com>
Co-authored-by: Kyle Kirbatski <kkirbatski@gmrmarketing.com>
for JAR and AAR

(apacheGH-540)

Co-Authored-By: Kyle Kirbatski <kkirbatski@gmrmarketing.com>
Co-Authored-By: Christopher J. Brody <chris.brody@gmail.com>
Co-Authored-By: @afdev82 (Antonio Facciolo)
for Java source, JAR, and AAR

Co-authored-by: Christopher J. Brody <chris.brody@gmail.com>
Co-authored-by: Kyle Kirbatski <kkirbatski@gmrmarketing.com>
Co-authored-by: Antonio Facciolo <afdev82@users.noreply.github.com>
Possibly related to: CB-13830: Add handlers for plugins
that use non-Java source files, such as Camera
of plugin source file installed into app/src/main with
old target-dir scheme

NOTE: These tests do *not* check compatibility of
plugins with old lib target-dir scheme.
(source-file entries)

including aidl, aar, jar, and so files
…t directory (apache#553)

as documented in https://cordova.apache.org/docs/en/latest/guide/platforms/android/?#extending-buildgradle

and deal with multiple build-extras.gradle locations

Co-authored-by: David Boho <david.boho@tu-ilmenau.de>
Co-authored-by: Christopher J. Brody <chris.brody@gmail.com>
@brodycj
Copy link
Contributor Author

brodycj commented Nov 16, 2018

I just updated the changes as follows:

  • moved a couple gradle updates to the end
  • added number of original PR on master where it was not present

I also updated the description to list the changes in the same order as they appear in this PR.

I hope this makes it easier to check what changes are in this PR. Thanks @jcesarmobile for your efforts to check the changes in both branches.

Copy link
Member

@jcesarmobile jcesarmobile left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@brodycj brodycj removed the request for review from dpogue November 18, 2018 00:31
@brodycj brodycj merged commit 442734d into apache:7.1.x Nov 18, 2018
@brodycj brodycj deleted the 7.1.3-patch-updates branch November 18, 2018 00:33
@brodycj
Copy link
Contributor Author

brodycj commented Nov 18, 2018

Thanks @jcesarmobile!

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

Successfully merging this pull request may close these issues.

None yet

5 participants