-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Fix debug module publication with shadow plugin #3357
Conversation
jar { | ||
setEnabled(false) | ||
manifest { | ||
attributes "Premain-Class": "kotlinx.coroutines.debug.AgentPremain" | ||
attributes "Can-Redefine-Classes": "true" | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I find this very counterintuitive: if a task is not enabled, why does its configuration contribute to anything? I propose to move the manifest
block to shadowJar
. If I understood correctly (and from a quick check of the resulting META-INF/MANIFEST.MF
in the published JAR), this shouldn't change the result, but it would look much more clear.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Checking further, I see that this line was lost from the manifest compared to the one published to Maven Central:
Class-Path: kotlinx-coroutines-core-jvm-1.6.3.jar jna-platform-5.9.0.jar
jna-5.9.0.jar kotlin-stdlib-jdk8-1.6.21.jar kotlin-stdlib-jdk7-1.6.21.
jar kotlin-stdlib-1.6.21.jar kotlin-stdlib-common-1.6.21.jar annotation
s-13.0.jar
However, deleting the locally-published repo and reverting my change, I can't make this line return. No idea what's going on.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shadow jar extends the original jar task. Moved it to JAR task, it seems like the resulting published JAR contains the correct manifest.
It still worth to check it during the release tho
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's what I was proposing to do, but then I noticed the Class-Path
line disappearing from the manifest.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The previous version also does not generate Class-Path
attribute.
This is a nice catch, though I cannot convince myself that it actually ever affected anything.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is what I observed as well, yes, just wasn't sure.
Could you please try using the debug module in a minimal project as a dependency and check that everything still works, just to be safe? We do have integration tests for using it as a Java agent, but not for just using DebugProbes
, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Restored it back, created #3361
Will push integration test soon as well
integration-testing/src/debugDynamicAgentTest/kotlin/DynamicAttachDebugTest.kt
Outdated
Show resolved
Hide resolved
…tachDebugTest.kt Co-authored-by: dkhalanskyjb <[email protected]>
…to baxter/upstream-flow-timeout * origin/baxter/upstream-flow-timeout: (328 commits) Commit API dump Cleanup API, update knit Fix typo in runTest method docs (Kotlin#3417) Update coroutines-and-channels.md (Kotlin#3410) chore: update the website's release step (Kotlin#3397) ktl-695 chore: support Dokka HTML customization (Kotlin#3388) update: KT-50122 adding kotlinx.dependencies Improve bump-version.sh (Kotlin#3365) Fix documentation for `DEBUG_PROPERTY_VALUE_OFF` (Kotlin#3389) feat: moving coroutines hands-on to docs (Kotlin#3369) Version 1.6.4 Improve CoroutineDispatcher documentation (Kotlin#3359) Update binary compatibility validator to 0.11.0 (Kotlin#3362) Add a scope for launching background work in tests (Kotlin#3348) Fix debug module publication with shadow plugin (Kotlin#3357) Comply with Subscriber rule 2.7 in the `await*` impl (Kotlin#3360) Update readme (Kotlin#3343) Reduce reachable references of disposed invokeOnTimeout handle (Kotlin#3353) breakleg; knit validation fix Additional comment in CoroutineScheduler ... # Conflicts: # README.md
It seems like Maven's BOM has never worked with shadow plugin in our previous releases: added
api
dependency was just too late.It has been changed in the latest shadow plugin release and it automatically promoted all
api
dependencies toruntime
(GradleUp/shadow#321), triggering #3345. Gradle builds were never affected though because they were primarily parsing Gradle metadata.Resulting debug module POM