Skip to content

Use Gradle KTS in new Android app projects by default - #154061

Merged
auto-submit[bot] merged 16 commits into
flutter:masterfrom
bartekpacia:gradle_kotlin_by_default
Oct 18, 2024
Merged

auto-submit[bot] merged 16 commits into
flutter:masterfrom
bartekpacia:gradle_kotlin_by_default

Conversation

@bartekpacia

Copy link
Copy Markdown
Member

This PR resolves #151166

Pre-launch Checklist

  • I read the [Contributor Guide] and followed the process outlined there for submitting PRs.
  • I read the [Tree Hygiene] wiki page, which explains my responsibilities.
  • I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement].
  • I signed the [CLA].
  • I listed at least one issue that this PR fixes in the description above.
  • I updated/added relevant documentation (doc comments with ///).
  • I added new tests to check the change I am making, or this PR is [test-exempt].
  • I followed the [breaking change policy] and added [Data Driven Fixes] where supported.
  • All existing and new tests are passing.

@github-actions github-actions Bot added the tool Affects the "flutter" command-line tool. See also t: labels. label Aug 24, 2024
@bartekpacia bartekpacia changed the title uUse Gradle KTS in new Android app projects by default Use Gradle KTS in new Android app projects by default Aug 24, 2024
@bartekpacia

Copy link
Copy Markdown
Member Author

should the plugins template be updated two ?

Not until #154125 gets merged to master and we wait a few days to get some confidence that it's not going to get reverted.

Comment on lines +470 to +482
static final List<RegExp> _declarativeKotlinPluginPatterns = <RegExp>[
RegExp('^\\s*id\\s*\\(?\\s*[\'"]kotlin-android[\'"]\\s*\\)?\\s*\$'),
RegExp('^\\s*id\\s*\\(?\\s*[\'"]org.jetbrains.kotlin.android[\'"]\\s*\\)?\\s*\$'),
];

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Here's a small DartPad playground I made to ensure I got the regexes right.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Please include some examples in a dart doc above this regex. I know we do not have them for other examples but I find it super helpful when reading the code.

@bartekpacia

Copy link
Copy Markdown
Member Author

No idea why Linux gradle_plugin_light_apk_test is failing.

@bartekpacia
bartekpacia marked this pull request as ready for review September 20, 2024 21:43
@reidbaker

Copy link
Copy Markdown
Contributor

out of band comms, this is ready for review

Comment on lines +470 to +482
static final List<RegExp> _declarativeKotlinPluginPatterns = <RegExp>[
RegExp('^\\s*id\\s*\\(?\\s*[\'"]kotlin-android[\'"]\\s*\\)?\\s*\$'),
RegExp('^\\s*id\\s*\\(?\\s*[\'"]org.jetbrains.kotlin.android[\'"]\\s*\\)?\\s*\$'),
];

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Please include some examples in a dart doc above this regex. I know we do not have them for other examples but I find it super helpful when reading the code.


kotlinOptions {
jvmTarget = JavaVersion.VERSION_1_8
jvmTarget = "1.8"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Out of curiosity why not use the constant and move to a string. We usually want to move away from string based types.

I ask because this value will get moved to java 11 shortly.

@bkonyi

bkonyi commented Oct 17, 2024

Copy link
Copy Markdown
Contributor

@bartekpacia just following up on this. Do you think you'll have time to address @reidbaker's comments? If not, this is probably something we can push over the finish line since it's a change we'd like to land.

@bartekpacia

Copy link
Copy Markdown
Member Author

hey @bkonyi - sure I'm happy to do it!

i was kinda distracted recently, sorry

@bartekpacia

Copy link
Copy Markdown
Member Author

done!

also rebased with master

@bartekpacia bartekpacia added the autosubmit Merge PR when tree becomes green via auto submit App label Oct 17, 2024

kotlinOptions {
jvmTarget = JavaVersion.VERSION_1_8
jvmTarget = "1.8"

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Should this also be JavaVersion.VERSION_1_8.toString(), like the other instance? I'd also like to avoid hardcoded strings here

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yup you're right. I missed it. I'll fix it tomorrow.

Feel free to push to my branch though and merge it.

@auto-submit auto-submit Bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Oct 17, 2024
String get rootPath => path.join(parent.path, name);
String get androidPath => path.join(rootPath, 'android');
String get iosPath => path.join(rootPath, 'ios');
File get appBuildFile {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Interesting that we have an independent FlutterProject here
(independent of

).

I don't think I was aware of this

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Do you think it's worth to create an issue to track this?

(I'm not sure)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'll take a look to try to understand why it exists and make an issue if it doesn't seem to me like it should

@bartekpacia

Copy link
Copy Markdown
Member Author

status: of course there were more tests that broke. Fixing them now.

@bartekpacia bartekpacia added the autosubmit Merge PR when tree becomes green via auto submit App label Oct 18, 2024
@auto-submit auto-submit Bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Oct 18, 2024
@auto-submit

auto-submit Bot commented Oct 18, 2024

Copy link
Copy Markdown
Contributor

auto label is removed for flutter/flutter/154061, due to - The status or check suite Mac dart_plugin_registry_test has failed. Please fix the issues identified (or deflake) before re-applying this label.

@bartekpacia bartekpacia added the autosubmit Merge PR when tree becomes green via auto submit App label Oct 18, 2024
@bartekpacia

bartekpacia commented Oct 18, 2024 •

Copy link
Copy Markdown
Member Author

auto label is removed for flutter/flutter/154061, due to - The status or check suite Mac dart_plugin_registry_test has failed. Please fix the issues identified (or deflake) before re-applying this label.

re-running fixed it ¯\_(ツ)_/¯

@auto-submit
auto-submit Bot merged commit d3c54a1 into flutter:master Oct 18, 2024
@bartekpacia
bartekpacia deleted the gradle_kotlin_by_default branch October 18, 2024 20:47
@QuncCccccc

Copy link
Copy Markdown
Contributor

Reason for revert: might be the reason that cause Framework tree to red

@QuncCccccc QuncCccccc added the revert Autorevert PR (with "Reason for revert:" comment) label Oct 18, 2024
auto-submit Bot pushed a commit that referenced this pull request Oct 18, 2024
@auto-submit auto-submit Bot removed the revert Autorevert PR (with "Reason for revert:" comment) label Oct 18, 2024
auto-submit Bot added a commit that referenced this pull request Oct 18, 2024
…)" (#157194)

Reverts: #154061
Initiated by: QuncCccccc
Reason for reverting: might be the reason that cause Framework tree to red
Original PR Author: bartekpacia

Reviewed By: {gmackall, reidbaker}

This change reverts the following previous change:
This PR resolves #151166
@gmackall

gmackall commented Oct 18, 2024 •

Copy link
Copy Markdown
Member

Oof, yes this test definition will need changing
https://github.com/flutter/flutter/blob/master/dev/devicelab/lib/framework/dependency_smoke_test_task_definition.dart#L112-L134

Perhaps these two tests
android_java17_dependency_smoke_tests and android_java11_dependency_smoke_tests should be marked with runIf logic to run on changes to android templates.

Also could have been avoided by #153399

@bartekpacia

Copy link
Copy Markdown
Member Author

@gmackall I'm on it

@gmackall

Copy link
Copy Markdown
Member

Perhaps these two tests android_java17_dependency_smoke_tests and android_java11_dependency_smoke_tests should be marked with runIf logic to run on changes to android templates.

Will attempt to do this in #157196

@bartekpacia

Copy link
Copy Markdown
Member Author

good idea, thanks! much appreciated

auto-submit Bot pushed a commit that referenced this pull request Oct 22, 2024
M97Chahboun pushed a commit to M97Chahboun/flutter that referenced this pull request Oct 30, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 12, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 13, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 13, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 6, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 7, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

autosubmit Merge PR when tree becomes green via auto submit App tool Affects the "flutter" command-line tool. See also t: labels.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Change language of Gradle scripts in templates from Groovy to Kotlin

5 participants