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.

Sign up for GitHub

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

Jump to bottom

IR backend #126

Closed
robertfmurdock opened this issue Aug 18, 2020 · 23 comments · Fixed by #139
Closed

IR backend #126

robertfmurdock opened this issue Aug 18, 2020 · 23 comments · Fixed by #139

Comments

@robertfmurdock
Copy link
Contributor

1.4.0 is out! Time to compile this in BOTH mode.
@oshai
Copy link
Owner

oshai commented Aug 18, 2020

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

@robertfmurdock
Copy link
Contributor Author

robertfmurdock commented Aug 18, 2020 via email

@robertfmurdock
Copy link
Contributor Author

robertfmurdock commented Aug 18, 2020 via email

@altavir
Copy link
Contributor

altavir commented Aug 19, 2020

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
Copy link
Contributor Author

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

@oshai
Copy link
Owner

oshai commented Sep 8, 2020

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

@robertfmurdock
Copy link
Contributor Author

robertfmurdock commented Sep 8, 2020 via email

@oshai
Copy link
Owner

oshai commented Sep 8, 2020

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

@robertfmurdock
Copy link
Contributor Author

robertfmurdock commented Sep 8, 2020 via email

@YarnSphere YarnSphere mentioned this issue Sep 13, 2020
Merged
@YarnSphere
Copy link
Contributor

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
Copy link
Contributor

altavir commented Sep 14, 2020

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
Copy link
Contributor

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

@altavir
Copy link
Contributor

altavir commented Sep 14, 2020

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
Copy link
Contributor

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
Copy link
Contributor

altavir commented Sep 14, 2020

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
Copy link
Contributor

YarnSphere commented Sep 14, 2020 •
edited
Loading

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
Copy link
Owner

oshai commented Sep 14, 2020

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

  • Multiplatform is in alpha, it means they might break it again later.
  • Should we also change packages names? will it help users to avoid collisions or just confuse them more?
  • Do we have any other way to help users to avoid conflicts?
  • How many people are affected by this issue?

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
Copy link
Contributor

YarnSphere commented Sep 14, 2020 •
edited
Loading

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
Copy link
Contributor

altavir commented Sep 14, 2020

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
Copy link
Contributor

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
Copy link
Owner

oshai commented Sep 15, 2020

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?

@oshai oshai reopened this Sep 15, 2020
@oshai oshai mentioned this issue Sep 15, 2020
Closed
@YarnSphere
Copy link
Contributor

@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.

@altavir altavir mentioned this issue Sep 15, 2020
Merged
@oshai
Copy link
Owner

oshai commented Sep 15, 2020

Fixed in #145

@oshai oshai closed this as completed Sep 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Support JS-IR backend
4 participants
@oshai @robertfmurdock @YarnSphere @altavir