Bump Gradle to 8.5, fix cache. #4139

wwadge · 2023-11-10T09:28:24Z

Updated #4069 to not touch junit.

wwadge · 2023-11-10T11:38:10Z

Note: shift from port 5000 -> 5001 is because macOS is running AirPlay Receiver on the same port (settings -> Airdrop & Handoff -> AirPlay Receiver)

loosebazooka · 2023-11-10T13:02:22Z

Thanks for splitting it up!

loosebazooka

I understand that maybe you switching 5000 -> 5001 was needed for some local mac development, but it really does pollute this PR with a HUGE number of changed. Once again making it hard to review. I think isolating the logical changes in here from that massive change would make it easier to read and review (and not accidentally miss something).

jib-gradle-plugin/src/main/java/com/google/cloud/tools/jib/gradle/BuildDockerTask.java

wwadge · 2023-11-14T05:38:14Z

I understand that maybe you switching 5000 -> 5001 was needed for some local mac development, but it really does pollute this PR with a HUGE number of changed. Once again making it hard to review. I think isolating the logical changes in here from that massive change would make it easier to read and review (and not accidentally miss something).

Agree: removed the 5000 -> 5001 changes.

build.gradle

jib-cli/src/main/java/com/google/cloud/tools/jib/cli/war/WarFiles.java

jib-core/src/test/java/com/google/cloud/tools/jib/builder/steps/LocalBaseImageStepsTest.java

jib-gradle-plugin/src/main/java/com/google/cloud/tools/jib/gradle/BuildDockerTask.java

loosebazooka

okay cool, I think I got through most of this, just a few questions

jib-gradle-plugin/src/main/java/com/google/cloud/tools/jib/gradle/ExtraDirectoryParameters.java

jib-gradle-plugin/src/main/java/com/google/cloud/tools/jib/gradle/skaffold/FilesTaskV2.java

...plugin/src/main/java/com/google/cloud/tools/jib/gradle/skaffold/SkaffoldWatchParameters.java

...in/src/test/java/com/google/cloud/tools/jib/gradle/GradleProjectPropertiesExtensionTest.java

wwadge · 2023-11-17T14:50:56Z

Note: there's one more deprecated thing in place:

The StartParameter.settingsFile property has been deprecated. This is scheduled to be removed in Gradle 9.0. Setting custom build file to select the default project has been deprecated. Please use 'projectDir' to specify the directory of the default project instead.

This is coming from the researchgate plugin:

        at org.gradle.StartParameter.logBuildOrSettingsFileDeprecation(StartParameter.java:507)
        at org.gradle.StartParameter.getSettingsFile(StartParameter.java:497)
        at java.base/java.lang.reflect.Method.invoke(Method.java:568)
        at groovy.lang.MetaMethod.doMethodInvoke(MetaMethod.java:323)
        at net.researchgate.release.ReleasePlugin$_apply_closure5.doCall$original(ReleasePlugin.groovy:107)
        at net.researchgate.release.ReleasePlugin$_apply_closure5.doCall(ReleasePlugin.groovy)

There's no update to the version we're using (v3.0.2) at time of writing

loosebazooka · 2023-11-17T14:58:23Z

Yeah it's okay about the research gate plugin. We'll deal with that later

build.gradle

jib-cli/src/integration-test/resources/jarTest/spring-boot/build-layered.gradle

jib-gradle-plugin/src/main/java/com/google/cloud/tools/jib/gradle/BuildDockerTask.java

jib-gradle-plugin/src/main/java/com/google/cloud/tools/jib/gradle/ExtraDirectoryParameters.java

mpeddada1

I think we're getting close! Added just a few questions. Thanks again so much for your patience @wwadge! Will take a look at the windows issue.

jib-maven-plugin/src/test/resources/maven/projects/war_servlet25/pom-tomcat.xml

jib-gradle-plugin/src/main/java/com/google/cloud/tools/jib/gradle/GradleProjectProperties.java

jib-gradle-plugin/src/main/java/com/google/cloud/tools/jib/gradle/ExtraDirectoryParameters.java

jib-gradle-plugin/src/main/java/com/google/cloud/tools/jib/gradle/GradleProjectProperties.java

mpeddada1 · 2023-12-13T21:01:54Z

Looks like we're getting a slightly different error message now:

com.google.cloud.tools.jib.gradle.GradleProjectPropertiesTest > testGetMajorJavaVersion_jvm11 SKIPPED

com.google.cloud.tools.jib.gradle.JibExtensionTest > testProperties FAILED
    expected: \foo
    but was : C:\tmpfs\src\github\jib\jib-gradle-plugin\build\tmp\test\work\gradle1551860326683086368projectDir\foo
        at com.google.cloud.tools.jib.gradle.JibExtensionTest.testProperties(JibExtensionTest.java:525)

Hm it looks like the returned file path is resolving to the absolute path instead of the expected \foo.

wwadge · 2023-12-14T14:23:52Z

Looks like we're getting a slightly different error message now:

com.google.cloud.tools.jib.gradle.GradleProjectPropertiesTest > testGetMajorJavaVersion_jvm11 SKIPPED

com.google.cloud.tools.jib.gradle.JibExtensionTest > testProperties FAILED
    expected: \foo
    but was : C:\tmpfs\src\github\jib\jib-gradle-plugin\build\tmp\test\work\gradle1551860326683086368projectDir\foo
        at com.google.cloud.tools.jib.gradle.JibExtensionTest.testProperties(JibExtensionTest.java:525)

Hm it looks like the returned file path is resolving to the absolute path instead of the expected \foo.

I changed the way from(..) was implemented, let's try again with the new implementation that I just pushed

loosebazooka · 2023-12-14T15:34:16Z

com.google.cloud.tools.jib.gradle.JibExtensionTest > testProperties FAILED
    expected: \foo
    but was : C:\tmpfs\src\github\jib\jib-gradle-plugin\build\tmp\test\work\gradle5672696465279302889projectDir\foo
        at com.google.cloud.tools.jib.gradle.JibExtensionTest.testProperties(JibExtensionTest.java:525)

mpeddada1 · 2023-12-18T16:38:34Z

jib-gradle-plugin/src/test/java/com/google/cloud/tools/jib/gradle/JibExtensionTest.java

+    assertThat(
+            testJibExtension.getSkaffold().getWatch().getExcludes().getFiles().stream()
+                .map(File::toPath)
+                .collect(Collectors.toSet()))
        .containsExactly(root.resolve("watch4").toAbsolutePath());
  }



It looks like there is some inconsistency in how windows and other systems resolve paths; the former resolves to the absolute path whereas the latter resolves to the relative path. For consistent test behavior, can we always test on the absolute path with the help of toAbsolutePath()?

jib/jib-gradle-plugin/src/test/java/com/google/cloud/tools/jib/gradle/JibExtensionTest.java

Lines 511 to 514 in 440bd48

assertThat(testJibExtension.getExtraDirectories().getPaths().get(0).getFrom())

.isEqualTo(Paths.get("/foo"));

assertThat(testJibExtension.getExtraDirectories().getPaths().get(1).getFrom())

.isEqualTo(Paths.get("/bar/baz"));

Same as what is being done in testSkaffold(). cc/ @loosebazooka

These docs have this to say:
Never use new File(relative path) unless passed to file() or files() or from() or other methods being defined in terms of file() or files(). Otherwise this creates a path relative to the current working directory (CWD). Gradle can make no guarantees about the location of the CWD, which means builds that rely on it may break at any time.

Adding toAbsolutePath() to that test....

Stacktrace:

com.google.cloud.tools.jib.gradle.JibExtensionTest > testProperties FAILED expected: C:\foo but was : C:\tmpfs\src\github\jib\jib-gradle-plugin\build\tmp\test\work\gradle10718998764490656projectDir\foo at com.google.cloud.tools.jib.gradle.JibExtensionTest.testProperties(JibExtensionTest.java:525)

Added a few observations in #4159 (comment) after trying out a couple of things (including adjusting the tests to verify on the absolute path). The getFrom() method (pasted below) always resolves to the absolute path in both windows and ubuntu) when the file paths are provided through gradle properties but shows discrepancies when the path is provided through a System Property.

@Override @InputFile public Path getFrom() { return from.getSingleFile().toPath(); }

Since this is not a blocker for Gradle 8.x updates, could we revert the logic of extra directories to how it was originally? This annotation is an interesting enhancement to explore but probably belongs in a separate issue due to the change in behavior that is yet to be understood further.

wwadge · 2024-01-04T09:09:06Z

Hopefully this latest commit is close enough to how it was and works - I cannot revert completely because we used "project.file" previously. I will try to obtain a windows box to test if this one fails too for I'm still working a bit blind.

sonarcloud · 2024-01-05T06:31:46Z

Quality Gate passed

Kudos, no new issues were introduced!

0 New issues
0 Security Hotspots
86.3% Coverage on New Code
1.7% Duplication on New Code

See analysis details on SonarCloud

wwadge · 2024-01-05T06:33:31Z

One more kokoru run please - I reverted that .toAbsolutePath() bit we had added to our tests. Now that I reverted how I'm handling paths, I needed to revert that .toAbsolutePath() bit too.

Seems to work on a local windows box.

loosebazooka · 2024-01-08T18:11:20Z

@mpeddada1 I think this is ready for another review

blakeli0 · 2024-01-16T19:43:41Z

jib-gradle-plugin/src/main/java/com/google/cloud/tools/jib/gradle/JibPlugin.java

 import org.gradle.api.tasks.TaskContainer;
 import org.gradle.api.tasks.TaskProvider;
 import org.gradle.api.tasks.bundling.Jar;
 import org.gradle.util.GradleVersion;

 public class JibPlugin implements Plugin<Project> {

-  @VisibleForTesting static final GradleVersion GRADLE_MIN_VERSION = GradleVersion.version("5.1");
+  @VisibleForTesting static final GradleVersion GRADLE_MIN_VERSION = GradleVersion.version("8.0");


Unfortunately this is a breaking change for Jib, we still have customers on Gradle versions < 8, and we don't want to stop the support for them. Since javaPluginExtension.getSourceSets() does not exist until Gradle 8, I don't think we have a way to add this feature without stop supporting Gradle 6 and 7 customers.

Is this the only breaking change? We can make conditional calls to various things.

It's from 7.1 according to: https://docs.gradle.org/current/javadoc/org/gradle/api/plugins/JavaPluginExtension.html#getSourceSets--

We're using it in very few places so I don't think it's very difficult to support older versions. I can attempt a patch to support as from v6 if you want though I understand if you wish to put this PR on hold till some arbitrary future date.

blakeli0 · 2024-01-16T19:52:56Z

@wwadge Thanks for submitting this PR and all the effort! @loosebazooka Thanks for putting the effort reviewing the PR! Unfortunately, as I mentioned in the comment, after careful consideration, we don't want to introduce a breaking change for customers still use Gradle < 8 at this moment. So we can not accept this PR as it is right now, we may re-visit it once we feel comfortable to move the minimum supported version for jib to 8.

We are really sorry for the back and forth in the past few months, please let us know if you have other suggestions!

wwadge marked this pull request as draft November 10, 2023 09:55

wwadge force-pushed the patch-to-8.4 branch 4 times, most recently from bde6b73 to d9f4877 Compare November 10, 2023 11:35

wwadge force-pushed the patch-to-8.4 branch from d9f4877 to 56c1360 Compare November 10, 2023 11:41

wwadge marked this pull request as ready for review November 10, 2023 11:59

wwadge mentioned this pull request Nov 10, 2023

Upgrade to gradle 8.4, junit 5. Fix configuration cache issues. #4069

Closed

loosebazooka reviewed Nov 13, 2023

View reviewed changes

jib-gradle-plugin/src/main/java/com/google/cloud/tools/jib/gradle/BuildDockerTask.java Outdated Show resolved Hide resolved

wwadge force-pushed the patch-to-8.4 branch 2 times, most recently from 7df6a84 to e12e973 Compare November 14, 2023 05:28

wwadge commented Nov 14, 2023

View reviewed changes

build.gradle Show resolved Hide resolved

wwadge commented Nov 14, 2023

View reviewed changes

jib-cli/src/main/java/com/google/cloud/tools/jib/cli/war/WarFiles.java Show resolved Hide resolved

wwadge commented Nov 14, 2023

View reviewed changes

jib-core/src/test/java/com/google/cloud/tools/jib/builder/steps/LocalBaseImageStepsTest.java Outdated Show resolved Hide resolved

loosebazooka reviewed Nov 15, 2023

View reviewed changes

jib-gradle-plugin/src/main/java/com/google/cloud/tools/jib/gradle/BuildDockerTask.java Show resolved Hide resolved

wwadge force-pushed the patch-to-8.4 branch 2 times, most recently from 49f88fb to 19daba5 Compare November 16, 2023 08:38

loosebazooka reviewed Nov 16, 2023

View reviewed changes

wwadge force-pushed the patch-to-8.4 branch from 19daba5 to 9d80f95 Compare November 17, 2023 14:25

loosebazooka added the kokoro:run label Nov 17, 2023

kokoro-team removed the kokoro:run label Nov 17, 2023

mpeddada1 reviewed Nov 17, 2023

View reviewed changes

wwadge force-pushed the patch-to-8.4 branch from 9d80f95 to b12d4d8 Compare November 17, 2023 15:46

rquinio mentioned this pull request Nov 19, 2023

Upgrade workflow to run unit tests in Java 17 - the latest LTS #3139

Open

alicejli mentioned this pull request Nov 20, 2023

Upgrade to Gradle v8.4, junit 5. Fix configuration cache issues #4070

Open

mpeddada1 added the kokoro:run label Dec 13, 2023

kokoro-team removed kokoro:run labels Dec 13, 2023

mpeddada1 reviewed Dec 13, 2023

View reviewed changes

wwadge force-pushed the patch-to-8.4 branch 2 times, most recently from 7732789 to 8a62443 Compare December 14, 2023 14:14

wwadge force-pushed the patch-to-8.4 branch 2 times, most recently from 6e2e981 to dd08d58 Compare December 14, 2023 14:45

loosebazooka added the kokoro:run label Dec 14, 2023

kokoro-team removed the kokoro:run label Dec 14, 2023

mpeddada1 reviewed Dec 18, 2023

View reviewed changes

wwadge force-pushed the patch-to-8.4 branch from dd08d58 to e174834 Compare December 19, 2023 09:10

mpeddada1 added the kokoro:force-run label Dec 19, 2023

kokoro-team removed the kokoro:force-run label Dec 19, 2023

wwadge force-pushed the patch-to-8.4 branch from e174834 to 3c7187a Compare January 4, 2024 08:59

loosebazooka added the kokoro:run label Jan 4, 2024

kokoro-team removed the kokoro:run label Jan 4, 2024

Bump Gradle to 8.5, fix cache.

c71af28

wwadge force-pushed the patch-to-8.4 branch from 3c7187a to c71af28 Compare January 5, 2024 06:25

loosebazooka added the kokoro:run label Jan 5, 2024

kokoro-team removed the kokoro:run label Jan 5, 2024

blakeli0 reviewed Jan 16, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bump Gradle to 8.5, fix cache. #4139

Bump Gradle to 8.5, fix cache. #4139

wwadge commented Nov 10, 2023

wwadge commented Nov 10, 2023

loosebazooka commented Nov 10, 2023

loosebazooka left a comment

wwadge commented Nov 14, 2023

loosebazooka left a comment

wwadge commented Nov 17, 2023

loosebazooka commented Nov 17, 2023

mpeddada1 left a comment

mpeddada1 commented Dec 13, 2023 •

edited

Loading

wwadge commented Dec 14, 2023

loosebazooka commented Dec 14, 2023

mpeddada1 Dec 18, 2023

wwadge Dec 19, 2023

mpeddada1 Dec 21, 2023 •

edited

Loading

wwadge commented Jan 4, 2024

sonarcloud bot commented Jan 5, 2024

wwadge commented Jan 5, 2024

loosebazooka commented Jan 8, 2024

blakeli0 Jan 16, 2024

loosebazooka Jan 16, 2024

wwadge Jan 16, 2024

blakeli0 commented Jan 16, 2024

	assertThat(testJibExtension.getExtraDirectories().getPaths().get(0).getFrom())
	.isEqualTo(Paths.get("/foo"));
	assertThat(testJibExtension.getExtraDirectories().getPaths().get(1).getFrom())
	.isEqualTo(Paths.get("/bar/baz"));

Bump Gradle to 8.5, fix cache. #4139

Are you sure you want to change the base?

Bump Gradle to 8.5, fix cache. #4139

Conversation

wwadge commented Nov 10, 2023

wwadge commented Nov 10, 2023

loosebazooka commented Nov 10, 2023

loosebazooka left a comment

Choose a reason for hiding this comment

wwadge commented Nov 14, 2023

loosebazooka left a comment

Choose a reason for hiding this comment

wwadge commented Nov 17, 2023

loosebazooka commented Nov 17, 2023

mpeddada1 left a comment

Choose a reason for hiding this comment

mpeddada1 commented Dec 13, 2023 • edited Loading

wwadge commented Dec 14, 2023

loosebazooka commented Dec 14, 2023

mpeddada1 Dec 18, 2023

Choose a reason for hiding this comment

wwadge Dec 19, 2023

Choose a reason for hiding this comment

mpeddada1 Dec 21, 2023 • edited Loading

Choose a reason for hiding this comment

wwadge commented Jan 4, 2024

sonarcloud bot commented Jan 5, 2024

Quality Gate passed

wwadge commented Jan 5, 2024

loosebazooka commented Jan 8, 2024

blakeli0 Jan 16, 2024

Choose a reason for hiding this comment

loosebazooka Jan 16, 2024

Choose a reason for hiding this comment

wwadge Jan 16, 2024

Choose a reason for hiding this comment

blakeli0 commented Jan 16, 2024

mpeddada1 commented Dec 13, 2023 •

edited

Loading

mpeddada1 Dec 21, 2023 •

edited

Loading