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 normal filesystem-based Gradle paths. #2332

Merged
merged 4 commits into from
Dec 18, 2020

Conversation

anuraaga
Copy link
Contributor

I recently found that I had to rely on a sneaky ordering constraint to handle our name mangling

https://github.com/open-telemetry/opentelemetry-java/blob/master/settings.gradle#L73

And in #2295 I found replace(logging- to be causing problems too. I think in general using the filesystem for Gradle paths is more intuitive (just type what's on the path) and it brings more consistency with the java instrumentation repo. So finally have replaced the project name mangling with group-scoped archivesBaseName mangling instead.

Also fixed an issue where our BOM contains non-published artifacts too - not super-harmful but a bit weird.

@trask
Copy link
Member

trask commented Dec 17, 2020

it brings more consistency with the java instrumentation repo

nice

@anuraaga anuraaga closed this Dec 17, 2020
@anuraaga anuraaga reopened this Dec 17, 2020
@@ -0,0 +1,7 @@
subprojects {
Copy link
Member

Choose a reason for hiding this comment

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

Whoa, this is an annoying bug. And an old one 😃

How about we rename the directories instead?

":opentelemetry-bom",
":opentelemetry-perf-harness"

rootProject.children.each {
Copy link
Member

Choose a reason for hiding this comment

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

I would be happy to get rid of this, but the archiveFilename workaround does not look much better all-in-all IMHO. Maybe we can rename the directories so we can get rid of that workaround too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah my comment might have been confusing, the workaround is only setting the group so group + name are unique.

The rest is applying our convention for archive names in a generic way. Another alternative is to not apply rules and just set archivesBaseName in each Gradle file. It's what I do for my own projects. Don't have a particular preference for either though.

I do like the current folder names since the gradle commands are much shorter and easier to execute. Gradle zsh completion has never worked well for me so always have to type everything out.

Copy link
Member

Choose a reason for hiding this comment

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

Oh, I see. So basically we are doing the same with the archiveBasenames now that we used to to with the project directory names before, but distributed over multiple files.

I think I would prefer either having everything in one place (maybe the top-level build.gradle) or, like you said, have each project's build.gradle just set it's own name. As it is now in this PR, it's kind of a mixture of the two.

@jkwatson
Copy link
Contributor

I have no strong opinion one way or another on whether we specify the published names in one place, or individually. Just happy someone else is wrangling the gradle. 😜

@anuraaga
Copy link
Contributor Author

I'd like to avoid the top-level, I think it does end up with some ordering constraints like we already have (https://github.com/open-telemetry/opentelemetry-java/blob/master/settings.gradle#L73 deeper projects must be configured before non-deep projects). This approach takes advantage of normal Gradle ordering rather than having to worry about it ourselves.

I don't think we add groups often enough for this approach to be too weird, so I'm thinking of sticking with it unless there's a stronger push towards per-build-file setting of the archive name :)

Copy link
Contributor

@jkwatson jkwatson 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 being the #1 gradle-wrangler!

@anuraaga anuraaga merged commit 6954d1f into open-telemetry:master Dec 18, 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.

4 participants