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

fix: copy gradle wrapper from tools to platform root dir #1781

Merged
merged 1 commit into from
Mar 18, 2025

Conversation

erisu
Copy link
Member

@erisu erisu commented Feb 19, 2025

Motivation and Context

Copy the gradle wrapper files from platforms/android/tools to platforms/android

Fixes #1760

Description

Updated install step to just copy all the gradle files up one directory.

Testing

  • Created a Cordova Project
  • Added Platform
  • Built Android Platform

Checklist

  • I've run the tests to see all new and existing tests pass
  • I added automated test coverage as appropriate for this change
  • Commit is prefixed with (platform) if this change only applies to one platform (e.g. (android))
  • If this Pull Request resolves an issue, I linked to the issue in the text above (and used the correct keyword to close issues using keywords)
  • I've updated the documentation if necessary

@erisu erisu added this to the 14.0.0 milestone Feb 19, 2025
@codecov-commenter
Copy link

codecov-commenter commented Feb 19, 2025

Codecov Report

Attention: Patch coverage is 25.00000% with 3 lines in your changes missing coverage. Please review.

Project coverage is 71.89%. Comparing base (7f95294) to head (11b6988).

Files with missing lines Patch % Lines
lib/builders/ProjectBuilder.js 25.00% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1781      +/-   ##
==========================================
- Coverage   72.00%   71.89%   -0.11%     
==========================================
  Files          23       23              
  Lines        1850     1854       +4     
==========================================
+ Hits         1332     1333       +1     
- Misses        518      521       +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@erisu erisu changed the title feat: move gradle wrapper from tools to platform root dir fix: move gradle wrapper from tools to platform root dir Feb 19, 2025
@breautek
Copy link
Contributor

removed tools/settings.gradle. Appears not needed.

It will be needed if the installed gradle at the system level is less than what AGP requires. That was the whole point of adding it, to serve as a dummy project with no dependencies so that we can get our gradle wrapper.

@breautek
Copy link
Contributor

To solve #1760

I think what we should do is copy the generated gradle, gradlew, and gradlew.bat files up a directory on each gradle wrapper install into platforms/android/. If the wrapper files already exists, then it should be replaced. We could also experiment with creating a symlink to tools/gradlew, tools/gradle, and tools/gradlew.bat (but idk if Android Studio will follow symlinks or if that will play nicely on windows)

We should also retain the tools/ gradle wrapper copy for future install commands. Like I mentioned, the tools/ project is a dummy project with no dependencies, so it allows for the user to have a wide range of system gradle installed. On each build, the install command is issued, but if the wrapper is already installed at the desired version, it's nearly a no-operation, so it will help subsequent builds as well.

Android Studio will use the wrapper installed at platforms/android/. The CLI could continue to use the wrapper in platforms/android/tools/ or be updated to use the copied version at platforms/android, that's up to preference at this point. But the CLI should make sure that platforms/android/ gradle install is identical to the one inside platform/android/tools/.

I hope this makes sense. I know having a copy is not ideal, but it helps give us the best of both worlds:

  1. Virtually any Gradle version can be used to install the gradle wrapper of the version needed by Cordova.
  2. Installed gradle wrapper will be used by Android Studio if the project is opened in the IDE.

If the symlink idea works, then that could be use to reduce the maintenance burden as well.

@breautek
Copy link
Contributor

This is kind of out of scope for #1760 but I also notice that we don't install the wrapper on create. I think it may only occur on build.

We should also consider doing a gradle install on project creation too (on cordova platform add android) so that the wrapper is immediately available. We should still continue to install on build as well in case they change the gradle version preference...

As it stands right now, the wrapper is only installed on cordova build android which means if someone added the android platform and then opened it up in Android Studio, it won't have the expected wrapper version, until they run a cordova build android

Just something I noticed while testing the current behaviour.

@erisu erisu marked this pull request as draft February 20, 2025 02:17
@erisu erisu force-pushed the feat/move-gradle-location branch from c1fb457 to 85f0c36 Compare March 17, 2025 01:37
@erisu erisu marked this pull request as ready for review March 17, 2025 01:39
@erisu erisu force-pushed the feat/move-gradle-location branch 2 times, most recently from 81c43fd to ffaa33e Compare March 17, 2025 02:45
@erisu erisu force-pushed the feat/move-gradle-location branch from ffaa33e to 11b6988 Compare March 17, 2025 03:37
@erisu
Copy link
Member Author

erisu commented Mar 17, 2025

we should do is copy the generated gradle, gradlew, and gradlew.bat files up a directory on each gradle wrapper install into platforms/android/.

This PR now copy up a directory as suggested.

If the wrapper files already exists, then it should be replaced.

This should be complete. It always runs a copy with recursive+force on build.

experiment with creating a symlink

Symlink will not be used because we need to ensure that the files remain when the platforms directory are zipped. It seems that there is a use case to prepare the platforms in one environment and built in another.

We should also retain the tools/ gradle wrapper copy for future install commands.

Done

Android Studio will use the wrapper installed at platforms/android/. The CLI could continue to use the wrapper in platforms/android/tools/

CLI uses tools version.

CLI should make sure that platforms/android/ gradle install is identical to the one inside platform/android/tools/

CLI should always call the install gradle wrapper method and copy step during build. This would handle the case if users changes the version by preference.

This PR does not, and will not, cover the case when users upgrades gradle version from Android Studio. CLI build will undo those changes and replace gradle with the tools version.

This PR will not cover creating of the gradle wrapper on platform add.

@erisu erisu changed the title fix: move gradle wrapper from tools to platform root dir fix: copy gradle wrapper from tools to platform root dir Mar 17, 2025
@erisu erisu merged commit ff11f65 into apache:master Mar 18, 2025
14 checks passed
@erisu erisu deleted the feat/move-gradle-location branch March 18, 2025 01:55
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.

Impossible to open Project in Android Studio
3 participants