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

[TIMOB-24734] Add compatibility with Hyperloop AAR handling #9086

Merged
merged 5 commits into from Jul 6, 2017

Conversation

janvennemann
Copy link
Contributor

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

Optional Description:
Hyperloop currently still has its own AAR handling which interferes with the new AAR handling in the SDK. This fix will ensure the compatibility by reverting any changes Hyperloop is trying to make to the builder.

Hyperloop currently still has its own AAR handling which interferes with the new AAR handling in the SDK. This fix will ensure the compatibility by reverting any changes Hyperloop is trying to make to the builder.
@cb1kenobi
Copy link
Contributor

LGTM

@sgtcoolguy
Copy link
Contributor

I opened tidev/hyperloop.next#179 to make hyperloop build against SDK 6.2.0+ (with the new module api version of 4, due to V8 API changes).

@@ -41,9 +41,10 @@ const LIBRARY_ORIGIN_PORJECT = 'Project';

exports.cliVersion = '>=3.2';

exports.init = function (logger, config, cli) {
exports.init = function (logger, config, cli, appc) {
Copy link
Contributor

Choose a reason for hiding this comment

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

The varying argument order for the same objects bugs me:
(logger, config, cli, appc) vs
(cli, builder, appc, logger) vs
(builder, callback) vs
(builder, logger, callback)

I prefer to try and retain argument order relatively if possible, so I guess in these cases it would be:
(logger, config, cli, appc, callback) - where builder falls, I'm not sure beyond having it before callback

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I sorted the arguments a little bit by their priority. cli and builder being the main objects i interact with, followed by utility stuff like appc and logger and the callback always being the last argument.

return;
}

var hyperloopBuildPath = path.join(builder.projectDir, 'build/hyperloop/android');
Copy link
Contributor

Choose a reason for hiding this comment

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

does node handle unix slashes properly on Windows? I always tend to avoid using separators just in case:
var hyperloopBuildPath = path.join(builder.projectDir, 'build', 'hyperloop', 'android');

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, path.join will properly take care of the slashes on Windows

Copy link
Contributor

@sgtcoolguy sgtcoolguy left a comment

Choose a reason for hiding this comment

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

LGTM

@hansemannn hansemannn added this to the 6.2.0 milestone Jul 6, 2017
@hansemannn hansemannn merged commit b260d67 into tidev:master Jul 6, 2017
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

4 participants