Bring back Maven dependencies into Gradle #21202

abstratt · 2022-07-07T22:27:23Z

Context

Back in 7.0, we removed most Maven artifacts from the Gradle distro so they could be dynamically fetched in gradle init --type pom, in an attempt to reduce the Gradle payload. However, that breaks users that are behind corporate firewalls.

This commit brings those dependencies back, while moving to a more
recent versions of Maven artifacts.

Issue: #20596

Gradle Core Team Checklist

Verify design and implementation
Verify test coverage and CI build status
Verify documentation
Recognize contributor in release notes

abstratt · 2022-07-07T22:28:49Z

This is mostly based on @big-guy 's #21156

gradle/verification-metadata.xml

big-guy · 2022-07-08T19:43:55Z

Your PR is failing the DCO checks because your commits aren't signed. This looks like it was working before, e.g., you can see the "Verified" on this other commit: d56201e

You'll have to rebase your branch and sign the commits with something like git rebase HEAD~3 -S. The documentation the check provides is misleading because it's talking about Signed-off-by. This is for external contributors.

I think it would be better if the update to Maven 3.8.6 was at least a separate commit from the changes to remove the dependency on Maven Central. You could also combine the changes to verification-metadata to go along with the upgrade.

abstratt · 2022-07-11T14:10:51Z

Your PR is failing the DCO checks because your commits aren't signed. This looks like it was working before, e.g., you can see the "Verified" on this other commit: d56201e

Thanks - it seems to have to do with doing the rebase using GH's web UI, though apparently that is supposed to work.

I think it would be better if the update to Maven 3.8.6 was at least a separate commit from the changes to remove the dependency on Maven Central. You could also combine the changes to verification-metadata to go along with the upgrade.

My only reservation with doing it that way now is that I would have to re-test both scenarios separately (or at least what would be the first commit), as I have only verified those two changes together.

Are you considering the idea we may want to merge one change but not the other?

abstratt · 2022-07-11T18:20:40Z

...tributions-integ-tests/src/integTest/groovy/org/gradle/BinDistributionIntegrationSpec.groovy

@@ -31,7 31,7 @@ class BinDistributionIntegrationSpec extends DistributionIntegrationSpec {

 @Override


~ ls -lah *.zip -rw-r--r-- 1 rafael staff 115M Jul 11 15:17 master.zip -rw-r--r-- 1 rafael staff 118M Jul 11 15:18 pr-20596.zip

abstratt · 2022-07-11T18:23:58Z

...distributions-integ-tests/src/integTest/groovy/org/gradle/DistributionIntegrationSpec.groovy

@@ -36,7 36,7 @@ import static org.hamcrest.MatcherAssert.assertThat

 abstract class DistributionIntegrationSpec extends AbstractIntegrationSpec {

- protected static final THIRD_PARTY_LIB_COUNT = 145
+ protected static final THIRD_PARTY_LIB_COUNT = 167



➜ gradle git:(rchaves/maven-dependencies-20596) ✗ find ~/gradle-installs/pr-20596 -name "*.jar" | wc -l 255 ➜ gradle git:(rchaves/maven-dependencies-20596) ✗ find ~/gradle-installs/master -name "*.jar" | wc -l 233

do you know what these extra libraries are? I thought this only removed a few (not >20) libraries when we removed them in 7.0?

diff /tmp/master-libs.txt /tmp/pr-21202-libs.txt 130a131 > lib/plugins/aopalliance-1.0.jar 139a141 > lib/plugins/cdi-api-1.0.jar 140a143 > lib/plugins/commons-lang3-3.12.0.jar 195a199 > lib/plugins/guice-4.2.3-no_aop.jar 213a218 > lib/plugins/jsr250-api-1.0.jar 217a223 > lib/plugins/maven-artifact-3.6.3.jar 218a225,226 > lib/plugins/maven-compat-3.6.3.jar > lib/plugins/maven-core-3.6.3.jar 219a228,229 > lib/plugins/maven-model-builder-3.6.3.jar > lib/plugins/maven-plugin-api-3.6.3.jar 220a231,237 > lib/plugins/maven-resolver-api-1.4.1.jar > lib/plugins/maven-resolver-connector-basic-1.4.1.jar > lib/plugins/maven-resolver-impl-1.4.1.jar > lib/plugins/maven-resolver-provider-3.6.3.jar > lib/plugins/maven-resolver-spi-1.4.1.jar > lib/plugins/maven-resolver-transport-http-1.4.1.jar > lib/plugins/maven-resolver-util-1.4.1.jar 222a240 > lib/plugins/maven-shared-utils-3.2.1.jar 224a243,244 > lib/plugins/org.eclipse.sisu.inject-0.3.4.jar > lib/plugins/org.eclipse.sisu.plexus-0.3.4.jar 225a246,247 > lib/plugins/plexus-classworlds-2.6.0.jar > lib/plugins/plexus-component-annotations-2.1.0.jar 230a253 > lib/plugins/wagon-provider-api-3.3.4.jar

@big-guy It is basically most-of-Maven vs maven-settings-builder.

ljacomet · 2022-07-14T19:47:24Z

Given that this PR will have to do changes in dependency verification, make sure to account for the process changes in #21234

Back in 7.0, we removed most Maven artifacts from the Gradle distro so they could be dynamically fetched in gradle init --type pom, in an attempt to reduce the Gradle payload. However, that breaks users that are behind corporate firewalls. This commit brings those dependencies back. Issue: #20596

abstratt · 2022-07-14T23:01:00Z

@bot-gradle test this

bot-gradle · 2022-07-14T23:01:09Z

OK, I've already triggered the following builds for you:

PullRequestFeedback build

abstratt · 2022-07-14T23:03:17Z

This is now pretty much @big-guy's changes (#21156), plus updates to tests that checked artifact counts/distro sizes. With the reversal of the Maven dependency upgrades, the changes to verification metadata are no longer needed. We can look into that once this PR is merged.

@ljacomet Could you please take a second look?

abstratt · 2022-07-15T13:15:42Z

Given that this PR will have to do changes in dependency verification, make sure to account for the process changes in #21234

Thanks, that would be needed when we actually upgrade Maven dependencies, but this PR is no longer trying to do that.

eskatos

We should do the upgrade of the added dependencies.

I left a blocking question.

eskatos · 2022-07-19T07:14:58Z

...init/src/main/java/org/gradle/buildinit/plugins/internal/maven/PomProjectInitDescriptor.java

 @Override
 public void generate(InitSettings initSettings) {
 IncubationLogger.incubatingFeatureUsed("Maven to Gradle conversion");
 try {
 Settings settings = settingsProvider.buildSettings();
- executor.classLoaderIsolation(config -> config.getClasspath().from(mavenClasspath))
+ executor.noIsolation()


I wonder if we'd want to keep the maven dependencies isolated. It would mean including the jars in the distro but not including them in the Gradle core and plugins classpath and resolving them from the distro using the module registry / classpath provider mechanism instead.

WDYT @big-guy, @ljacomet ?

@eskatos What would that look like? Just:

executor.classLoaderIsolation() .submit(...)

?

(your recommendation does make sense to me, btw, at least everything else being equal)

At this place it would look like (pseudo-code):

FileCollection mavenClasspath = classpathRegistry.getClassPath(MAVEN) executor.classLoaderIsolation(config -> config.getClasspath().from(mavenClasspath))

and require support for building that classpath from jars in the distro that are not part of the core and plugins classpath. I guess a bit like the ANT classpath.

Issue: #20596

abstratt · 2022-07-19T15:03:15Z

We should do the upgrade of the added dependencies.

Agreed! Once we merge this, I will open a ticket about that.

abstratt · 2022-07-19T15:03:39Z

@bot-gradle test this

bot-gradle · 2022-07-19T15:03:47Z

OK, I've already triggered the following builds for you:

PullRequestFeedback build

abstratt force-pushed the rchaves/maven-dependencies-20596 branch from ad74d61 to fd287d4 Compare July 8, 2022 13:17

abstratt commented Jul 8, 2022

View reviewed changes

gradle/verification-metadata.xml Outdated Show resolved Hide resolved

abstratt force-pushed the rchaves/maven-dependencies-20596 branch from e7e781a to b5bf7d8 Compare July 8, 2022 18:17

abstratt marked this pull request as ready for review July 8, 2022 18:17

abstratt self-assigned this Jul 8, 2022

big-guy added this to the 7.6 RC1 milestone Jul 8, 2022

big-guy self-requested a review July 8, 2022 19:30

abstratt force-pushed the rchaves/maven-dependencies-20596 branch from b5bf7d8 to 1af955a Compare July 11, 2022 14:02

gradle deleted a comment from abstratt Jul 11, 2022

abstratt commented Jul 11, 2022

View reviewed changes

abstratt force-pushed the rchaves/maven-dependencies-20596 branch from a7495ec to 81b07ba Compare July 14, 2022 22:31

abstratt force-pushed the rchaves/maven-dependencies-20596 branch from 81b07ba to 8f28d45 Compare July 14, 2022 23:00

gradle deleted a comment from abstratt Jul 14, 2022

eskatos reviewed Jul 19, 2022

View reviewed changes

Use classloader isolation in pom migration

3da3bfb

Issue: #20596

gradle deleted a comment from abstratt Jul 19, 2022

big-guy changed the base branch from master to release August 9, 2022 21:42

big-guy added the @core Issue owned by GBT Core label Sep 7, 2022

abstratt modified the milestones: 7.6 RC1, 8.0 RC1 Sep 7, 2022

big-guy assigned big-guy and unassigned abstratt Nov 8, 2022

big-guy modified the milestones: 8.0 RC1, 8.1 RC1 Dec 15, 2022

jvandort modified the milestones: 8.1 RC1, 8.2 RC1 Mar 1, 2023

big-guy removed the @core Issue owned by GBT Core label Apr 6, 2023

big-guy modified the milestones: 8.2 RC1, 8.3 RC1 Apr 6, 2023

big-guy modified the milestones: 8.3 RC1, 9.0 RC1 Jul 6, 2023

ValentinLeTallec mentioned this pull request Feb 19, 2024

Gradle init detached configuration resolver maven repo #20596

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bring back Maven dependencies into Gradle #21202

Bring back Maven dependencies into Gradle #21202

abstratt commented Jul 7, 2022

abstratt commented Jul 7, 2022

big-guy commented Jul 8, 2022

abstratt commented Jul 11, 2022

abstratt Jul 11, 2022

abstratt Jul 11, 2022 •

edited

Loading

big-guy Jul 15, 2022

abstratt Jul 15, 2022

abstratt Jul 15, 2022

ljacomet commented Jul 14, 2022

abstratt commented Jul 14, 2022

bot-gradle commented Jul 14, 2022

abstratt commented Jul 14, 2022 •

edited

Loading

abstratt commented Jul 15, 2022

eskatos left a comment

eskatos Jul 19, 2022

abstratt Jul 19, 2022 •

edited

Loading

eskatos Jul 19, 2022

abstratt commented Jul 19, 2022

abstratt commented Jul 19, 2022

bot-gradle commented Jul 19, 2022

		@@ -31,7 31,7 @@ class BinDistributionIntegrationSpec extends DistributionIntegrationSpec {

		@Override

Bring back Maven dependencies into Gradle #21202

Are you sure you want to change the base?

Bring back Maven dependencies into Gradle #21202

Conversation

abstratt commented Jul 7, 2022

Context

Gradle Core Team Checklist

abstratt commented Jul 7, 2022

big-guy commented Jul 8, 2022

abstratt commented Jul 11, 2022

abstratt Jul 11, 2022

Choose a reason for hiding this comment

abstratt Jul 11, 2022 • edited Loading

Choose a reason for hiding this comment

big-guy Jul 15, 2022

Choose a reason for hiding this comment

abstratt Jul 15, 2022

Choose a reason for hiding this comment

abstratt Jul 15, 2022

Choose a reason for hiding this comment

ljacomet commented Jul 14, 2022

abstratt commented Jul 14, 2022

bot-gradle commented Jul 14, 2022

abstratt commented Jul 14, 2022 • edited Loading

abstratt commented Jul 15, 2022

eskatos left a comment

Choose a reason for hiding this comment

eskatos Jul 19, 2022

Choose a reason for hiding this comment

abstratt Jul 19, 2022 • edited Loading

Choose a reason for hiding this comment

eskatos Jul 19, 2022

Choose a reason for hiding this comment

abstratt commented Jul 19, 2022

abstratt commented Jul 19, 2022

bot-gradle commented Jul 19, 2022

abstratt Jul 11, 2022 •

edited

Loading

abstratt commented Jul 14, 2022 •

edited

Loading

abstratt Jul 19, 2022 •

edited

Loading