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.

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

Use byte-buddy-dep instead of byte-buddy #4400

Merged
merged 11 commits into from
Oct 19, 2021

Conversation

laurit
Copy link
Contributor

@laurit laurit commented Oct 15, 2021

Replaces byte-buddy which includes shaded asm with byte-buddy-dep that just depends on asm. As byte-buddy includes only the asm classes it needs it is missing some that we might need. For example for #4370 we need SerialVersionUIDAdder which is not included in byte-buddy.

Comment on lines +278 to +281
- name: Local publish gradle plugin
run: ../gradlew publishToMavenLocal -x javadoc
working-directory: gradle-plugins

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this won't be needed now that I finally got #4248 working

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll remove this and the same in pr.yml when #4248 is merged

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 going to merge this PR since #4248 is getting reworked, and I'll take care of it in that PR

Copy link
Contributor

@iNikem iNikem left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this SerialVersionUIDAdder classes needed only for our gradle plugin? Compile time? Or runtime? Why do we have to switch dependency for the remaining project?


val extractBbGradlePlugin by registering(Copy::class) {
from(zipTree(bbGradlePlugin.files.first { it.name.contains("byte-buddy-gradle-plugin") })) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Honestly, this looks like a hack. Why exactly do we need to unzip a dependency jar file into our classes folder instead of putting jar file to a classpath?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for pointing this out. I think I figured out how to get rid of this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To answer your other question we could use SerialVersionUIDAdder for virtual field implementation which is in javaagent-tooling but so is io.opentelemetry.javaagent.tooling.muzzle.generation.MuzzleCodeGenerator which, as far as I can tell, is the actual class that does the work for gradle plugin.
Instead of replacing byte-buddy with byte-buddy-dep we could copy SerialVersionUIDAdder class from asm or we could shade our own copy of asm-commons that includes all classes under the same package as bytebuddy uses. I went for replacing byte-buddy with byte-buddy-dep because in my opinion this gives us the clearest path forward in case we need to include more asm classes.

@mateuszrzeszutek
Copy link
Member

Is this SerialVersionUIDAdder classes needed only for our gradle plugin? Compile time? Or runtime? Why do we have to switch dependency for the remaining project?

It's needed in the runtime -- we'll want to use it in the VirtualField real field injector class.

@trask trask merged commit 17a85bb into open-telemetry:main Oct 19, 2021
RashmiRam pushed a commit to RashmiRam/opentelemetry-auto-instr-java that referenced this pull request May 23, 2022
* Use byte-buddy-dep instead of byte-buddy

* print stacktrace on examples failure

* try to fix gradle plugins

* try to fix extension build

* try to fix extension build

* try to fix extension build

* try to fix extension build

* try removing mavenLocal

* add mavenLocal plugin repository

* publish gradle-plugins to mavenLocal for examples ci build

* Fix bytebuddy exclusion
@laurit laurit deleted the byte-buddy-dep branch July 6, 2023 17:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants