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

refactor!: remove most platform binaries #1100

Merged
merged 14 commits into from Jul 6, 2021

Conversation

raphinesse
Copy link
Contributor

@raphinesse raphinesse commented Oct 19, 2020

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 by cordova-lib to determine the platform version)
  • cordova/android_sdk_version (used to implement the <engine> requirement that can be specified in plugin.xml)
  • cordova/lib/list-devices (for cordova targets)
  • cordova/lib/list-emulator-images (for cordova targets)

We could remove all the *.bat counterparts though, since execa (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 use execa then.

There are still places where nopt is used to parse arguments passed through from CLI, so we cannot remove it.

@raphinesse raphinesse added this to the 10.0.0 milestone Oct 19, 2020
raphinesse added a commit to raphinesse/cordova-android that referenced this pull request Oct 19, 2020
Copy link
Member

@dpogue dpogue left a 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

@erisu
Copy link
Member

erisu commented Oct 21, 2020

Everything seems fine but had only 1 question:

  • android_sdk_version, which was removed, seems to be called from cordova-lib. Can it be confirmed that this code in lib is not called in the CLI workflow?

@raphinesse
Copy link
Contributor Author

android_sdk_version, which was removed, seems to be called from cordova-lib. Can it be confirmed that this code in lib is not called in the CLI workflow?

Good catch! Unfortunately this is called during plugin installation as part of the check for the <engine> requirement that can be specified in plugin.xml.

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.

@codecov-io
Copy link

codecov-io commented Oct 22, 2020

Codecov Report

Merging #1100 (0d4f08f) into master (aa679ea) will increase coverage by 0.38%.
The diff coverage is n/a.

Impacted file tree graph

@@            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     
Impacted Files Coverage Δ
bin/lib/create.js 93.47% <ø> (-0.40%) ⬇️
bin/templates/cordova/lib/android_sdk.js 100.00% <ø> (ø)
bin/templates/cordova/lib/build.js 39.39% <ø> (+6.06%) ⬆️
bin/templates/cordova/lib/run.js 100.00% <ø> (ø)

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 aa679ea...0d4f08f. Read the comment docs.

raphinesse added a commit to raphinesse/cordova-android that referenced this pull request Oct 22, 2020
@erisu erisu added this to In Progress in Release Plan - 10.1.0 via automation Apr 13, 2021
@erisu
Copy link
Member

erisu commented Apr 16, 2021

@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?

@raphinesse
Copy link
Contributor Author

@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.

@raphinesse raphinesse changed the title refactor!: remove platform binaries refactor!: remove most platform binaries Jul 5, 2021
@codecov-commenter
Copy link

codecov-commenter commented Jul 6, 2021

Codecov Report

Merging #1100 (a8f46bb) into master (69b24db) will increase coverage by 0.55%.
The diff coverage is n/a.

Impacted file tree graph

@@            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     
Impacted Files Coverage Δ
bin/lib/create.js 94.59% <ø> (-0.31%) ⬇️
bin/templates/cordova/lib/build.js 39.39% <ø> (+6.06%) ⬆️
bin/templates/cordova/lib/run.js 96.55% <ø> (-1.01%) ⬇️

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 69b24db...a8f46bb. Read the comment docs.

@raphinesse raphinesse marked this pull request as ready for review July 6, 2021 09:13
Release Plan - 10.1.0 automation moved this from In Progress to Reviewer approved Jul 6, 2021
@raphinesse raphinesse merged commit facffb0 into apache:master Jul 6, 2021
Release Plan - 10.1.0 automation moved this from Reviewer approved to Done Jul 6, 2021
@raphinesse raphinesse deleted the remove-platform-binaries branch July 6, 2021 13:37
wedgberto pushed a commit to wedgberto/cordova-android that referenced this pull request May 17, 2022
* 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

6 participants