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

#181 - move ktfmt to use Worker API for classloader isolation #182

Merged
merged 9 commits into from
Aug 4, 2023

Conversation

cloudshiftchris
Copy link
Contributor

🚀 Description

This PR moves ktfmt to use the Worker API for class loader isolation to mitigate class path collisions with differing Kotlin/Gradle versions.

📄 Motivation and Context

Addresses #181 and possibly #157, and likely other latent issues cause by class path conflicts.

🧪 How Has This Been Tested?

All unit and integration tests updated to reflect this change.

📦 Types of changes

  • [x ] Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

✅ Checklist

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.

@cloudshiftchris
Copy link
Contributor Author

Notes on this comment:

I've been investigating isolating the KtFmt dependencies so that users could effectively specify the tool versions without having a 1:1 relationship between gradle plugin-ktfmt tool.

While this PR addresses running ktfmt in an isolated class loader, it doesn't directly provide the capability to alter the ktfmt version separate from the plugin, though it does lay the groundwork for it.

To do so requires addressing two challenges:

  1. Binary compatibility with currently supported and future ktfmt versions;
  2. Semantic compatibility with currently supported and future ktfmt versions;

Binary compatibility can be handled by providing adapter interfaces for versions of ktfmt that alter the public API (it's a small footprint, but does include things like constants, enumerations, etc). This can be seen, for example, in Spotless where ktlint / ktfmt versions are specified in a Gradle extensions thru to selecting an appropriate adapter, etc.

Semantic compatibility, in this context, refers to maintaining expected behaviour for current and future ktfmt versions. This is accomplished by having a robust test suite (large set of data to format, across different format styles / configurations, across different ktfmt versions), with deviations addressed by either the aforementioned adapters or 'fyi' documentation/release note updates.

Copy link
Owner

@cortinico cortinico left a comment

Choose a reason for hiding this comment

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

Amazing work @cloudshiftchris! Thank you very much for doing this 👍 really appreciate your time.

I've left several comments, which are mostly questions. Once this is all sorted out we can merge it and I can make a new release just after.

@cloudshiftchris
Copy link
Contributor Author

@cortinico have addressed all PR feedback; will create a couple issues for later cleanup:

  • source files are in java folder which is preventing use of Gradle Kotlin DSL, resulting in more verbose code;
  • revisit FormattingOptionsBean and how that is passed to tasks

Should be good-to-go for isolated class loader changes.

@cortinico cortinico enabled auto-merge (squash) August 4, 2023 14:15
@cortinico cortinico merged commit e811d0b into cortinico:main Aug 4, 2023
@cortinico
Copy link
Owner

will create a couple issues for later cleanup:

@cloudshiftchris do you need a new release with just this change or are you planning to follow-up on those items so we can make a release after?

@cloudshiftchris
Copy link
Contributor Author

@cortinico eventually those changes will go into a new release; not large/material enough to warrant a release themselves.

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