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

Write release notes for Gradle Module Metadata change #6604

Closed
cpovirk opened this issue Jun 28, 2023 · 3 comments
Closed

Write release notes for Gradle Module Metadata change #6604

cpovirk opened this issue Jun 28, 2023 · 3 comments
Assignees
Labels
P2 package=general type=documentation Documentation that is other than for an API

Comments

@cpovirk
Copy link
Member

cpovirk commented Jun 28, 2023

@jjohannes, thanks for offering to have a look and fill in what I'm missing. Here's what I've come up with. You can post suggestions for edits, or you can copy the whole thing and post a comment with your full, revised version.

Gradle Module Metadata

The Gradle team has contributed a metadata file for Guava. If you use Gradle 6 or higher, you will see better handling of two kinds of dependency conflicts:

Selecting the appropriate flavor

When Gradle automatically selects the newest version of Guava in your dependency graph, it will now also select the appropriate flavor (-android or -jre) based on whether you project targets Android or not. For example, if you depend on 32.1.0-android and 30.0-jre, Gradle will select 32.1.0-jre. This is the version most likely to be compatible with all your dependencies.

If you need to override Gradle's choice, you can TODO: something like the following? I thought I'd convinced myself that I understood which part(s) of it were necessary, but now I'm confused again, sorry:

dependencies {
constraints {
"api"("com.google.guava:guava") {
attributes {
// if the Gradle version is 7+, you can use
// TargetJvmEnvironment.TARGET_JVM_ENVIRONMENT_ATTRIBUTE
attribute(Attribute.of("org.gradle.jvm.environment", String::class.java), "android")
}
}
}
}
configurations.all {
resolutionStrategy.capabilitiesResolution {
withCapability("com.google.guava:guava") {
candidates
.find {
val variantName = it.javaClass.getDeclaredMethod("getVariantName")
(variantName.invoke(it) as String).contains("android")
}
?.apply { select(this) }
}
}
}

Reporting dependencies that overlap with Guava

If your dependency graph contains the very old google-collections or the hacky listenablefuture, Gradle will now report that those libraries contain duplicates of Guava classes. When this happens, you'll need to tell Gradle to select Guava:

configurations.all {
    resolutionStrategy.capabilitiesResolution.withCapability("com.google.collections:google-collections") {
        select("com.google.guava:guava:0")
    }
    // and/or
    resolutionStrategy.capabilitiesResolution.withCapability("com.google.guava:listenablefuture") {
        select("com.google.guava:guava:0")
    }
}

(I could also mention that we'll now omit j2objc-annotations from the runtime classpath, but I think we may be better off focusing on the bigger changes, since there's plenty to discuss. Let me know if you disagree.)

@cpovirk cpovirk added package=general type=documentation Documentation that is other than for an API P2 labels Jun 28, 2023
@cpovirk cpovirk self-assigned this Jun 28, 2023
@jjohannes
Copy link
Contributor

jjohannes commented Jun 29, 2023

Here is a better version of Listing 1 (the code you took from the test contains reflection to not have old Gradle versions break). I think for now you can assume that most folks use Gradle 7 or 8 where all of this syntax is supported:

dependencies.constraints {
  implementation("com.google.guava:guava") {
    attributes {
      attribute(TargetJvmEnvironment.TARGET_JVM_ENVIRONMENT_ATTRIBUTE, 
              objects.named(TargetJvmEnvironment, TargetJvmEnvironment.ANDROID))
    }
  }
}

// If the above leads to a conflict error, because there are additional transitive dependencies to Guava:
configurations.all {
  resolutionStrategy.capabilitiesResolution.withCapability("com.google.guava:guava") {
    select(candidates.find { it.variantName.contains("android") })
  }
}

2nd Listing and text all sounds good to me!

Regarding the runtime classpath: Maybe add just one general sentence on the topic?

Dependencies of Guava that are not needed at runtime are omitted from the runtime classpath (see #2824).


About the above topic: would it be helpful if I create a draft PR that also removes the other annotation libraries from the runtime classpath to further discuss that topic? In my opinion, it would be the better "default". Or is there already an issue on the topic that is still open (as #2824 is closed)?


There is a little thing I found while testing this gain. I don't like how the variants are "named" now. I think this is the result of the back and forth with some things in the original PR over the years. Usually the "names" are not that important (the attributes are the central thing) but there are places where they shine through if you need to fine tune things in Gradle.

Right now, in the metadata, the JRE variants are named like this: "name": "standard-jvmApiElements",
I would prefer to use "jreApiElements" here instead to be in line with what it is called in Guava elsewhere (also the - in the name that otherwise uses camelCase is bugging me)

I created a small follow up PR to do that change if you agree: #6605

@cpovirk
Copy link
Member Author

cpovirk commented Jun 29, 2023

Thanks for the simplified text.

I can include the text about runtime dependencies.

I'll get the PR with the rename merged.

I'll try to think more about the other annotation dependencies. I've spent more time on Guava lately than I'd expected, and while I'm very happy with the results, I also want to postpone further work when I can get away with it, especially when there's a chance that any of the recent changes could end up requiring yet more work :) It's up to you whether you want to put together a PR now.

@jjohannes
Copy link
Contributor

Thanks @cpovirk for your investment in this. I am fine with all of that and very happy that this got integrated eventually.

I created #6606 about the runtime classpath thing. It's also for myself to remember where this topic stands right now in case this should pop up in the future again. Don't feel obliged to spend any time on that now. Just leaving it there for the community to share feedback is totally fine for me.

Also: If after the next release, any issues pop up because of this, feel free to tag me in GitHub comments. I am happy to answer questions or help with problems.

copybara-service bot pushed a commit that referenced this issue Jun 29, 2023
Small follow up to #3683

See: #6604 (comment)

Fixes #6605

RELNOTES=n/a
PiperOrigin-RevId: 544337005
copybara-service bot pushed a commit that referenced this issue Jun 29, 2023
Small follow up to #3683

See: #6604 (comment)

Fixes #6605

RELNOTES=n/a
PiperOrigin-RevId: 544384609
@cpovirk cpovirk closed this as completed Jun 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P2 package=general type=documentation Documentation that is other than for an API
Projects
None yet
Development

No branches or pull requests

2 participants