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

build(android): rework merging of external library's resources #11094

Merged
merged 5 commits into from Oct 30, 2019

Conversation

ypbnv
Copy link
Contributor

@ypbnv ypbnv commented Jul 30, 2019

JIRA: https://jira.appcelerator.org/browse/TIMOB-27252

Description:
Rework the merging of third party native libraries resources. In order to try to keep backwards compatibility the merging now:

  • appends children nodes with different names for parents with the same name;
  • overwrites only children with the same names for parents with the same name (previously it overwrote the parent). This happens in order of expanding the libraries.

Test case:
There is an archive in the JIRA ticket with a version of a third party native library. Put the AAR files under app/platform/android for an Alloy project and build the project.

Rework the merging of third party native libraries resources.
@build
Copy link
Contributor

build commented Jul 30, 2019

Fails
🚫 Tests have failed, see below for more information.
Messages
📖

💾 Here's the generated SDK zipfile.

📖 ❌ 1 tests have failed There are 1 tests failing and 479 skipped out of 4890 total tests.
📖 ✊ The commits in this PR match our conventions! Feel free to Rebase and Merge this PR when ready.

Tests:

ClassnameNameTimeError
ios.Titanium.UI.WebViewbaseURL should be accessible via window.location2.001
Error: timeout of 2000ms exceeded
file:///Users/build/Library/Developer/CoreSimulator/Devices/2589D817-D6C9-4AAB-ABDB-8C8068955919/data/Containers/Bundle/Application/89216A44-E14D-4C5D-BBE1-85C6D199CB5A/mocha.app/ti-mocha.js:4326:27

Generated by 🚫 dangerJS against 690b87a

@sgtcoolguy
Copy link
Contributor

@ypbnv Given that this is an Android fix/improvement, shouldn't this be scheduled for 8.3.0 milestone/release?

@ypbnv ypbnv modified the milestones: 8.2.0, 8.3.0 Jul 31, 2019
@ypbnv
Copy link
Contributor Author

ypbnv commented Jul 31, 2019

@sgtcoolguy Ah, my bad. I will change it. BTW this change may be irrelevant once we move to using gradle, because it is dealing with merging resource files. Maybe we can consider backporting it for a release before it?

});
nodes[node.tagName][n].appendChild(nodeChild.cloneNode(true));
});
_t.logger.warn(__('Merging XML node %s in file %s', String(n).cyan, dest.cyan));
Copy link
Contributor

Choose a reason for hiding this comment

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

This has been bothering me for ages, but i think this should be debug rather than warn. Also move it back to the top before doing the actual merging otherwise it could cause confusion when something goes wrong during the merge and the last log was for a completely different merge operation.

// We have a node with the same name. Merging as follows:
// Nodes with the same name get overwritten to maintain backwards compatiblity.
// Nodes with different name are appended to the parent node.
xml.forEachElement(node, function (nodeChild) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
xml.forEachElement(node, function (nodeChild) {
xml.forEachElement(node, function (childNode) {

@ypbnv
Copy link
Contributor Author

ypbnv commented Oct 8, 2019

@janvennemann Updated.

@jquick-axway jquick-axway changed the title build(android): rework merging of external librarie's resources build(android): rework merging of external library's resources Oct 9, 2019
Copy link
Contributor

@jquick-axway jquick-axway left a comment

Choose a reason for hiding this comment

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

CR: Pass

@ssekhri
Copy link

ssekhri commented Oct 29, 2019

FR Passed.
Verified on:
Mac OS: 10.14.5
SDK: 8.3.0.v20191008074512
Appc CLI: 7.1.1
JDK: 1.8.0_162
Node: 10.5.0
Studio: 5.1.4.201909061933

@sgtcoolguy sgtcoolguy merged commit 616b749 into tidev:master Oct 30, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants