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

No longer require tooling that has been removed and/or is no longer referenced by the SDK build #467

Merged
merged 6 commits into from Aug 24, 2021

Conversation

ewanharris
Copy link
Contributor

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

Removes the requirement for the dx tool (removed in the API 31 install) and the also removes the tools no longer directly referenced in the SDK build scripts.

This will need a follow up on the Titanium CLI repo to no longer as that checks that they exist in the ti setup check command, no breakage seen in Studio

@build
Copy link

build commented Aug 13, 2021

Warnings
⚠️

lib/android.js#L56 - lib/android.js line 56 – Found non-literal argument in require (security/detect-non-literal-require)

⚠️

lib/android.js#L260 - lib/android.js line 260 – 'stderr' is defined but never used. Allowed unused args must match /^_.+/u. (no-unused-vars)

⚠️

lib/android.js#L281 - lib/android.js line 281 – 'err' is defined but never used. Allowed unused args must match /^_.+/u. (no-unused-vars)

⚠️

lib/android.js#L313 - lib/android.js line 313 – 'stderr' is defined but never used. Allowed unused args must match /^_.+/u. (no-unused-vars)

⚠️

lib/android.js#L1036 - lib/android.js line 1036 – 'systemImages' is defined but never used. Allowed unused args must match /^_.+/u. (no-unused-vars)

Messages
📖 🎉 - congrats on your new release
📖

✅ All tests are passing
Nice one! All 27 tests are passing.

Generated by 🚫 dangerJS against f83c6c9

It looks to take at least 90 seconds for the emulator to start on github
lib/android.js Outdated
message += '\n';
}
message +=
__('Current installed Android SDK tools:') + '\n'
Copy link

Choose a reason for hiding this comment

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

  • ⚠️ lib/android.js line 781 – Found non-literal argument to RegExp Constructor (security/detect-non-literal-regexp)

lib/android.js Outdated
message += '\n';
}
message +=
__('Current installed Android SDK tools:') + '\n'
Copy link

Choose a reason for hiding this comment

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

  • ⚠️ lib/android.js line 717 – '+=' should be placed at the beginning of the line. (operator-linebreak)

lib/android.js Outdated
message += '\n';
}
message +=
__('Current installed Android SDK tools:') + '\n'
Copy link

Choose a reason for hiding this comment

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

  • ⚠️ lib/android.js line 256 – 'stderr' is defined but never used. Allowed unused args must match /^_.+/u. (no-unused-vars)

lib/android.js Outdated
message += '\n';
}
message +=
__('Current installed Android SDK tools:') + '\n'
Copy link

Choose a reason for hiding this comment

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

  • ⚠️ lib/android.js line 782 – Found non-literal argument to RegExp Constructor (security/detect-non-literal-regexp)

@ewanharris
Copy link
Contributor Author

Jenkins failures were due to the emulator not being closed on the node. So the tests never called start, which dropped the coverage below the threshold

@jquick-axway
Copy link
Contributor

@ewanharris, so I'm guessing our ti command line tool will still report error on these. That's the follow up PR changes you need to make, right?
https://github.com/appcelerator/titanium/blob/master/lib/commands/setup.js#L1023-L1059

Maybe we close this PR (ie: keep the executable lookups we're doing) and just remove the exe checks in our titanium repo instead? This way older versions of the ti command line tool won't throw a fit about the executable checks we're removing here?

@ewanharris
Copy link
Contributor Author

@jquick-axway, so that's for ti setup check. For the error from ti info the dx property needs removing from requiredSdkTools in this repo as that is used to determine the missing tooling here. If you'd like, I can just remove the executables from that requiredSdkTools object and keep them on the results object we return

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.

LGTM

@ewanharris ewanharris merged commit a70afc8 into tidev:master Aug 24, 2021
@ewanharris ewanharris deleted the remove_dx branch August 24, 2021 20:41
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

3 participants