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

#552: (android) check for build-extras.gradle in the app-parent directory #553

Merged
merged 2 commits into from
Nov 14, 2018

Conversation

DavidWiesner
Copy link
Contributor

Platforms affected

  • android

What does this PR do?

What testing has been done on this change?

  • in a local build environment

Checklist

  • Reported an issue in the JIRA database
  • Commit message follows the format: "CB-3232: (android) Fix bug with resolving file paths", where CB-xxxx is the JIRA ID & "android" is the platform affected.
  • Added automated test coverage as appropriate for this change.

@janpio
Copy link
Member

janpio commented Nov 14, 2018

This would be a breaking change right now, so I guess it would be nicer to just check both locations?

@brodycj
Copy link
Contributor

brodycj commented Nov 14, 2018

I think we will need to adapt pluginHandlers.js instead. Followup question is coming on #552.

Copy link
Contributor

@brodycj brodycj left a comment

Choose a reason for hiding this comment

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

I concur with @janpio that we should check both locations.

Alternative is to just update the documentation (apache/cordova-docs#905).

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

brodycj commented Nov 14, 2018

I just pushed an update to support both locations, didn't test it yet.

@DavidWiesner
Copy link
Contributor Author

Ok I can update the pull request. Wich file should win in the case both files existing? Should the apply exclusively or in sequence if both files existing?

@codecov-io
Copy link

Codecov Report

Merging #553 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #553   +/-   ##
=======================================
  Coverage   61.98%   61.98%           
=======================================
  Files          17       17           
  Lines        1978     1978           
  Branches      369      369           
=======================================
  Hits         1226     1226           
  Misses        752      752

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 576ad18...ff1c01b. Read the comment docs.

@brodycj
Copy link
Contributor

brodycj commented Nov 14, 2018

Good question. I thought we should run both files, didn't think about the ordering.

I would personally like to see the old location deprecated at some point.

If my commit doesn't help then feel free to remove it with a force push. I am still not a real Gradle expert.

And we should update the documentation as well. If I follow your link and click the edit button it takes me to the following file: https://github.com/apache/cordova-docs/blob/master/www/docs/en/dev/guide/platforms/android/index.md

@janpio can you think of anything else?

@DavidWiesner
Copy link
Contributor Author

I think your commit is just fine. This was my prefered order and method. I hope the old location will stay. It was luck that I found this bug before shipping our alpha build to public. Otherwise ~2000 people local settings and database get lost.

@brodycj
Copy link
Contributor

brodycj commented Nov 14, 2018

Thanks @DavidWiesner, would like to get review from one more member before merging.

If someone else does a merge, I recommend a squash merge but please be sure to give @DavidWiesner credit in the commit. I hope we can do this today.

@brodycj brodycj mentioned this pull request Nov 14, 2018
7 tasks
@brodycj brodycj merged commit 7eed65e into apache:master Nov 14, 2018
brodycj pushed a commit to brodycj/cordova-android that referenced this pull request Nov 14, 2018
…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 pushed a commit to brodycj/cordova-android that referenced this pull request Nov 15, 2018
…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 pushed a commit to brodycj/cordova-android that referenced this pull request Nov 16, 2018
…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 pushed a commit to brodycj/cordova-android that referenced this pull request Nov 16, 2018
…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>
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