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

Fixed multiple jvm targets #62

Merged
merged 8 commits into from
Oct 4, 2021

Conversation

pdvrieze
Copy link
Contributor

@pdvrieze pdvrieze commented Jul 2, 2021

This is a rework of the multiple-jvm-targets branch that actually works. The testing is "fixed" and the implementation is fixed. Note that this is not 100% compatible with existing projects as it uses target-specific subdirectories (only for multiplatform projects).

@qwwdfsad qwwdfsad self-requested a review July 5, 2021 08:02
@qwwdfsad
Copy link
Collaborator

qwwdfsad commented Jul 5, 2021

Thanks! Currently I'm busy with other things, but I'll try to return to this PR by the end of this week

qwwdfsad and others added 4 commits August 30, 2021 14:22
works by:
- renaming the tasks to include the target (except for jvm - for some compatibility)
- storing/building the api in subdirectories named after the target

Note that for single-target projects the names are not changed.

Author:     Paul de Vrieze <[email protected]>
…or kotlin multi

platform projects, just have two collector tasks "apiDump" and "apiCheck" that
depend on the  platform specific versions. It improves consistency, compatibility
and usability.
@pdvrieze pdvrieze force-pushed the fixed-multiple-jvm-targets branch from d7a9356 to ad0bdad Compare August 30, 2021 13:29
@pdvrieze
Copy link
Contributor Author

The pull request has been rebased upon 0.7.1

Copy link
Collaborator

@qwwdfsad qwwdfsad left a comment

Choose a reason for hiding this comment

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

Sorry for the delay!

The build does not pass as the plugin now creates the same task twice for any multiplatform project with a single JVM source set, this definitely should not be the case. In fact, it's impossible to apply it to any multiplatform project

src/main/kotlin/BinaryCompatibilityValidatorPlugin.kt Outdated Show resolved Hide resolved
…droid target. When

there is only one jvm/android target, don't create target specific api dirs. A future version could use the extension to make the behaviour configurable.
@pdvrieze
Copy link
Contributor Author

Sorry for the delay!

The build does not pass as the plugin now creates the same task twice for any multiplatform project with a single JVM source set, this definitely should not be the case. In fact, it's impossible to apply it to any multiplatform project

This should be fixed in the new pull request.

Copy link
Collaborator

@qwwdfsad qwwdfsad left a comment

Choose a reason for hiding this comment

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

Great job, thanks!

Please address few minor comments and then the PR will be ready to go: I will do a few minor stylistic cleanups and will release 0.8.0-RC next week

build.gradle.kts Outdated
@@ -36,6 +36,9 @@ tasks.register<Test>("functionalTest") {
}
tasks.check { dependsOn(tasks["functionalTest"]) }

// Hack (from guidance on gradle forums) needed to handle optional plugin dependencies
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you please add a link to the source of the suggestion?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

build.gradle.kts Outdated Show resolved Hide resolved
src/main/kotlin/BinaryCompatibilityValidatorPlugin.kt Outdated Show resolved Hide resolved
…nal dependencies

used as classpath for the plugin under test. This is needed for optional dependencies
(as used by this plugin) that would normally be found through other applied plugins
(testkit isolates the classpath, unlike regular gradle).
@pdvrieze
Copy link
Contributor Author

pdvrieze commented Oct 3, 2021

All three changes have been processed in the latest push. I found a better source for the code, and documented what is happening much better.

@qwwdfsad qwwdfsad self-requested a review October 4, 2021 14:52
Copy link
Collaborator

@qwwdfsad qwwdfsad left a comment

Choose a reason for hiding this comment

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

That's a great job, thanks for taking care of it!

@qwwdfsad qwwdfsad merged commit 6a3180b into Kotlin:master Oct 4, 2021
@pdvrieze pdvrieze deleted the fixed-multiple-jvm-targets branch October 5, 2021 18:55
shanshin pushed a commit to JetBrains/kotlin that referenced this pull request Oct 28, 2024
 Make the plugin work with Kotlin multiplatform for multiple targets:

* Rename tasks to include the target (except for the JVM - for backwards compatibility)
* Rather than having separate tasks where some use the original name, just have two collector tasks "apiDump" and "apiCheck" that depend on the platform-specific versions. It improves consistency, compatibility, and usability.
* Store and build .api files in subdirectories named after the target

Author: Paul de Vrieze <[email protected]>
Co-authored-by: Vsevolod Tolstopyatov <[email protected]>
Pull request Kotlin/binary-compatibility-validator#62
shanshin pushed a commit to JetBrains/kotlin that referenced this pull request Dec 3, 2024
 Make the plugin work with Kotlin multiplatform for multiple targets:

* Rename tasks to include the target (except for the JVM - for backwards compatibility)
* Rather than having separate tasks where some use the original name, just have two collector tasks "apiDump" and "apiCheck" that depend on the platform-specific versions. It improves consistency, compatibility, and usability.
* Store and build .api files in subdirectories named after the target

Author: Paul de Vrieze <[email protected]>
Co-authored-by: Vsevolod Tolstopyatov <[email protected]>
Pull request Kotlin/binary-compatibility-validator#62
shanshin pushed a commit to JetBrains/kotlin that referenced this pull request Dec 13, 2024
 Make the plugin work with Kotlin multiplatform for multiple targets:

* Rename tasks to include the target (except for the JVM - for backwards compatibility)
* Rather than having separate tasks where some use the original name, just have two collector tasks "apiDump" and "apiCheck" that depend on the platform-specific versions. It improves consistency, compatibility, and usability.
* Store and build .api files in subdirectories named after the target

Author: Paul de Vrieze <[email protected]>
Co-authored-by: Vsevolod Tolstopyatov <[email protected]>
Pull request Kotlin/binary-compatibility-validator#62
shanshin pushed a commit to JetBrains/kotlin that referenced this pull request Dec 23, 2024
 Make the plugin work with Kotlin multiplatform for multiple targets:

* Rename tasks to include the target (except for the JVM - for backwards compatibility)
* Rather than having separate tasks where some use the original name, just have two collector tasks "apiDump" and "apiCheck" that depend on the platform-specific versions. It improves consistency, compatibility, and usability.
* Store and build .api files in subdirectories named after the target

Author: Paul de Vrieze <[email protected]>
Co-authored-by: Vsevolod Tolstopyatov <[email protected]>
Pull request Kotlin/binary-compatibility-validator#62

Moved from Kotlin/binary-compatibility-validator@6a3180b
KotlinBuild pushed a commit to JetBrains/kotlin that referenced this pull request Jan 3, 2025
 Make the plugin work with Kotlin multiplatform for multiple targets:

* Rename tasks to include the target (except for the JVM - for backwards compatibility)
* Rather than having separate tasks where some use the original name, just have two collector tasks "apiDump" and "apiCheck" that depend on the platform-specific versions. It improves consistency, compatibility, and usability.
* Store and build .api files in subdirectories named after the target

Author: Paul de Vrieze <[email protected]>
Co-authored-by: Vsevolod Tolstopyatov <[email protected]>
Pull request Kotlin/binary-compatibility-validator#62

Moved from Kotlin/binary-compatibility-validator@6a3180b
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.

2 participants