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

Sync gradle-plugins version with main project #4248

Merged
merged 10 commits into from
Oct 29, 2021

Conversation

trask
Copy link
Member

@trask trask commented Sep 29, 2021

I think releasing these simultaneously is important for the next release, since we are doing composite build + dependency substitution now, it seems like without this it would be easy for the substituted local dependency to drift from the published version that extension authors use(?)

btw, I tried but failed to get nebula working in gradle-plugins because it's not in the git root, and git.root can't be relative, and what I really want is to set it in the gradle file so that you don't have to always specify it on the command line, but I couldn't figure out how to do that...

@anuraaga
Copy link
Contributor

For the plugins, since they aren't published on the main build and the automatic tag based versioning isn't needed there, does it make sense to just read the system property we set directly?

@trask
Copy link
Member Author

trask commented Oct 5, 2021

For the plugins, since they aren't published on the main build and the automatic tag based versioning isn't needed there, does it make sense to just read the system property we set directly?

@anuraaga here's an attempt at this: 222935a, the examples projects still require some hard-coding or coordination (composite? 😬)

# javadoc task fails sporadically fetching https://docs.oracle.com/javase/8/docs/api/
run: ./gradlew publishToMavenLocal -x javadoc

- name: Local publish of gradle plugins
run: ../gradlew publishToMavenLocal -Pversion=1.7.0-alpha-SNAPSHOT
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like the new feature is this publishToMavenLocal, which we don't like due to the version requirement (and not being able to nebula the version).

It may work to do something like this in the examples settings.gradle

if (gradle.startParameters["includeGradlePlubins"]) {
  includeBuild("../gradle-plugins") {
  dependencySubstitution {
    substitute(module("io.opentelemetry.instrumentation.muzzle-generation:io.opentelemetry.instrumentation.muzzle-generation.gradle.plugin")).using(project(":"))
    substitute(module("io.opentelemetry.instrumentation.muzzle-check:io.opentelemetry.instrumentation.muzzle-check.gradle.plugin")).using(project(":"))
  }
}

a bit similar to https://github.com/open-telemetry/opentelemetry-java-instrumentation/blob/main/conventions/settings.gradle.kts#L1 but allowing examples to still build independently too.

@trask trask force-pushed the sync-gradle-plugins-version branch from 222935a to 6401984 Compare October 17, 2021 23:58
@trask trask marked this pull request as ready for review October 18, 2021 00:21
@trask
Copy link
Member Author

trask commented Oct 18, 2021

it seems to be working 🤞🤞

i figure this will break the release build somehow, but we need a release to test it, so may as well be this one 😭🤷‍♂️

@trask
Copy link
Member Author

trask commented Oct 18, 2021

ptal at the changes to the release instructions in case you have idea how to avoid any steps

}

if (this.rootProject.name != "gradle-plugins") {
includeBuild("../../gradle-plugins") {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need to include gradle-plugins to all projects? Plugins are only used from conventions project and they are already included there.

Copy link
Member Author

Choose a reason for hiding this comment

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

this file is only used when building the examples projects

@trask
Copy link
Member Author

trask commented Oct 19, 2021

Discussed in SIG meeting, using composite build defeats part of the purpose of the examples build which ensures (locally) published artifacts work. So instead, decided to remove nebula and manually increment version since that should resolve, and seems needed for #3516 anyways.

@trask trask marked this pull request as draft October 19, 2021 20:58
@trask trask force-pushed the sync-gradle-plugins-version branch 2 times, most recently from d5335e8 to 6a2f68b Compare October 19, 2021 21:28
@trask trask force-pushed the sync-gradle-plugins-version branch from 6a2f68b to 5c53d70 Compare October 19, 2021 22:20
@trask trask mentioned this pull request Oct 19, 2021
@trask trask marked this pull request as ready for review October 20, 2021 02:27
@trask
Copy link
Member Author

trask commented Oct 20, 2021

This is ready for a fresh look

RELEASING.md Outdated
Comment on lines 38 to 39
* build.gradle.kts
* gradle-plugins/build.gradle.kts
Copy link
Member

Choose a reason for hiding this comment

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

Nit: WDYT about keeping the version in some version.properties file that could be reused by all projects involved in the composite build?

@trask
Copy link
Member Author

trask commented Oct 20, 2021

@anuraaga or @iNikem would be great to get one more review on this, thx

@@ -253,8 +253,6 @@ jobs:

examples:
runs-on: ubuntu-latest
# When we make PR against pre-release branch examples may point to yet-unpublished version
if: github.base_ref == 'main'
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this not needed any more?

Copy link
Member Author

Choose a reason for hiding this comment

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

I was thinking now that we will remove -SNAPSHOT from artifact version at the same time, the locally published artifacts won't have -SNAPSHOT, and so this will work... I could definitely be wrong...

@@ -29,8 +29,8 @@ dependencies {
implementation(gradleApi())
implementation(localGroovy())

implementation("io.opentelemetry.instrumentation.muzzle-generation:io.opentelemetry.instrumentation.muzzle-generation.gradle.plugin:0.8.0")
implementation("io.opentelemetry.instrumentation.muzzle-check:io.opentelemetry.instrumentation.muzzle-check.gradle.plugin:0.8.0")
implementation("io.opentelemetry.instrumentation.muzzle-generation:io.opentelemetry.instrumentation.muzzle-generation.gradle.plugin:${version}")
Copy link
Contributor

Choose a reason for hiding this comment

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

conventions always include plugins as composite, probably can skip version altogether.

Copy link
Member Author

Choose a reason for hiding this comment

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

nice 👍

@trask trask closed this Oct 24, 2021
@trask trask deleted the sync-gradle-plugins-version branch October 24, 2021 20:37
@trask
Copy link
Member Author

trask commented Oct 28, 2021

I must have hit close instead of merge by accident 😩

@trask trask restored the sync-gradle-plugins-version branch October 28, 2021 00:46
@trask trask reopened this Oct 28, 2021
@mateuszrzeszutek
Copy link
Member

What's the plan this PR? Is there anything that's blocking it from being merged?

I just tried to update our distro to SNAPSHOT versions and I got the following error when running muzzle codegen:

Caused by: java.lang.AbstractMethodError: Receiver class io.opentelemetry.javaagent.tooling.muzzle.generation.MuzzleCodeGenerator does not define or inherit an implementation of the resolved method 'abstract net.bytebuddy.jar.asm.ClassVisitor wrap(net.bytebuddy.description.type.TypeDescription, net.bytebuddy.jar.asm.ClassVisitor, net.bytebuddy.implementation.Implementation$Context, net.bytebuddy.pool.TypePool, net.bytebuddy.description.field.FieldList, net.bytebuddy.description.method.MethodList, int, int)' of interface net.bytebuddy.asm.AsmVisitorWrapper.

We need to either merge this PR or bump the muzzle plugins' version to 0.9.0-SNAPSHOT to fix that.

@trask trask merged commit 37e24ec into open-telemetry:main Oct 29, 2021
@trask trask deleted the sync-gradle-plugins-version branch October 29, 2021 17:34
@trask
Copy link
Member Author

trask commented Oct 29, 2021

What's the plan this PR? Is there anything that's blocking it from being merged?

sorry, I thought I had merged it a while ago 😭

RashmiRam pushed a commit to RashmiRam/opentelemetry-auto-instr-java that referenced this pull request May 23, 2022
* Sync gradle-plugins version with main project

* More fixes

* Can rely on examples build against all branches now?

* Use common version.gradle.kts

* Update doc

* Simplify
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.

None yet

4 participants