-
Notifications
You must be signed in to change notification settings - Fork 55
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
base: develop
Are you sure you want to change the base?
Conversation
e94154b
to
bb48561
Compare
bb48561
to
25efc08
Compare
25efc08
to
5a03695
Compare
import java.util.jar.JarFile | ||
import javax.inject.Inject | ||
|
||
public open class KotlinApiBuildTask @Inject constructor( | ||
) : BuildTaskBase() { | ||
@OutputFile | ||
public lateinit var outputApiFile: File |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 )
There was a problem hiding this comment.
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 :(
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will changing property type break a lot? Because if we don't change it now, it will stay that way for a long time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few weeks ago I had an impression that there are a lot of projects relying on these properties. After checking it once again, it seems like I was wrong and there are only a couple of projects on GH relying on it.
Originally, I was reluctant to make breaking changes as I wanted to limit the scope of the release to KLib support only. But as there are some other breaking changes already, it might be worth changing this part too and provide a migration guide for those who use corresponding properties.
As it was previously discussed elsewhere, currently BCV is mostly concerned with its own source compatibility as there are no other projects (beside Adam's BCV-MU) extending the BCV, so it should be more or less fine to change property types now.
2b7b900
to
4de527a
Compare
Also, reworked error reporting for the compare task.
- SyncFile is no longer cachable - removed KotlinApiCompareTask's ctor
8b0f562
to
d8271ab
Compare
import org.gradle.api.tasks.Input | ||
import org.gradle.api.tasks.Internal | ||
import org.gradle.api.tasks.OutputFile | ||
import java.io.File | ||
|
||
|
||
public abstract class BuildTaskBase : DefaultTask() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we link a public task so tightly to a specific extension?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These tasks are tightly bound to BCV anyway. The thing that worries me more is implicit initialization of properties, but I'm not sure that making initialization explicit and moving it to BinaryCompatibilityValidatorPlugin
will make a big difference for now
What was done:
Fixed #203