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

CB-13580: (android) fix build for multiple apks (different product flavors) #417

Merged
merged 2 commits into from Nov 16, 2017

Conversation

@DavidStrausz
Copy link
Contributor

@DavidStrausz DavidStrausz commented Nov 16, 2017

Platforms affected

cordova-android

What does this PR do?

This PR fixes the build if cdvBuildMultipleApks is set to true by defining flavorDimensions "default" in build.gradle and by adapting the findOutputApksHelper method in GenericBuilder.js to recursively scan the build/outputs/apk directory for matching candidates.

A positive side-effect of this is that builds with crosswalk will work again :-)

What testing has been done on this change?

I ran the unit tests and the connectedAndroidTest tests and did manual tests of builds with crosswalk (multiple APKs) and builds without crosswalk (single APK).

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.
@infil00p
Copy link
Member

@infil00p infil00p commented Nov 16, 2017

@DavidStrausz Fix the lint errors and I'll merge this in. I tried adding more flavours and it kept crapping the bed on me. I didn't realize that just adding default would get this to work properly again.

If you're inclined, you can always send a PR to my Crosswalk repo. It's technically the "official" repo, and if you could get that working, that would win you some serious cred. (And yeah, I'll try to merge it in, because I can do that, even though I'm not technically supposed to).

@DavidStrausz
Copy link
Contributor Author

@DavidStrausz DavidStrausz commented Nov 16, 2017

@infil00p Done! 👍 Yeah it also took me some time to figure it out. But the migration guide was really useful if read carfully.

I will definitely keep that in mind for future contributions, thanks for pointing that out!

@codecov-io
Copy link

@codecov-io codecov-io commented Nov 16, 2017

Codecov Report

Merging #417 into master will decrease coverage by 0.23%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #417      +/-   ##
==========================================
- Coverage   44.01%   43.77%   -0.24%     
==========================================
  Files          17       17              
  Lines        1679     1688       +9     
  Branches      304      306       +2     
==========================================
  Hits          739      739              
- Misses        940      949       +9
Impacted Files Coverage Δ
...n/templates/cordova/lib/builders/GenericBuilder.js 27.5% <0%> (-3.49%) ⬇️

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 3760616...5d99e50. Read the comment docs.

@infil00p infil00p merged commit 6404780 into apache:master Nov 16, 2017
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@fredgalvao
Copy link

@fredgalvao fredgalvao commented Nov 20, 2017

@DavidStrausz thank you for this.

@infil00p Considering that the combination of the following aspects:

  • Oreo
  • crosswalk breakage
  • wave of plugin updates

is quite important and that crosswalk usage is considerably high (my estimate, no data), what are the chances of a 6.4.1/6.5.0 coming out soon to include this? I'm interested in your PoV in this subject.
I've seen many people delaying/cancelling upgrade plans because of that on the cordova slack channels, and I'd say it would bring a lot of people/projects up to speed again (including mine).

@infil00p
Copy link
Member

@infil00p infil00p commented Nov 21, 2017

@fredgalvao The next version of Cordova-Android will be 7.0.0, and this will be a breaking change where we finally update the project structure to work with Android Studio, as opposed to the cobbled legacy project layout that we're using today. I don't have any plans on releasing a 6.4.1 release at this time. Other than Crosswalk, I don't see a pressing need (we have Oreo support already, plugins are decoupled from the platform and people should be migrating from Crosswalk, since we have no guarantee that it'll stay working now that Intel dropped support.

@fredgalvao
Copy link

@fredgalvao fredgalvao commented Nov 21, 2017

@infil00p Unfortunately, for some projects, migrating out of crosswalk isn't really a crosswalk in the park. I share your mindset nevertheless, I'm just looking for a way to be able to upgrade stuff without having to wait for a long-term upgrade plan to migrate data for users.

Thanks for the info, and I'm looking forward to 7.0.0.

infil00p added a commit to infil00p/cordova-android that referenced this issue Nov 21, 2017
Fixing conflict caused by merging apache#417 into master
@fredgalvao
Copy link

@fredgalvao fredgalvao commented Nov 21, 2017

I'll just let it documented here to anyone looking for a way out, that @dpa99c has taken the initiative to make a plugin that can migrate data from Crosswalk to the system webview, to be used to move out of Crosswalk.

https://github.com/dpa99c/cordova-plugin-crosswalk-data-migration

@Robula
Copy link

@Robula Robula commented Dec 13, 2017

Did this affect the output path to APKs?

I'm seeing that the the output is now:
build/outputs/apk/release/android-release.apk
Where previously it was:
build/outputs/apk/android-release.apk

I've created an issue in the relevant place where I am experiencing problems.
microsoft/vsts-cordova-tasks#81

@DavidStrausz
Copy link
Contributor Author

@DavidStrausz DavidStrausz commented Dec 14, 2017

@Robula The output path was not affected, it changed with the gradle upgrade. This PR just fixed how an already output *.apk is found in the build/outputs/apk directory.

@Robula
Copy link

@Robula Robula commented Dec 15, 2017

@DavidStrausz Thank you for the explanation. That is strange as I am seeing a difference in output paths between 6.3.0 and 6.4.0. I'll trash my platforms and plugins and reconfirm.

@ataraxus
Copy link

@ataraxus ataraxus commented Dec 21, 2017

Hello folks,

foremost, thanks @DavidStrausz for your effort and explanation!

But seriously, i know this is symptomatic for the JS world, but introducing breaking changes with a minor release? WTF!? The Ionites do have the cordova dependency declared as ^6.2.3 so after checking out the project from scratch and building it we get a broken build! Due to the Crosswalk issue and due to changed output directories.

Now, i guess to rub salt and tequila in my eyes, instead of somehow fleshing out a bugfix release, I'm supposed to ditch Crosswalk and my android < 5 user base!? Seriously?

I know that my post is not helpful in anyway and also symptomatic for the JS world, but so be it - I'm defeated.

Cheers to all who try to make stuff working!

benjamn added a commit to meteor/cordova-android that referenced this issue Jan 15, 2018
CB-13580: (android) fix build for multiple apks (different product flavors)

Note from @benjamn: this commit was cherry-picked from version 7.0.0 onto
version 6.4.0 in Meteor's fork of cordova-android, since the maintainer
has indicated the changes probably will not be back-ported to 6.x:
apache#417 (comment)

cc @menelike @abernix
@Ankoul
Copy link

@Ankoul Ankoul commented Feb 17, 2018

This issue has been merged, but this code are not on release 7.0.0 and i still getting flavors error.
If add 'flavorDimensions "default"' on gradle.build manually it works perfectly, but cordova-android will override it on any "platform add android". Does someone knows a stable version with have this code?

benjamn added a commit to meteor/meteor that referenced this issue Jan 14, 2019
This version of cordova-android includes the PR that previously required
us to fork the package: apache/cordova-android#417

The cordova-ios update is just 4.5.4 => 4.5.5, so hopefully entirely
backwards compatible. :crossed-fingers:
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

8 participants