oshai / kotlin-logging

Lightweight Multiplatform logging framework for Kotlin. A convenient and performant logging facade.
Other
2.67k stars 113 forks source link

IR backend #126

Closed robertfmurdock closed 4 years ago

robertfmurdock commented 4 years ago

1.4.0 is out! Time to compile this in BOTH mode.

oshai commented 4 years ago

Hi @robertfmurdock can you please share more details on what exactly do you mean?

robertfmurdock commented 4 years ago

Oh sure. Kotlin 1.4 adds a new compiler backend called IR, and to support the new backend libs can be built in “both” mode to allow users to compile in whatever mode they choose. More info is available on the kotlin 1.4 update page

Sent from my iPhone

On Aug 18, 2020, at 6:55 PM, Ohad Shai notifications@github.com wrote:

 Hi @robertfmurdock can you please share more details on what exactly do you mean?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

robertfmurdock commented 4 years ago

I should be more specific - I’m talking about the JS IR. The JVM IR has no ‘both’ mode AFAIK, and thus isn’t great for libs as of now.

On Aug 18, 2020, at 6:55 PM, Ohad Shai notifications@github.com wrote:

Hi @robertfmurdock https://github.com/robertfmurdock can you please share more details on what exactly do you mean?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/MicroUtils/kotlin-logging/issues/126#issuecomment-675758977, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABPNPUU6MN5NALQO7EEA3NLSBMBHJANCNFSM4QEHAPOA.

altavir commented 4 years ago

I've currently hit the same problem. We are migrating libraries to Kotlin 1.4 and it is impossible to use kotlin-logging because it does not export IR artifacts. In order to make it work, you should update kotlin to 1.4 and select BOTH or IR option. I do not plan to support legacy mode, so IR mode only will be OK for me.

robertfmurdock commented 4 years ago

@oshai Looks like things are merged... but a new release has yet to be published! 😨

oshai commented 4 years ago

Yes, I encountered some issues while publishing. See #130 for status and details.

robertfmurdock commented 4 years ago

Ah, makes sense. I just looked and discovered I put in the bintray workaround in my own projects last year (to support the new Gradle metadata regarding Multiplatform projects), so I never saw this problem. 😅

On Sep 8, 2020, at 12:34 AM, Ohad Shai notifications@github.com wrote:

Yes, I encountered some issues while publishing. See #130 https://github.com/MicroUtils/kotlin-logging/issues/130 for status and details.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/MicroUtils/kotlin-logging/issues/126#issuecomment-688612454, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABPNPUWMLIUZGIDSZ4AXLQLSEWX47ANCNFSM4QEHAPOA.

oshai commented 4 years ago

If you can point to a similar example where it works it will be great.

robertfmurdock commented 4 years ago

https://github.com/robertfmurdock/testmints/blob/2b45f7c52ecb9b530467ca0eadced338a61795c4/build.gradle.kts#L109 https://github.com/robertfmurdock/testmints/blob/2b45f7c52ecb9b530467ca0eadced338a61795c4/build.gradle.kts#L109

This is the relevant section in my own libs. The migration docs for library authors include a specific workaround:

https://github.com/bintray/gradle-bintray-plugin/issues/229#issuecomment-473123891 https://github.com/bintray/gradle-bintray-plugin/issues/229#issuecomment-473123891

Hope that helps

On Sep 8, 2020, at 5:22 PM, Ohad Shai notifications@github.com wrote:

If you can point to a similar example where it works it will be great.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/MicroUtils/kotlin-logging/issues/126#issuecomment-689143379, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABPNPUR2KTWDFKUHE45KTPDSE2OC5ANCNFSM4QEHAPOA.

YarnSphere commented 4 years ago

I don't know what's missing, but I still can't use kotlin-logging v1.11.0 when compiling with IR.

I'll investigate and report here, but maybe this issue should be reopened as I don't think the PR fixed this.

altavir commented 4 years ago

The current build file does not use IR: https://github.com/MicroUtils/kotlin-logging/blob/800d89c93c0e7061d897ff09b71a2a65fc3fc4e4/build.gradle.kts#L59. It uses default legacy.

YarnSphere commented 4 years ago

@altavir https://github.com/MicroUtils/kotlin-logging/blob/800d89c93c0e7061d897ff09b71a2a65fc3fc4e4/gradle.properties#L2

altavir commented 4 years ago

I am not sure it is a correct way to do that. Anyway, gradle metadata modules are missing from the distribution so standard kotlin multiplatform techniques won't work. My fork is working fine though.

YarnSphere commented 4 years ago

I'm sure the kotlin.js.compiler flag is fine like that, I use it and lots of other projects do too: https://github.com/Kotlin/kotlinx.coroutines/blob/master/gradle.properties#L35

Both ways are fine.

So you're saying it's the publishing that has problems? Can you upstream the changes you made on your fork?

altavir commented 4 years ago

I can't upstream is since it breaks artifact naming (brings it in line with the current MPP scheme) and @oshai does not want to do that. I've also dropped the bugged bintray plugin in favor of maven-publish.

You can see the build source here: https://github.com/altavir/kotlin-logging/blob/master/build.gradle.kts And the artifact itself here: https://bintray.com/mipt-npm/dev/kotlin-logging/1.9.0-dev-npm-2 It also partially solves #45 since it uses the same sourceset for all native artifacts.

YarnSphere commented 4 years ago

Yeah, I was thinking about that too, the naming scheme is currently non-conventional and forces us to add multiple dependencies instead of just depending on it once in the common source.

@oshai, would you be willing to break backwards compatibility and release a version 2.0 with the current MPP naming scheme, functional JS-IR, and the partial fix for #45? I think this would be best in the long term; currently this project is not following MPP best practices and since MPP is still in alpha, I feel like it will only get worse over time whenever new changes to Kotlin MPP are introduced.

oshai commented 4 years ago

Generally, I am willing to break backwards compatibility. But when doing that we should consider few things:

I think you're welcome to create a PR, we can even test it with a snapshot, and then we can get smarter decisions.

YarnSphere commented 4 years ago

Multiplatform is in alpha, it means they might break it again later.

This is true, but at least in terms of naming packages, I don't think it'll break again.

Should we also change packages names? will it help users to avoid collisions or just confuse them more?

Yes, because MPP now has a "standard" for package names, I think it would be in the best interest of this project to follow it, I'm sure in the long term users will be expecting it, and anything other than the standard will feel odd. I'm not sure if this comes just from the naming, but in the latest Kotlin MPP I should only depend on kotlin-logging once, in the commonMain source set, the -jvm and -js variants would be depended upon automatically.

Do we have any other way to help users to avoid conflicts?

I think as long as we add a short paragraph in the README with a migration guide to version 2.0 it should be fine? Users shouldn't expect to simply increase a major version and everything to still work without any changes. Unless I'm missing your point here and you're asking something else. :D

How many people are affected by this issue?

Currently everyone using kotlin-logging can't compile their JS in IR mode, because all it takes is one dependency not providing their lib built with IR mode to break the build, afaik.

I think you're welcome to create a PR, we can even test it with a snapshot, and then we can get smarter decisions.

How would you like to proceed in terms of publishing? Stop using Bintray and go with maven-publish or try to work around Bintray's limitations? maven-publish seems to be the preferred way according to Kotlin docs, but you'd probably need to set up the infrastructure on your end, I'm not sure, I don't currently have any experience with publishing Kotlin projects, honestly.

@altavir, since you already have a working build, would you now be willing to push a PR with the changes?

altavir commented 4 years ago

It is possible to provide a single mpp dependency without changing the naming scheme. If one has gradle metadata, it does not matter what are actual names. It is also possible to provide aliases (never tried it though). Keeping the naming scheme requires some work though. I can create a PR that adheres to the new naming scheme rules (all I need is to copy relevant parts from my publishing plugin).

And answering @oshai question. artifact names have nothing to do with package names.

YarnSphere commented 4 years ago

It is possible to provide a single mpp dependency without changing the naming scheme. If one has gradle metadata, it does not matter what are actual names.

Yeah, thanks for the explanation. I assumed this was the case.

Keeping the naming scheme requires some work though.

Even if this was simple, imho I still think the naming scheme should be changed to adhere to MPP best practices. But, of course, this is up to @oshai to decide. :)

Thanks @altavir, I'll await your PR.

oshai commented 4 years ago

I don't know what's missing, but I still can't use kotlin-logging v1.11.0 when compiling with IR.

I'll investigate and report here, but maybe this issue should be reopened as I don't think the PR fixed this.

@nunocastromartins I just noticed you tried 1.11.0. Can you please also try 1.11.3 in case you haven't?

YarnSphere commented 4 years ago

@nunocastromartins I just noticed you tried 1.11.0. Can you please also try 1.11.3 in case you haven't?

No luck. Project won't build even without IR, was the same with 1.11.0 iirc.

oshai commented 4 years ago

Fixed in #145