Skip to content

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