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: move gradle wrapper from tools to platform root dir #1781

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

erisu
Copy link
Member

@erisu erisu commented Feb 19, 2025

Motivation and Context

Move the gradle wrapper file creation from platforms/android/tools to platforms/android

Fixes #1760

Description

  • Updated nodejs side to make sure path removed tools.
  • removed tools/settings.gradle. Appears not needed.

Testing

  • Created a Cordova Project
  • Added Platform
  • Built Android Platform
  • Verified that the gradle-wrapper files were created in platforms/android
  • Verified that the Project Structure settings in AS showed a Gradle version.
  • Verified that the build tools in AS settings had found the wrapper's gradle-wrapper.properties

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

All modified and coverable lines are covered by tests ✅

Project coverage is 72.46%. Comparing base (8f458b0) to head (c1fb457).

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1781      +/-   ##
==========================================
- Coverage   72.50%   72.46%   -0.05%     
==========================================
  Files          23       23              
  Lines        1837     1834       -3     
==========================================
- Hits         1332     1329       -3     
  Misses        505      505              

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

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