Klib support by fzhinkin · Pull Request #183 · Kotlin/binary-compatibility-validator

fzhinkin · 2024-01-31T11:48:21Z

This PR introduces KLib ABI validation support in the BCV plugin.

The brief overview of the design and decisions made is in the corresponding design doc: https://github.com/Kotlin/binary-compatibility-validator/blob/50ebf8b00492d754732e47453b4d8d0351f33baa/docs/design/KLibSupport.md

Providing a programmatic API similar to the kotlinx.validation.api API for working with JVM ABI is planned, but this PR does not include it.

Besides the design doc mentioned above, the main entry points for reviewing are

binary-compatibility-validator/src/main/kotlin/BinaryCompatibilityValidatorPlugin.kt

Line 370 in cac5669

private class KlibValidationPipelineBuilder(

where all the Gradle tasks are configured and

binary-compatibility-validator/src/main/kotlin/klib/KlibAbiDumpFileMerger.kt

Line 77 in cac5669

internal class KlibAbiDumpMerger {

where the current dump merging logic reside.

…r messages

Input klib dump files were not tracked as merger dependencies, so the merged dump was not updated when apiDump was re-executed after sources update

…test

sandwwraith

I didn't look at the implementation itself, but I decided to post comments now, because they were pending for too long

sandwwraith · 2024-02-14T17:50:59Z

+    /**
+     * Specify which version of signature KLib ABI dump should contain.
+     */
+    public var signatureVersion: Int = 2


Why this setting need to be exposed to end users? Can't we infer it from klib file itself?

We can, that's for sure. But when the default/latest version will change, old dumps will be invalid. So there should be a way to specify a particular version.
But here we should probably use some latest-version-available alias instead of the concrete version 2.

I don't get it. If the klib version changes, it means .klib.api file format (not actual signatures) would also change? That looks undesirable. Probably we should have some intermediate parser layer that would provide stable output.

Also, if klib version changes to 3, but user has signatureVersion = 2 it means that we parse klib v3 but output should look like 2. Which brings us again to some middle-layer transformation.

A klib may have multiple signature versions. For example, each declaration may have both v1 and v2 signatures.
The signature is a part of a dump we're producing, so when it changes, the dump also change.

Probably we should have some intermediate parser layer that would provide stable output.

Not sure how we can do that. Signatures are part of a declaration as a linker looks up for symbols by their signatures. And there may be no direct mapping between different signature versions and a declaration they belong to.

Also, if klib version changes to 3, but user has signatureVersion = 2 it means that we parse klib v3 but output should look like 2. Which brings us again to some middle-layer transformation.

We parse the same klib, but instead of dumping v2 signature, will dump v3 signature.

A klib's dump with v1 signature:
https://github.com/Kotlin/binary-compatibility-validator/blob/f5fdacf4d2e9548144f522aa2924742ac4e473f5/src/functionalTest/resources/examples/classes/TopLevelDeclarations.klib.v1.dump
The dump of the same klib using v2 signature:
https://github.com/Kotlin/binary-compatibility-validator/blob/f5fdacf4d2e9548144f522aa2924742ac4e473f5/src/functionalTest/resources/examples/classes/TopLevelDeclarations.klib.dump

So, v3 actually can't link with v2 because they're incompatible? But newer klibs will have both 3 and 2 sets for backwards compatibility?

So, v3 actually can't link with v2 because they're incompatible?

Yes, the linker won't be able to lookup symbols.

But newer klibs will have both 3 and 2 sets for backwards compatibility?

There's no v3 on the horizon right now, so it's hard to tell.

The docs here should guide user when to rely on the default value and when to set it to a fixed value.

Set the null as a default, mentioned that a user should not care about the option by default.

@ilya-g @sandwwraith are you ok with how the version is currently declared and documented?

sandwwraith · 2024-02-14T18:02:18Z

+     * By default, ABI dumped only for supported files will be validated. This option makes validation behavior
+     * stricter and treats having unsupported targets as an error.
+     */
+    public var strictValidation: Boolean = false


Shouldn't these values be of type Property<Boolean> to support some Gradle configuration feature? Was it configuration cache or something else, @shanshin ?

Configuration cache seems to work fine with primitive values. But if there's a case where Properties are better, I can change it (note that a parent class, ApiValidationExtension don't use Property too).

Co-authored-by: Leonid Startsev <sandwwraith@users.noreply.github.com>

… parser

fzhinkin · 2024-03-07T16:54:00Z

What's new:

added public API for managing merged dumps
Gradle tasks were rewritten to use that API
supported dumps generated for klibs with multiple native targets inside (hello, stdlib)
changes KlibTarget serialized form
get rid of some options affecting plugin and the API
fixed dump filter to use java class name format
added tests for the API
updated the design doc

- fixed naming - simplified some code constructions - swapped the order of KlibTarget fields - updated merged dump's header - removed unsupported targets from target hierarchy

…ure of a node being more or less common

Co-authored-by: ilya-g <ilya.gorbunov@jetbrains.com>

ilya-g

Forgot to submit the last portion of comments

Added experimental Klib ABI dump validation support. Validation could be performed using the Gradle plugin which provides almost the same validation and dump workflow as for JVM ABI validation. Also, a public API was added to allow users who don't want or can't use the plugin to implement their own workflows. --------- Co-authored-by: Leonid Startsev <sandwwraith@users.noreply.github.com> Co-authored-by: ilya-g <ilya.gorbunov@jetbrains.com> Pull request Kotlin/binary-compatibility-validator#183

Added experimental Klib ABI dump validation support. Validation could be performed using the Gradle plugin which provides almost the same validation and dump workflow as for JVM ABI validation. Also, a public API was added to allow users who don't want or can't use the plugin to implement their own workflows. --------- Co-authored-by: Leonid Startsev <sandwwraith@users.noreply.github.com> Co-authored-by: ilya-g <ilya.gorbunov@jetbrains.com> Pull request Kotlin/binary-compatibility-validator#183 Moved from Kotlin/binary-compatibility-validator@ad1bea6

fzhinkin linked an issue Jan 31, 2024 that may be closed by this pull request

Validate KLIB ABI compatibility on Multiplatform #149

Closed

fzhinkin force-pushed the klib-support branch from f80179f to 0a2d44f Compare February 5, 2024 09:50

fzhinkin marked this pull request as ready for review February 5, 2024 11:10

fzhinkin requested review from ilya-g and sandwwraith February 5, 2024 11:10

fzhinkin added the enhancement New feature or request label Feb 5, 2024

fzhinkin requested a review from shanshin February 5, 2024 11:17

fzhinkin added 13 commits February 6, 2024 16:14

Introduce target hierarchy mirroring default template from KMP

3c57419

Implement KLib dump merger

4e50f6d

Added Klib-related settings

ba998f0

Added a more precise Copy task

51f7756

Implemented Klib validation

c824926

Rebased to current dev

cb87220

Added a design doc

91903af

Cleanup

5a6cf13

Mark an experimental API with a dedicated annotation

d58121c

Cleanup

34313b1

Add some kdoc

4bc9d2a

Improved test coverage

a987909

Described a klib validation pipeline, fixed task names, improved erro…

5ba0dbe

…r messages

fzhinkin force-pushed the klib-support branch from cac5669 to 5ba0dbe Compare February 6, 2024 15:14

fzhinkin added 2 commits February 9, 2024 11:54

Fixed klib merge task's dependencies

9d9f1e7

Input klib dump files were not tracked as merger dependencies, so the merged dump was not updated when apiDump was re-executed after sources update

Added shortcuts to reduce the amount of boilerplate code in the klib …

cf65ddf

…test

shanshin approved these changes Feb 16, 2024

View reviewed changes

Fixed a test

3f93275

fzhinkin commented Feb 21, 2024

View reviewed changes

Comment thread docs/design/KLibSupport.md Outdated

fzhinkin commented Feb 21, 2024

View reviewed changes

Comment thread src/main/kotlin/klib/KlibAbiDumpFileMerger.kt Outdated

Store children decls as a map, get rid of a separate cache

6b17087

sandwwraith reviewed Feb 22, 2024

View reviewed changes

Fixed typos

f5fdacf

Co-authored-by: Leonid Startsev <sandwwraith@users.noreply.github.com>

fzhinkin added 3 commits March 7, 2024 12:11

Renaming and doc update

f51611d

Added public API prototype and switched Gradle tasks to it, fixed the…

9aa5be8

… parser

Improved kdoc and added samples

28cc85a

fzhinkin added 6 commits March 7, 2024 18:35

Update README

68ce7af

Update docs

4f4512e

Update API dump and target aliasing rule

ba9fcf1

Improved test coverage, clarified some corner cases

5e01cf4

Fixed typo in a filename

ba29177

Update the design doc

dd776f2

ilya-g reviewed Mar 11, 2024

View reviewed changes

fzhinkin added 3 commits March 12, 2024 11:28

Addressed review comments and suggestions

489eefa

- fixed naming - simplified some code constructions - swapped the order of KlibTarget fields - updated merged dump's header - removed unsupported targets from target hierarchy

Use a length of path from a target hierarchy root to a node as a meas…

a68f661

…ure of a node being more or less common

Consolidated all klib dump related classes in a single package

20b7fd9

fzhinkin requested a review from ilya-g March 12, 2024 10:58

Use root locale instead of a default one

4b19820

ilya-g reviewed Mar 12, 2024

View reviewed changes

Comment thread src/main/kotlin/api/klib/KlibDumpFilters.kt Outdated

ilya-g reviewed Mar 12, 2024

View reviewed changes

Comment thread src/main/kotlin/api/klib/KlibDump.kt Outdated

Comment thread src/main/kotlin/api/klib/KlibDump.kt Outdated

Comment thread src/main/kotlin/api/klib/KlibDump.kt Outdated

Comment thread src/main/kotlin/api/klib/KlibAbiDumpFileMerger.kt

fzhinkin and others added 4 commits March 12, 2024 17:50

Rephrased kdoc

077c7ec

Co-authored-by: ilya-g <ilya.gorbunov@jetbrains.com>

Reworked target group aliasing

c1c9921

Decapitalized L in KLib

081fdb0

Update api dump

4f63bae

ilya-g approved these changes Mar 15, 2024

View reviewed changes

Comment thread src/main/kotlin/api/klib/KlibDump.kt Outdated

Comment thread src/main/kotlin/api/klib/KlibDump.kt Outdated

Comment thread src/main/kotlin/api/klib/KlibAbiDumpFileMerger.kt Outdated

Improved kdoc, cleaned up the code

259ad7c

fzhinkin merged commit ad1bea6 into develop Mar 18, 2024

Uh oh!

Conversation

fzhinkin commented Jan 31, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sandwwraith left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fzhinkin Feb 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fzhinkin commented Mar 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ilya-g left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

fzhinkin commented Jan 31, 2024 •

edited

Loading

fzhinkin Feb 22, 2024 •

edited

Loading

fzhinkin commented Mar 7, 2024 •

edited

Loading