JetBrains / markdown

Markdown parser written in kotlin
Apache License 2.0
682 stars 75 forks source link

Make Kotlin compileOnly dependency #16

Open matklad opened 7 years ago

matklad commented 7 years ago

Hi!

build.gradle specifies compile dependency on kotlin-runtime and kotlin-stdlib, version 1.0.3. It is propagated to runtime dependency as well, so any plugin which uses intellij-markdown, will then bundle kotlin-runtime and kotlin-stdlib jars. This happens for Intellij-rust. I believe it also affects https://github.com/JetBrains/intellij-plugins/tree/master/markdown though I have not checked that specifically.

And intellij-plugins should not bundle their own Kotlin jars: http://www.jetbrains.org/intellij/sdk/docs/tutorials/kotlin.html#kotlin-gradle-plugin

I believe this PR should fix the issue. An alternative would be to somehow exclude this jar in the plugins themselves.

valich commented 7 years ago

I believe this PR fixes the issue indeed, however I am not sure it should be fixed in the second level libraries from the IJ POV. If I understand how this works correctly, after this PR intellij-markdown will not work well with potential java clients. Kotlin may be provided at compile time, but after bundling this just won't work without any warning during the build.

I neither think this is a work for rust nor markdown IJ plugins. I think this is a work for intellij-gradle plugin to explicitly exclude any kotlin jar being a runtime dependency in clients because the restriction itself comes from IJ platform implementation (namely, the presence of Kotlin by default) and has nothing to do with plugin writers.

matklad commented 7 years ago

If I understand how this works correctly, after this PR intellij-markdown will not work well with potential java clients.

Yes, that is indeed the case!

I think this is a work for intellij-gradle plugin to explicitly exclude any kotlin jar being a runtime dependency in clients because the restriction itself comes from IJ platform implementation

Sounds reasonable. @zolotov what do you think?

For completeness sake, the jars can be excluded from the plugins with

dependencies {
    compile('org.jetbrains:markdown:0.1.12') {
        exclude module: 'kotlin-runtime'
        exclude module: 'kotlin-stdlib'
    }
}
valich commented 7 years ago

Regarding your last snippet, I would like to emphasize that this is not the particular directly imported module (markdown) problem rather than the general one. So I think something like that could be used to exclude kotlin-runtime/stdlib from every dependency.

zolotov commented 7 years ago

I think this is a work for intellij-gradle plugin to explicitly exclude any kotlin jar being a runtime dependency in clients because the restriction itself comes from IJ platform implementation

Sounds reasonable. @zolotov what do you think?

Not for me. gradle-intellij-plugin must not change any explicit dependencies added to the plugin.

The markdown plugin explicitly adds kotlin-dependency to compile (hence to runtime as well), it means that it requires exactly this version of kotlin and it will bundle exactly this version of kotlin despite IDE version it compiles against and it will use exactly this version of kotlin in runtime.

zolotov commented 7 years ago

because the restriction itself comes from IJ platform implementation

@valich what's the restriction?

valich commented 7 years ago

Hi @zolotov,

The markdown plugin explicitly adds kotlin-dependency to compile (hence to runtime as well), it means that it requires exactly this version of kotlin and it will bundle exactly this version of kotlin despite IDE version it compiles against

As far as I get the current Kotlin ABI compatibility status this is not necessary; also in the SDK tutorial which was already posted here quite the opposite is stated:

Please note that you should not include kotlin-runtime and kotlin-stdlib jars with your plugin because Kotlin guarantees backward- and forward- binary compatibility.

As long as I understand there might be incompatibilities theoretically, that will affect so many plugins (mainly the ones built outside of JB) so that this is undesirable.

because the restriction itself comes from IJ platform implementation

@valich what's the restriction?

Well, I suppose if different kotlin classes will be loaded by platform and the plugin this may lead to some CCE or smth similar. Not sure we have a public kotlin-using API at the moment.

By the way, I think @yole can shed some light on the matter?

zolotov commented 7 years ago

As far as I get the current Kotlin ABI compatibility status this is not necessary

What is not necessary? Explicitly adding kotlin dependencies? You're right, but this is not the point. The point is that since you have added these dependencies – remove them, make them compileOnly or deal with them somehow, it's all on you. How do you think gradle-intellij-plugin related to your decision to add kotlin dependency?

yole commented 7 years ago

Bundling a Kotlin runtime with a plugin will not cause conflicts (the Kotlin plugin does that because it uses a newer version of the runtime than the one bundled with IDEA), but is unnecessary and in general not recommended. (And actually because plugins can use a newer version of kotlin-runtime, it would be wrong for gradle-intellij-plugin to change the dependency to compileOnly automatically.)

valich commented 7 years ago

The point is that since you have added these dependencies – remove them, make them compileOnly or deal with them somehow, it's all on you. How do you think gradle-intellij-plugin related to your decision to add kotlin dependency?

Dealing with my Kotlin dependencies is problematic (not transparent) in the case of a Kotlinless parent project. But I am not talking about particularly my lib, this is a general problem. Currently any library written in Kotlin used in IJ plugin leads to the need to disable Kotlin dependencies in the build script. Consider a case when some really popular lib (antlr for example) or even corner case where all 3rd party libs are written in Kotlin — then all IJ plugins based on them will require removal of the Kotlin in the scripts. Then why not to do this in gradle-intellij plugin if all usages of the plugin do the same thing? It looks as just DRYing for me.

Obviously this approach breaks if there's an case when one can't put this in their plugin. (using gradle-intellij). Can you conceive one?

zolotov commented 7 years ago

Currently any library written in Kotlin used in IJ plugin leads to the need to disable Kotlin dependencies in the build script.

No, why is that? Without this statement the rest comment doesn't make much sense, sorry.

Then why not to do this in gradle-intellij plugin if all usages of the plugin do the same thing?

I said why – none of gradle-plugin should change explicitly added dependencies.

Can you conceive one?

You can extrapolate the example that Dmitry bring up. This is not the main reason why I won't implement this, though. The main reason is that it's not a responsibility of gradle-plugins.

valich commented 7 years ago

Currently any library written in Kotlin used in IJ plugin leads to the need to disable Kotlin dependencies in the build script.

No, why is that? Without this statement the rest comment doesn't make much sense, sorry.

I consider that having two versions of kotlin is what we're trying to avoid. AFAIU gradle does not allow having two versions of the same lib simultaneously in the resolved dependency list (and tries to change versions if any). Is it true that in the case of IJ plugin gradle does not know that IJ already has some kotlin inside? So it does not trigger any resolve mechanism.

I said why – none of gradle-plugin should change explicitly added dependencies.

But can it add its own dependency for example? May that change explicitly added dependencies' versions which is thus also a dependency change?

The main reason is that it's not a responsibility of gradle-plugins

How to solve this outside of the plugin? compileOnly does not work because it's an IJ-independent lib; I can't see a way to tell gradle to add the compiled sources only if Kotlin is not present in parent projects.

matklad commented 7 years ago

I would say that silently changing dependencies can be a little to magical. But it would be prudent to issue a warning (or maybe even fail a build by default) from the gradle-intellij-plugin if Kotlin is detected in the declared or transitive runtime dependencies.

I personally don't worry that I need to exclude Kotlin explicitly in my build.gradle. But I'd love to be sure that it accidentally doesn't end in plugin's jar if, for example, I add a new dependency: I've uncovered the original problem in this issue by accident.

zolotov commented 7 years ago

I don't understand you at all. So let me answer your questions and then you'll tell me one more time what problem you're trying to solve.

AFAIU gradle does not allow having two versions of the same lib simultaneously in the resolved dependency list (and tries to change versions if any).

No, gradle allows it

Is it true that in the case of IJ plugin gradle does not know that IJ already has some kotlin inside?

No, it knows

But can it add its own dependency for example?

Yes, it can and it does this almost always. Except the case when you add kotlin dependencies explicitly.

May that change explicitly added dependencies' versions which is thus also a dependency change?

No. Explicitly dependencies should not be modified by plugin. Period.

How to solve this outside of the plugin?

I don't understand what you're going to solve outside the plugin. Please describe your problem one more time with as many details as you can.