Skip to content

Conversation

shubham7109
Copy link
Collaborator

@shubham7109 shubham7109 commented Aug 18, 2025

Related to issue: #5708

Description:

PR to migrate and wire all convention plugins into the Toolkit and Microapp modules.

Pre-merge Checklist

@shubham7109 shubham7109 changed the title Add all Toolkit convention migration changes Wire convention plugins into Toolkit and Microapp modules Aug 18, 2025
@shubham7109 shubham7109 self-assigned this Aug 18, 2025
@shubham7109 shubham7109 requested a review from sorenoid August 18, 2025 18:50
Copy link
Collaborator

@sorenoid sorenoid left a comment

Choose a reason for hiding this comment

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

wow that's a lot of work! I have noted some things I noticed.

*
* @param target The Gradle Project to which the dependency will be added.
*/
fun configureArcGISMapsDependencies(target: Project) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

this needs to configure the sdk as an api or implementation dependency depending on how it is used. Most if not all of the released components have the sdk as an api dependency.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Seems like microapps were only configured to use an implementation dependency, should we be safe to use an api dependency throughout? (toolkit & microapps) Since we don't manage publishing of the microapps it shouldn't change anything in the gradle builds.

version("mapsSdk", versionAndBuild)
library("mapsSdk", "com.esri", "arcgis-maps-kotlin").versionRef("mapsSdk")
}
}
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think that the logic below here that generates project names in your original branch needs to be reverted because it misnames some projects.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah seems like few microapps do not follow the naming pattern (-). I will add this to the consideration list, reverted to original for now.

* @param target The Gradle Project to which the dependency will be added.
*/
fun configureArcGISMapsDependencies(target: Project) {
target.dependencies {
Copy link
Collaborator

Choose a reason for hiding this comment

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

this logic needs to take into account finalBuild as well.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added custom logic to ignore buildNumber if gradle property finalBuild is used. 👍

@@ -1,7 +1,7 @@
[versions]

# ArcGIS Maps SDK for Kotlin version
arcgisMapsKotlinVersion = "300.0.0-4689"
arcgisMapsKotlinVersion = "300.0.0-4695"
Copy link
Collaborator

Choose a reason for hiding this comment

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

this should just be the last released version, 200.8.0, so that builds by github users outside of our network will work.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If we plan to eventually merge these changes to the Toolkit v.next, I would suppose only contributors would be cloning the branch, i.e. we would need the configured SDK version to be a 300.x daily build. In sample's we typically use a recent build updated in our v.next libs.

When releasing to main(during v.next merge) we update this arcgisMapsKotlinVersion in the libs to point to the final build version, ensuring for Github users outside of our network the project works.

Comment on lines -69 to -71
tasks.withType<Test> {
enabled = false
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

this seems like it was lost here and in a couple of app projects?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks, added back for :basemapgallery & :popup modules. 👍

Comment on lines -48 to -49
public final fun copy (Lcom/arcgismaps/httpcore/authentication/IapSignIn;)Lcom/arcgismaps/toolkit/authentication/BrowserAuthenticationChallenge$IapSignIn;
public static synthetic fun copy$default (Lcom/arcgismaps/toolkit/authentication/BrowserAuthenticationChallenge$IapSignIn;Lcom/arcgismaps/httpcore/authentication/IapSignIn;ILjava/lang/Object;)Lcom/arcgismaps/toolkit/authentication/BrowserAuthenticationChallenge$IapSignIn;
Copy link
Collaborator Author

@shubham7109 shubham7109 Aug 19, 2025

Choose a reason for hiding this comment

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

This change seems to have been surfaced due to the compiler arg -Xconsistent-data-class-copy-visibility used in the ArcGISMapsKotlinToolkitConventionPlugin. Should this be an acceptable .api change?

@shubham7109 shubham7109 requested a review from sorenoid August 19, 2025 19:43
shubham7109 and others added 10 commits August 19, 2025 16:17
* Bump dependency versions in libs.versions.toml

* bumps versions and updates additional api file

* update to use kotlin block

* removed deprecated api

* merges ech drawable folder

* delete outdated folder

* replae bock to use jvm toolchain

* removes compileOptions block

* removes jvmtarget

* update latest agp version

* update api file

* Delete buildSrc/.kotlin/errors/errors-1756399796794.log

* removes empty lines

* revert changes to remembersavable

* updates dokka to v2

* update supress deprecations
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.

3 participants