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

Loading a specific resource from a module must use class from module #5008

Merged
merged 1 commit into from Jun 29, 2022

Conversation

arturbosch
Copy link
Member

@arturbosch arturbosch commented Jun 28, 2022

Image following scenario:

A tool (e.g. IntelliJ plugin) uses detekt-tooling to run detekt.
The tool does not include the detekt jars by default (no implementation("detekt-core") usage) but loads them dynamically.
For example the user can specify the detekt version and plugin jars. The tool downloads cli-all.jar and the plugin jars, loads them via a URLClassLoader, gets a DetektProvider.get(): Detekt instance and runs detekt.

Notice that the tool must use the detekt-tooling api which is not inside the URLClassLoader (!). ExtensionSpec was loaded via the AppClassLoader (ClassLoader which wants to load detekt dynamically where detekt-tooling is known). ExtensionSpec.javaClass.getResource won't find the default-detekt-config.yml file.

Conclusion: when loading detekt core and all plugins dynamically detekt's modules are not allowed to use the ClassLoader from detekt-tooling or detekt-api.

@github-actions github-actions bot added the core label Jun 28, 2022
@BraisGabin
Copy link
Member

To be honest I don't understand how all the resources are loaded. I have little experience in load code dynamically. Is there an easy way to test this to avoid regressions?

@schalkms
Copy link
Member

@BraisGabin there were some issues raised in the detekt-intellij-plugin repo that reveal this mentioned problem. For instance:
detekt/detekt-intellij-plugin#190

@BraisGabin
Copy link
Member

We can merge this for sure. I'm just wondering if there is a way to unit test it. I assume there isn't.

@BraisGabin BraisGabin merged commit 5c39d7d into main Jun 29, 2022
@BraisGabin BraisGabin deleted the load-resource-from-core branch June 29, 2022 10:42
@cortinico cortinico added this to the 1.21.0 milestone Jun 29, 2022
@arturbosch
Copy link
Member Author

arturbosch commented Jun 29, 2022

@BraisGabin there were some issues raised in the detekt-intellij-plugin repo that reveal this mentioned problem. For instance: detekt/detekt-intellij-plugin#190

Hm, maybe you linked a wrong issue but this issue seems to be more a detekt-core problem with Intellij 2022.2-EAP. Seems like there will be a breaking change in the Kotlin compiler potentially.

We can merge this for sure. I'm just wondering if there is a way to unit test it. I assume there isn't.

It should be possible to write an integration test for detekt-tooling with a wierd dependency on the detekt-core jar.
Maybe similar how our run cli with args test was done. I'm adding a task to my stack for a future PR :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants