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

Kotlin module support #7

Merged
merged 3 commits into from
Aug 29, 2024
Merged

Conversation

0xera
Copy link
Collaborator

@0xera 0xera commented Aug 27, 2024

Fixes #6

@0xera 0xera requested a review from natario1 August 28, 2024 10:04
Comment on lines +9 to +10
import org.apache.tools.zip.ZipEntry
import org.apache.tools.zip.ZipOutputStream
Copy link
Member

Choose a reason for hiding this comment

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

Can't we use Java's ZipOutputStream?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This comes from shadow jar dependency, because it is using in transform interface functions

Comment on lines +538 to +545
target.plugins.withId("org.gradle.maven-publish") {
target.tasks.withType(PublishToMavenRepository::class.java) {
val publication = publication
if (publication is DefaultMavenPublication) {
if (creationConfig.name == publication.component.get().name) {
dependsOn(greaseShadowTask)
}
}
Copy link
Member

Choose a reason for hiding this comment

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

This looks wrong to me. Can you explain what's your goal here? There should be a more a robust solution.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

There was problem with publication, because publish task doesnt wait grease tasks, so grease task will be executed after publication and final aar will be incorrect. Therefore i made publish task to execute after grease task for common cases

Copy link
Member

Choose a reason for hiding this comment

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

The main problem is that people should be able to rely on bundle AAR task in all scenarios, without tricks like this one which only solves the publication use-case. We can fix this later though, I'll open an issue with some ideas without using finalizedBy.

Another problem is that code above may crash at component.get(). You can do component.getOrNull() but then it may be added later and you don't catch that.

Maybe it'd be better to iterate over components?

project.components.matching { it.name == creationConfig.name }.all {
    tasks.matching { it.name == "generate${creationConfig.name.capitalize()}PomFileForPublication" }.configureEach {
        mustRunAfter(greaseShadowTask)
    }
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Maybe, but there is so many publish tasks and we can skip one of them. Also we are configuring grease after evaluation so i think everything should be good

@natario1 natario1 self-requested a review August 29, 2024 14:47
@natario1 natario1 merged commit b1105eb into deepmedia:main Aug 29, 2024
1 check passed
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.

It doesn't work with @Composable
2 participants