-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
refactor!: remove most platform binaries #1100
refactor!: remove most platform binaries #1100
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me so far
Everything seems fine but had only 1 question:
|
Good catch! Unfortunately this is called during plugin installation as part of the check for the We will have to keep that too for now. In the long run we should probably change how lib delegates this task to the platforms. |
6b8185a
to
f3045ec
Compare
Codecov Report
@@ Coverage Diff @@
## master #1100 +/- ##
==========================================
+ Coverage 71.41% 71.79% +0.38%
==========================================
Files 20 19 -1
Lines 1770 1709 -61
==========================================
- Hits 1264 1227 -37
+ Misses 506 482 -24
Continue to review full report at Codecov.
|
f3045ec
to
11dec91
Compare
11dec91
to
0d4f08f
Compare
@raphinesse this PR will need to be rebased and the conflicts resolved. Other then that, were there other changes needed or is this PR ready and could be merged in? |
I'm not sure. I think at least the TODOs in the description still have to be applied. Given that the next major release of this platform is supposed to be independent from a tooling release, this would mean keeping anything that would otherwise break compatibility with current tooling. |
0d4f08f
to
7ef8313
Compare
7ef8313
to
1a6bfee
Compare
Codecov Report
@@ Coverage Diff @@
## master #1100 +/- ##
==========================================
+ Coverage 71.08% 71.64% +0.55%
==========================================
Files 22 21 -1
Lines 1705 1654 -51
==========================================
- Hits 1212 1185 -27
+ Misses 493 469 -24
Continue to review full report at Codecov.
|
Usage: cordova-lib/src/cordova/targets.js
This is used in CLI to implement an Android SDK version check for plugins. See https://cordova.apache.org/docs/en/latest/plugin_ref/spec.html#engines-and-engine
1a6bfee
to
c8c7b1a
Compare
* Remove binaries cordova/lib/* * Remove binary bin/android_sdk_version * Remove binary bin/update script * Remove binary bin/check_reqs * Remove binary bin/create script * Remove binary cordova/build * Remove binary cordova/run * Remove binary cordova/clean * Remove binary cordova/log * Remove unused module cordova/loggingHelper * Update README * Restore target-listing binaries used by CLI Usage: cordova-lib/src/cordova/targets.js * Restore binary bin/android_sdk_version for CLI compatibility This is used in CLI to implement an Android SDK version check for plugins. See https://cordova.apache.org/docs/en/latest/plugin_ref/spec.html#engines-and-engine * Remove version.bat
Motivation and Context
This removes all platform binaries that are not required for compatibility to Cordova CLI.
My work to stop copying
cordova/lib
to the platform folder is based on this PR among others.Details
The binaries that remain (relative to a platform project) are:
cordova/version
(used bycordova-lib
to determine the platform version)cordova/android_sdk_version
(used to implement the<engine>
requirement that can be specified inplugin.xml
)cordova/lib/list-devices
(forcordova targets
)cordova/lib/list-emulator-images
(forcordova targets
)We could remove all the
*.bat
counterparts though, sinceexeca
(which is used to run the binaries) also implements shebang-lookup on Windows. This will break compatibility to any CLI version <10, since we did not yet useexeca
then.There are still places where
nopt
is used to parse arguments passed through from CLI, so we cannot remove it.