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

Support K2 #302

Open
ZacSweers opened this issue Aug 7, 2022 · 10 comments · Fixed by #305
Open

Support K2 #302

ZacSweers opened this issue Aug 7, 2022 · 10 comments · Fixed by #305

Comments

@ZacSweers
Copy link
Contributor

Currently the new K2 compiler cannot even be enabled in KCT tests due to its own registrar not supporting it

e: There are some plugins incompatible with K2 compiler:
  com.tschuchort.compiletesting.MainComponentRegistrar

It would be helpful to enable this so plugin authors can start testing their plugins.

@tschuchortdev
Copy link
Owner

K2 should work now on the latest snapshot release. Both the compilerPlugins and commandLineProcessors have to be empty to prevent the resources path (containing the MainCommandlineProcessor) from being added to the compilation's plugin classpath.

@ZacSweers
Copy link
Contributor Author

I commented on the PR as well - why not just make this project's plugin compatible? It's just a boolean property that needs to be overridden to return true

@tschuchortdev
Copy link
Owner

Oh, I didn't know that was an option. Sounds good!

@tschuchortdev
Copy link
Owner

Do you perhaps have some documentation on hand about what makes a plugin compatible/incompatible? The MainComponentRegistrar also registers KaptComponentRegistrar, so I need to know if KAPT is compatible with K2. This blog post suggests that KAPT is compatible, but it is not listed here.

@tschuchortdev tschuchortdev reopened this Aug 12, 2022
@ZacSweers
Copy link
Contributor Author

Do you perhaps have some documentation on hand about what makes a plugin compatible/incompatible

Plugins must opt-in by returning true to supportsK2 like so: https://github.com/ZacSweers/redacted-compiler-plugin/blob/z/firTesting/redacted-compiler-plugin/src/main/kotlin/dev/zacsweers/redacted/compiler/RedactedPlugin.kt#L34-L35

Kapt is not compatible with K2 and I'm not sure it ever will be. That said, I don't think that's a blocker. I think this plugin should only apply kapt if the test using it needs kapt, not always. Would actually be a nice perf boost for tests that don't need kapt

@ZacSweers
Copy link
Contributor Author

in much the same way that it doesn't automatically apply KSP, for instance

@tschuchortdev
Copy link
Owner

I think this plugin should only apply kapt if the test using it needs kapt, not always

It seems to me this is already the case (at least the project components don't get registered and I believe the extra call to K2JVMCompiler is skipped as well).

I've been looking at the Kotlin compiler source code a bit and this property you're talking about only appears in 1.7.20-beta. Current version is 1.7.10, so I won't be able to include it in the stable release. Can you make do with the workaround until then or do you need it to test your own plugins? In that case, I'll have to make a pre-release for 1.7.20-beta.

@ZacSweers
Copy link
Contributor Author

It seems to me this is already the case (at least the project components don't get registered and I believe the extra call to K2JVMCompiler is skipped as well).

I don't think so, it appears to always be registered here

only appears in 1.7.20-beta. Current version is 1.7.10, so I won't be able to include it in the stable release.

You can compile against 1.7.20-beta without forcing it on older consumers, no?

@tschuchortdev
Copy link
Owner

I don't think so, it appears to always be registered here

Yes, but if you go into that method, it has an early exit when no annotation processors are given 😃.

You can compile against 1.7.20-beta without forcing it on older consumers, no?

Perhaps. The additional supportsK2 property should not lead to binary incompatibilities, but there may be other changes. I'll have to check it.

@ZacSweers
Copy link
Contributor Author

I took a pass at it in #306. The compileOnly approach there to avoid imposing the version works, but as I've mentioned before I think this library should just tie versions to the kotlinc version it's built against.

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 a pull request may close this issue.

2 participants