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!: drop platform binaries #1180

Merged
merged 24 commits into from
Nov 6, 2021
Merged

Conversation

erisu
Copy link
Member

@erisu erisu commented Nov 4, 2021

Motivation and Context

This PR:

  • Removes all platform binaries that are not required for compatibility to Cordova CLI.
  • Refactored the update process to remove excessive hops/function calls.
  • Removed some unneeded comments from version binary.
  • Updated create.spec.js to use JS methods for creating and building.

Description

  • Drop create binaries
    • Update create.spec.js
  • Drop build binaries
    • Update create.spec.js
  • Drop update binaries
  • Drop test binaries
    • This binary has never been used since 2012. It was referencing a binary that was renamed in 2012. If we want to add mobile-spec, it should be done in a separate PR and done with GH Actions Workflows.
  • Drop check_req binaries
  • Drop clean binaries
  • Drop log binaries
  • Drop run binaries
  • Cleanup version binaries
  • Drop uncrustify binary & config
    • This binary referenced a npm module that has not been in our repo for a long time.
    • It was not used for over 2~3+ years.
    • The npm module has not been updated over 7 years.
  • Drop list-emulator-build-targets binary
  • Drop list-started-emulators binary
  • Drop start-emulator binary

Testing

  • npm t
  • GitHub Actions
  • Cordova CLI 10.0.0 (cordova-lib@10.1.0)
    • cordova create
    • cordova platform add ../cordova-ios-7.0.0-dev.tgz
    • cordova platform update ../cordova-ios-7.0.0-dev.tgz
    • cordova prepare ios
    • cordova build ios
    • cordova info
    • cordova requirements
    • cordova platform
    • cordova run ios
    • cordova plugin add cordova-plugin-device

Checklist

  • I've run the tests to see all new and existing tests pass
  • I added automated test coverage as appropriate for this change

@erisu erisu added this to the 7.0.0 milestone Nov 4, 2021
@erisu erisu force-pushed the feat/drop-binaries branch 5 times, most recently from 728558b to 178e2d0 Compare November 4, 2021 07:31
@codecov-commenter
Copy link

codecov-commenter commented Nov 4, 2021

Codecov Report

Merging #1180 (381ed72) into master (a6044dd) will increase coverage by 0.07%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1180      +/-   ##
==========================================
+ Coverage   74.95%   75.03%   +0.07%     
==========================================
  Files          14       13       -1     
  Lines        1705     1650      -55     
==========================================
- Hits         1278     1238      -40     
+ Misses        427      412      -15     
Impacted Files Coverage Δ
bin/templates/scripts/cordova/Api.js 70.58% <0.00%> (-1.72%) ⬇️
bin/templates/scripts/cordova/lib/build.js 44.09% <ø> (-8.03%) ⬇️
bin/templates/scripts/cordova/lib/run.js 22.22% <ø> (+2.59%) ⬆️
bin/templates/scripts/cordova/loggingHelper.js

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 a6044dd...381ed72. Read the comment docs.

@erisu erisu force-pushed the feat/drop-binaries branch 5 times, most recently from d2e5d83 to 1806649 Compare November 4, 2021 08:20
@erisu
Copy link
Member Author

erisu commented Nov 4, 2021

Other binaries & scripts that will need reviewing... Maybe another PR?

  • apple_ios_version
  • apple_osx_version
  • apple_xcode_version
  • cordova_plist_to_config_xml
  • list-devices
  • list-emulator-build-targets
  • list-emulator-images
  • list-started-emulators
  • start-emulator

I believe some of these could be removed and/or merged into some JS file(s) and exported.

Copy link
Contributor

@raphinesse raphinesse left a comment

Choose a reason for hiding this comment

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

Great to see this happening. I left a few comments for now. Full review pending!

tests/spec/create.spec.js Outdated Show resolved Hide resolved
tests/spec/create.spec.js Outdated Show resolved Hide resolved
tests/spec/create.spec.js Outdated Show resolved Hide resolved
Copy link
Contributor

@raphinesse raphinesse left a comment

Choose a reason for hiding this comment

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

We should also remove hooks/ if we remove uncrustify.

@raphinesse
Copy link
Contributor

Other binaries & scripts that will need reviewing... Maybe another PR?
...
I believe some of these could be removed and/or merged into some JS file(s) and exported.

The following bins are used outside of cordova-ios and should be left alone for now:

apple_ios_version
./lib/src/plugman/util/default-engines.js
43:            { platform: 'ios', scriptSrc: path.join(project_dir, 'cordova', 'apple_ios_version') },

apple_osx_version
./lib/src/plugman/util/default-engines.js
45:            { platform: 'ios', scriptSrc: path.join(project_dir, 'cordova', 'apple_osx_version') },

apple_xcode_version
./lib/src/plugman/util/default-engines.js
41:            { platform: 'ios', scriptSrc: path.join(project_dir, 'cordova', 'apple_xcode_version') },

list-devices
./lib/src/cordova/targets.js
35:    const caller = { script: 'list-devices' };
37:    const cmd = path.join(projectRoot, 'platforms', platform, 'cordova', 'lib', 'list-devices');

list-emulator-images
./lib/src/cordova/targets.js
42:    const caller = { script: 'list-emulator-images' };
44:    const cmd = path.join(projectRoot, 'platforms', platform, 'cordova', 'lib', 'list-emulator-images');

These are not used outside of cordova-ios and could be removed in this PR:

  • cordova_plist_to_config_xml
  • list-emulator-build-targets
  • list-started-emulators
  • start-emulator

erisu and others added 2 commits November 5, 2021 21:03
@erisu
Copy link
Member Author

erisu commented Nov 5, 2021

Yes the pre-commit hook can be removed. I didn't notice it and it does not get runned.

@erisu
Copy link
Member Author

erisu commented Nov 5, 2021

@raphinesse I will excluded cordova_plist_to_config_xml from the dropping for now as it is written in a log output.

https://github.com/apache/cordova-ios/blob/master/CordovaLib/Classes/Public/CDVViewController.m#L155

Alternative message will need to be decided and maybe we could keep the script in some folder for handy scripts but not release it with the packages.

@raphinesse
Copy link
Contributor

raphinesse commented Nov 5, 2021

@raphinesse I will excluded cordova_plist_to_config_xml from the dropping for now as it is written in a log output.

https://github.com/apache/cordova-ios/blob/master/CordovaLib/Classes/Public/CDVViewController.m#L155

Alternative message will need to be decided and maybe we could keep the script in some folder for handy scripts but not release it with the packages.

I think the script and log message can both be removed without replacement. It's a python script that is supposed to convert Projects to the Cordova 2.3 format:

Converts a project's Cordova.plist file into a config.xml one. This conversion is required for Cordova 2.3.

But it might indeed be wise to do that in a separate PR and decide how to handle that whole if statement that is seemingly handling a very old legacy case.

@raphinesse
Copy link
Contributor

Yet another thing I noticed: we should be able to remove all remaining *.bat files. As mentioned in apache/cordova-android#1100:

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.

Should we do that as well, then?

Copy link
Contributor

@raphinesse raphinesse left a comment

Choose a reason for hiding this comment

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

Great stuff! Always happy when we can delete a few kLoCs 💪

Don't forget to check my last two comments, but feel free to merge as-is if you want.

@erisu
Copy link
Member Author

erisu commented Nov 5, 2021

Since we already removed it from the Android platform and released it, maybe it is safe to remove it from here as well?

I wonder if anyone has any concerns, even though it was already removed in Android... If we remove it and later it is seen as an issue, we can easily add back as a patch release.

@raphinesse
Copy link
Contributor

I'd remove the bat files. That's my 2¢

@erisu
Copy link
Member Author

erisu commented Nov 6, 2021

OK, the three last bat files were removed:

  • bin/apple_ios_version.bat
  • bin/apple_osx_version.bat
  • bin/apple_xcode_version.bat

@erisu erisu merged commit 49d8291 into apache:master Nov 6, 2021
@erisu erisu deleted the feat/drop-binaries branch November 6, 2021 04:46
gazben pushed a commit to apicore-engineering/cordova-ios that referenced this pull request Aug 26, 2022
* test(create): remove create & build binary usage
* refactor!: remove create binaries
* refactor!: remove build binaries
* refactor: update platform
* chore: cleanup update platform error message
* refactor!: remove update binaries
* refactor!: remove check_reqs binaries & usage
* refactor!: remove clean binaries
* refactor!: remove run binaries
* refactor!: remove test binaries
  * This file referenced a binary that hasnt existed since 2012. (apache@acc8b34)
* refactor!: remove log binaries
* refactor: cleanup version binaries
* refactor!: remove uncrustify binary & config
* refactor!: remove binary help methods for build & run
* refactor!: remove version.bat binary
* fix: get realpathSync of os tmp dir
* refactor: cleanup verify create & build methods
* refactor(create.spec): use expectAsync toBeResolved
* refactor!: remove uncrustify pre-commit hook
* refactor!: remove list-emulator-build-targets binary
* refactor!: remove list-started-emulators binary
* refactor!: remove start-emulator binary
* chore: cleanup eslint ignore
* refactor!: remove bat files

Co-authored-by: Raphael von der Grün <raphinesse@gmail.com>
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

3 participants