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

Set Main-Class attr for KMP 2.1.0 or above #1337

Merged
merged 10 commits into from
Mar 12, 2025
Merged

Conversation

Goooler
Copy link
Member

@Goooler Goooler commented Mar 12, 2025


  • CHANGELOG's "Unreleased" section has been updated, if applicable.

Sorry, something went wrong.

gemini-code-assist[bot]

This comment was marked as outdated.

Goooler added 4 commits March 12, 2025 12:24
…n-run

// Conflicts:
//	src/functionalTest/kotlin/com/github/jengelman/gradle/plugins/shadow/KmpPluginTest.kt
@Goooler Goooler marked this pull request as ready for review March 12, 2025 05:20
@Goooler Goooler changed the title Set mainClass from kotlin.jvm().mainRun Set mainClass for KMP 2.1.0 or above Mar 12, 2025
@Goooler Goooler changed the title Set mainClass for KMP 2.1.0 or above Set Main-Class attr for KMP 2.1.0 or above Mar 12, 2025
@Goooler Goooler enabled auto-merge (squash) March 12, 2025 05:29
@Goooler Goooler disabled auto-merge March 12, 2025 05:29
@Goooler Goooler enabled auto-merge (squash) March 12, 2025 05:29
@Goooler Goooler requested a review from Copilot March 12, 2025 05:32
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR sets the Main-Class attribute for KMP 2.1.0 or above by updating the Kotlin plugin documentation and the CHANGELOG.

  • Updated the Kotlin DSL example in docs/kmp-plugin/README.md to use the new mainRun configuration.
  • Updated the CHANGELOG to reflect the change.

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

File Description
docs/kmp-plugin/README.md Updated Kotlin DSL example to set the Main-Class attribute using the mainRun block
docs/changes/README.md Added a new entry describing the Main-Class attribute update

Goooler and others added 2 commits March 12, 2025 13:33
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
@Goooler Goooler disabled auto-merge March 12, 2025 05:35
@Goooler
Copy link
Member Author

Goooler commented Mar 12, 2025

/gemini review

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request introduces the functionality to set the Main-Class attribute for Kotlin Multiplatform (KMP) projects using Kotlin version 2.1.0 or above. The changes include updates to documentation, functional tests, and the core plugin logic. Overall, the changes seem well-structured and address the intended issue. However, there are a few areas that could benefit from further refinement.

Summary of Findings

  • Kotlin Version Check: The Kotlin version check in ShadowKmpPlugin.kt relies on KgpVersion.DEFAULT < KgpVersion.KOTLIN_2_1. It's crucial to ensure that this comparison accurately reflects the intended behavior and handles potential edge cases or future versioning changes correctly. Consider adding a comment explaining the rationale behind this specific comparison.
  • Manifest Attribute Injection: The code injects the Main-Class attribute into the manifest within a doFirst block. While this approach works, it might be beneficial to explore alternative methods for setting manifest attributes, potentially during task configuration, to ensure consistency and avoid potential race conditions.
  • Test Coverage: The functional test added in KmpPluginTest.kt covers the basic scenario. Consider adding more test cases to cover different scenarios, such as when the main class is not explicitly set or when there are conflicts with existing manifest attributes.

Merge Readiness

The pull request is almost ready for merging. Addressing the comments regarding the Kotlin version check, manifest attribute injection, and test coverage would improve the robustness and maintainability of the code. I am unable to directly approve this pull request, and recommend that others review and approve this code before merging. I recommend that the pull request not be merged until the high severity issues are addressed.

@Goooler Goooler requested a review from Copilot March 12, 2025 05:57

Choose a reason for hiding this comment

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

Pull Request Overview

This PR updates the documentation to showcase the new main class configuration for KMP projects, with examples for both Kotlin DSL and Groovy DSL usage.

  • Updated Kotlin DSL snippet in docs/kmp-plugin/README.md to demonstrate setting the main class using the new mainRun API.
  • Updated Groovy DSL snippet in docs/kmp-plugin/README.md similarly.
  • Revised the changelog in docs/changes/README.md to reflect the new configuration option.

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.

File Description
docs/kmp-plugin/README.md Updated DSL examples to use mainRun for setting the main class.
docs/changes/README.md Added changelog entry for the new Main-Class attribute feature.
Comments suppressed due to low confidence (1)

docs/kmp-plugin/README.md:19

  • The comment references 'Kotlin 2.1.0' while the PR title indicates this change is for KMP 2.1.0; please clarify the version reference to avoid confusion.
// Optionally, set the main class for `runJvm`, it's available from Kotlin 2.1.0
@Goooler
Copy link
Member Author

Goooler commented Mar 12, 2025

/gemini review

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

The pull request introduces the functionality to set the Main-Class attribute for Kotlin Multiplatform (KMP) projects using Kotlin version 2.1.0 or above. This is achieved by leveraging the mainRun configuration within the Kotlin DSL. The changes include updates to documentation, functional tests, and the core plugin logic. Overall, the changes seem well-structured and address the intended functionality.

Summary of Findings

  • Kotlin Version Check: The code correctly checks for Kotlin version 2.1 or above before configuring the Main-Class attribute. This ensures compatibility with older Kotlin versions.
  • Functional Test Coverage: The addition of a parameterized test (canSetMainClassAttribute) enhances the test suite by verifying the functionality with and without the shadow attribute.
  • Serialization Issue: The code includes a comment addressing a potential serialization issue with KotlinJvmRun. This indicates awareness of potential problems and proactive mitigation.

Merge Readiness

The pull request appears to be well-structured and addresses the intended functionality. The addition of a functional test and the handling of a potential serialization issue are commendable. However, the potential issue with the realClass variable being null or empty should be addressed. I am unable to directly approve this pull request, and recommend that others review and approve this code before merging. Given the high severity comment, I recommend that the pull request not be merged until this is addressed.

@Goooler Goooler enabled auto-merge (squash) March 12, 2025 06:01
@Goooler Goooler merged commit b5bcde9 into main Mar 12, 2025
7 checks passed
@Goooler Goooler deleted the g/20250312/compat-main-run branch March 12, 2025 06:05
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.

None yet

1 participant