Specify Automatic-Module-Name for kotlinx-coroutines-core #4320
Conversation
Definitely would vote for "common" or "metadata" over "trampoline". |
dkhalanskyjb
left a comment
dkhalanskyjb
left a comment
There was a problem hiding this comment.
I can confirm that the reproducer provided at #3842 no longer has the described issue after these changes.
My understanding of Java modules is surface-level, so please double-check my understanding. Right now, both kotlinx-coroutines-core and kotlinx-coroutines-core-jvm have the Java module name name kotlinx.coroutines.core; core-jvm specifies it explicitly, and -core just gets it automatically. If someone depends on core, they also depend on core-jvm, so any mentions of kotlinx.coroutines.core in Java module specifications will now correctly resolve to core-jvm (and not to both), but in addition to that, kotlinx.coroutines.core.trampoline will refer to a module containing nothing of use, so no one has any reason to write kotlinx.coroutines.core.trampoline in their code, ever. Right?
If so, the name I'd prefer would be scary and highlight the uselessness of the module. Something like kotlinx.coroutines.core.artifact_disambiguating_module.
README.md
Outdated
| <dependency> | ||
| <groupId>org.jetbrains.kotlinx</groupId> | ||
| <artifactId>kotlinx-coroutines-core</artifactId> | ||
| <artifactId>kotlinx-coroutines-core-jvm</artifactId> |
There was a problem hiding this comment.
In my opinion, -jvm is visual noise in this case and doesn't actually mean anything. Of course it's JVM, we're mentioning a Kotlin project in a Maven file. I understand that there's an extra dependency resolution step, but does it have any non-negligible effect?
There was a problem hiding this comment.
I understand that there's an extra dependency resolution step, but does it have any non-negligible effect?
Just trying to make the world better by saving ~130KB of networking traffic for each resolution when the artifact is not cached locally yet. :)
There was a problem hiding this comment.
However, you're also adding 4 bytes to every load of the README file or the title page of kotlinx.coroutines; also, 4 bytes are going to be added to every version of README.md in the git repository starting from this PR (to be fair, those blob objects are compressed, so this is less critical).
There was a problem hiding this comment.
Okay, let's put the discussion about a recommended artifact name aside and solve the problem with auto-module names first.
I'll revert the README change.
Yes, you're absolutely correct.
That'd work for me. |
README.md
Outdated
| <dependency> | ||
| <groupId>org.jetbrains.kotlinx</groupId> | ||
| <artifactId>kotlinx-coroutines-core</artifactId> | ||
| <artifactId>kotlinx-coroutines-core-jvm</artifactId> |
There was a problem hiding this comment.
However, you're also adding 4 bytes to every load of the README file or the title page of kotlinx.coroutines; also, 4 bytes are going to be added to every version of README.md in the git repository starting from this PR (to be fair, those blob objects are compressed, so this is less critical).
|
@fzhinkin, hi! Are we doing this? |
|
@dkhalanskyjb, sorry, I missed your question. Not now, I'm still about to figure out how to proceed with it for all our libraries. |
fzhinkin
changed the title
Fixes #3842 # Conflicts: # README.md # kotlinx-coroutines-core/build.gradle.kts
fzhinkin
force-pushed
the
3842-resolve-module-conflict-name
branch
from
a15ceb3 to
4b5875c
Compare
|
The same issue existed for |
fzhinkin
changed the title
| manifest { | ||
| attributes("Automatic-Module-Name" to "kotlinx.coroutines.test.artifact_disambiguating_module") | ||
| } | ||
| } |
There was a problem hiding this comment.
Since this logic affects all multiplatform projects, would kotlin-multiplatform-conventions be the better place to put it in?
Open questions are:
kotlinx.coroutines.core.metadatawill be better.Should we continue suggesting to usekotlinx-coroutines-coredependency in the readme file? I don't see any immediate benefits, but I see a downside:-coredependency adds an extra dep resolution step.Fixes #3842