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

Make Klib-related Gradle tasks public #204

Open
wants to merge 17 commits into
base: develop
Choose a base branch
from
Open

Conversation

fzhinkin
Copy link
Collaborator

@fzhinkin fzhinkin commented Mar 26, 2024

What was done:

  • cleaned up klib-related tasks API
  • rewrote code gluing up tasks together to utilize Gradle properties for proper dependency tracking
  • made tasks public
  • made some programmatic API classes used as tasks inputs serializable

Fixed #203

@fzhinkin fzhinkin added enhancement New feature or request klib labels Mar 27, 2024
@fzhinkin fzhinkin linked an issue Mar 27, 2024 that may be closed by this pull request
@fzhinkin fzhinkin marked this pull request as ready for review March 27, 2024 09:20
@fzhinkin fzhinkin requested a review from shanshin March 27, 2024 09:20
Base automatically changed from 199-disable-tasks-for-empty-projects to develop March 27, 2024 14:36
* Path to a resulting dump file.
*/
@get:InputFiles
@get:SkipWhenEmpty

Choose a reason for hiding this comment

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

Can there be situations where dumpFile is not specified for the target?

/**
* Path to a resulting dump file.
*/
@get:InputFiles

Choose a reason for hiding this comment

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

A rather ad-hoc way)
I think in such cases it is worth writing that InputFiles is used for a single file if it may not be present or the file may be empty

Copy link
Collaborator Author

@fzhinkin fzhinkin May 13, 2024

Choose a reason for hiding this comment

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

Updated KDoc with the details on why a file could be missing and what to expect then

src/main/kotlin/-Utils.kt Show resolved Hide resolved
import java.util.jar.JarFile
import javax.inject.Inject

public open class KotlinApiBuildTask @Inject constructor(
) : BuildTaskBase() {
@OutputFile
public lateinit var outputApiFile: File

Choose a reason for hiding this comment

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

Why File but not RegularFileProperty?
It seems that because of this, lateinit looks very unusual

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 change moves the property back from BuildTaskBase, so I left the same type. However, as this PR will break binary compatibility for the Compare task, it might be worth breaking it here as well and providing a more robust implementation in return.

Choose a reason for hiding this comment

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

You can try to add a reliable field, but maintain backward compatibility.
In BuildTaskBase

    @get:Internal
    public abstract var outputApiFile: File

In KotlinApiBuildTask

    @get:OutputFile
    ...
    public val realOutputApiFile: RegularFileProperty = project.objects.fileProperty()

    @get:Internal
    public override var outputApiFile: File
        get() = realOutputApiFile.get().asFile
        set(value) = realOutputApiFile.set(value)
  • I didn 't check it )

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The main issue here is that we definitely want to phase these old properties out and ask users to migrate to realXXX ones, but old properties already claimed good names :(

@PathSensitive(PathSensitivity.RELATIVE)
public lateinit var projectApiFile: File
@get:InputFiles
@get:SkipWhenEmpty

Choose a reason for hiding this comment

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

File can be empty or not specified?

onlyIf("There are no klibs compiled for the project") { hasCompilableTargets.get() }
strictValidation.set(extension.klib.strictValidation)
requiredTargets.addAll(supportedTargets())
inputAbiFile.set(klibApiDir.get().resolve(klibDumpFileName))

Choose a reason for hiding this comment

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

klibApiDir.map { it.resolve(...) } ?

*/
@get:InputFiles
@get:SkipWhenEmpty
public val dumpFile: RegularFileProperty

Choose a reason for hiding this comment

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

Provider<File>?
Properties is not very often used in nested types, in this case, you will not need to call objects.fileProperty()

signatureVersion = SerializableSignatureVersion(extension.klib.signatureVersion)
outputApiFile = apiBuildDir.resolve(klibDumpFileName)
this.target.set(target)
klibFile.from(project.provider { compilation.output.classesDirs })

Choose a reason for hiding this comment

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

ConfigurableFileCollection is a live collection, try use it directly klibFile.from(compilation.output.classesDirs )

@@ -636,29 +592,21 @@ private class KlibValidationPipelineBuilder(
}

private fun Project.unsupportedTargetDumpProxy(
compilation: KotlinCompilation<KotlinCommonOptions>,
@Suppress("UNUSED_PARAMETER") compilation: KotlinCompilation<KotlinCommonOptions>,

Choose a reason for hiding this comment

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

Why unused parameter is still present?

dumpFileName = klibDumpFileName
dependsOn(project.tasks.withType(KotlinKlibAbiBuildTask::class.java))
target.set(unsupportedTarget)
oldMergedKlibDump.set(klibApiDir.get().resolve(klibDumpFileName))

Choose a reason for hiding this comment

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

oldMergedKlibDump.fileProvider(klibApiDir.map { it.resolve(...) }

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request klib
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make KLib validation related tasks public
2 participants