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

Rename spring instrumentation modules #1125

Merged
merged 5 commits into from
Sep 9, 2020

Conversation

dengliming
Copy link
Member

closes #1106

@trask
Copy link
Member

trask commented Aug 30, 2020

It looks like this is failing:

./gradlew :instrumentation:spring:spring-boot-autoconfigure:compileJava

for an interesting reason.

Gradle is performing dependency substitution, thinking that the library modules are all the same since they have the same name:

./gradlew :instrumentation:spring:spring-boot-autoconfigure:dependencies

...
project :instrumentation:spring:spring-web-3.1:library -> project :instrumentation:spring:spring-webflux-5.0:library
project :instrumentation:spring:spring-webmvc-3.1:library -> project :instrumentation:spring:spring-webflux-5.0:library
...

I found this on google that seems to confirm this diagnosis: https://discuss.gradle.org/t/dependency-substitution-wrong-with-more-than-one-sub-project-with-same-name/7253/8

One option for this would be to give them unique names, e.g. adding the bolded line below into settings.gradle

def setBuildFile(project) {
  if (['auto', 'library', 'testing'].contains(project.projectDir.name)) {
    project.name = "${project.projectDir.parentFile.name}-${project.projectDir.name}"
    project.buildFileName = "${project.projectDir.parentFile.name}-${project.projectDir.name}.gradle"
  } else {
    project.buildFileName = "${project.name}.gradle"
  }
  project.children.each {
    setBuildFile(it)
  }
}

Another option would be to update standard folder structure, e.g.

instrumentation ->
    ...
    yarpc-1.0 ->
        yarpc-1.0-auto
            yarpc-1.0-auto.gradle
        yarpc-1.0-library
            yarpc-1.0-library.gradle
        yarpc-1.0-testing
            yarpc-1.0-testing.gradle

@anuraaga @iNikem any preference?

@anuraaga
Copy link
Contributor

I think second option is better to make sure gradle tasks are clear (match folder name). A third option that comes to mind that's a questionable workaround is to do similar name mangling as option 1 but for group instead of project. And in publish.gradle compute the correct group for maven publishing instead of using the default. group is not really used during development so using it to work around can let us have shorter folders and still publish correctly IIUC, which could be nice.

For reference, here's the official=tracking issue (was so happy when Gradle 6.2 almost fixed it! :()
gradle/gradle#847

@trask
Copy link
Member

trask commented Aug 31, 2020

I do type those gradle task names a lot, so probably would vote for the shorter names hack (your option 3), and hopefully this will be a temporary workaround (thanks for the link to the official gradle issue for this). Let's see if @iNikem has a preference.

@iNikem
Copy link
Contributor

iNikem commented Aug 31, 2020

gradle/gradle#847 (comment)

When we publish, we use only 2 groups: io.opentelemetry.instrumentation and io.opentelemetry.instrumentation.auto, right? And every sub-project's name has either opentelemetry or opentelemetry-auto prefix, right? If we assign the same groupId and artifactId to the projects right from the start and not only during publishing, that should solve the current problem, right?

@anuraaga
Copy link
Contributor

Hmm - I thought Gradle was using it's own project.group / project.name for this differentiation. Not sure setting the maven publication like that eagerly would change it, though worth a try maybe.

@iNikem
Copy link
Contributor

iNikem commented Aug 31, 2020

No-no, not maven publication. I thought specifically about Gradle's own groupId and project name

@anuraaga
Copy link
Contributor

If we set the project name to a long name, tasks / project dependencies also have to use that long name, I think it's basically @trask's option 1.

@iNikem
Copy link
Contributor

iNikem commented Aug 31, 2020

Yes, it is essentially the same as Trask's suggestion. I find it confusing if we have different names during development and when published. If we want shorter names during development, we can omit opentelemetry prefixes.

As usual, I will support that option, which results in smaller build scripts :) Less code is always better for me :)

@anuraaga
Copy link
Contributor

anuraaga commented Sep 3, 2020

@dengliming I fixed the dupe problem in #1145 - can you try rebasing and see if it works ok?

@dengliming
Copy link
Member Author

dengliming commented Sep 3, 2020

@anuraaga Is there anything else that needs to be modified?

> Task :instrumentation:shadowJar FAILED
ex
org.gradle.api.file.DuplicateFileCopyingException: Encountered duplicate path "opentelemetry-auto-auto-0.1.0-SNAPSHOT.jar" during copy operation configured with DuplicatesStrategy.FAIL
	at org.gradle.api.internal.file.copy.DuplicateHandlingCopyActionDecorator.lambda$execute$0(DuplicateHandlingCopyActionDecorator.java:59)
	at org.gradle.api.internal.file.copy.CopyFileVisitorImpl.processFile(CopyFileVisitorImpl.java:64)
	at org.gradle.api.internal.file.copy.CopyFileVisitorImpl.visitFile(CopyFileVisitorImpl.java:48)
	at org.gradle.api.internal.file.collections.DirectoryFileTree.processSingleFile(DirectoryFileTree.java:138)

@anuraaga
Copy link
Contributor

anuraaga commented Sep 4, 2020

Sorry, I found some copy-paste among or auto/xxx.gradle files that I've finally moved into the shared location in #1169. I checked with your branch that the command works after it.

@dengliming
Copy link
Member Author

dengliming commented Sep 4, 2020

@anuraaga Can you rerun the CI to confirm these?

@iNikem
Copy link
Contributor

iNikem commented Sep 9, 2020

@dengliming please rebase the PR

Copy link
Member

@trask trask left a comment

Choose a reason for hiding this comment

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

Thanks for moving us forward with this!

Copy link
Contributor

@anuraaga anuraaga left a comment

Choose a reason for hiding this comment

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

Thanks for dealing with our config issues!

@anuraaga anuraaga merged commit 4959aa7 into open-telemetry:master Sep 9, 2020
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.

Rename spring instrumentation modules a bit more
4 participants