Update maven wrapper `0.5.6` -> `3.1.0` #11387

ssheikin · 2022-03-09T08:23:44Z

mvnw copied from
https://mvnrepository.com/artifact/org.apache.maven.wrapper/maven-wrapper-distribution/3.1.0
maven-wrapper.jar copied from
https://mvnrepository.com/artifact/org.apache.maven.wrapper/maven-wrapper/3.1.0

kokosing · 2022-03-09T10:44:42Z

Please annotate flaky test failures.

kokosing · 2022-03-09T10:44:57Z

What are the changes between these version?

Praveen2112 · 2022-03-09T12:28:37Z

mvnw

@@ -199,6 +203,85 @@ if [ -z "$BASE_DIR" ]; then
  exit 1;
 fi

+##########################################################################################
+# Extension to allow automatically downloading the maven-wrapper.jar from Maven-central


Since we bundle the jar as a part of this commit - do we still need to download them ? Or should we the jar that we have bundled ?

No, we don't need to download a new jar.
From the first glance this is a dead code.

mvnw file is a dumb copy from
https://mvnrepository.com/artifact/org.apache.maven.wrapper/maven-wrapper-distribution/3.1.0
I don't think we want/need to tweak it.

I've updated commit message.

Can we point it to the source code location in github ?

BTW how different is new maven-wrapper.jar from the one added in this PR - #9970

I've doublechecked, and seems this code is not that dead. I'll experiment with wrapperUrl property.

Ah, ok, it's still takes in effect only if the jar is not present.

BTW how different is new maven-wrapper.jar from the one added in this PR - #9970
What are the changes between these version?

If I understand correctly the major thing is the donation of the maven wrapper to the apache foundation. Other changes were minor, but they could be quite subtle ones and it's cheaper to keep wrapper up to date.
apache/maven-wrapper@maven-wrapper-0.5.6...maven-wrapper-parent-3.1.0

Can we point it to the source code location in github ?

I've added a link to the gh repo to the commit message.
mvnw file itself is generated during the build.

Praveen2112 · 2022-03-09T12:29:49Z

mvnw

  -classpath "$MAVEN_PROJECTBASEDIR/.mvn/wrapper/maven-wrapper.jar" \
-  "-Dmaven.home=${M2_HOME}" "-Dmaven.multiModuleProjectDirectory=${MAVEN_PROJECTBASEDIR}" \
+  "-Dmaven.home=${M2_HOME}" \
+  "-Dmaven.multiModuleProjectDirectory=${MAVEN_PROJECTBASEDIR}" \


nit: Can we move this to new commit ?

No. #11387 (comment)

ssheikin · 2022-03-09T16:17:41Z

Please annotate flaky test failures.

https://github.com/trinodb/trino/runs/5477229326?check_suite_focus=true

2022-03-09T09:46:20.1563106Z [ERROR] Tests run: 3124, Failures: 1, Errors: 0, Skipped: 92, Time elapsed: 2,348.463 s <<< FAILURE! - in TestSuite
2022-03-09T09:46:20.1564177Z [ERROR] io.trino.plugin.hive.TestHiveDynamicPartitionPruningTest.testJoinWithEmptyBuildSide  Time elapsed: 3.617 s  <<< FAILURE!
2022-03-09T09:46:20.1565293Z java.lang.AssertionError: expected [true] but found [false]
2022-03-09T09:46:20.1565839Z 	at org.testng.Assert.fail(Assert.java:94)
2022-03-09T09:46:20.1566457Z 	at org.testng.Assert.failNotEquals(Assert.java:513)
2022-03-09T09:46:20.1567249Z 	at org.testng.Assert.assertTrue(Assert.java:42)
2022-03-09T09:46:20.1567663Z 	at org.testng.Assert.assertTrue(Assert.java:52)
2022-03-09T09:46:20.1568270Z 	at io.trino.testing.BaseDynamicPartitionPruningTest.testJoinWithEmptyBuildSide(BaseDynamicPartitionPruningTest.java:119)
2022-03-09T09:46:20.1568947Z 	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
2022-03-09T09:46:20.1569504Z 	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
2022-03-09T09:46:20.1570139Z 	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
2022-03-09T09:46:20.1570672Z 	at java.base/java.lang.reflect.Method.invoke(Method.java:566)
2022-03-09T09:46:20.1571169Z 	at org.testng.internal.MethodInvocationHelper.invokeMethod(MethodInvocationHelper.java:104)
2022-03-09T09:46:20.1571709Z 	at org.testng.internal.InvokeMethodRunnable.runOne(InvokeMethodRunnable.java:54)
2022-03-09T09:46:20.1572214Z 	at org.testng.internal.InvokeMethodRunnable.run(InvokeMethodRunnable.java:44)
2022-03-09T09:46:20.1572839Z 	at java.base/java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:515)
2022-03-09T09:46:20.1573281Z 	at java.base/java.util.concurrent.FutureTask.run(FutureTask.java:264)
2022-03-09T09:46:20.1573760Z 	at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1128)
2022-03-09T09:46:20.1574475Z 	at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:628)
2022-03-09T09:46:20.1574888Z 	at java.base/java.lang.Thread.run(Thread.java:829)
2022-03-09T09:46:20.1575327Z 
2022-03-09T09:46:20.8780123Z [INFO] 
2022-03-09T09:46:20.8782385Z [INFO] Results:
2022-03-09T09:46:20.8782834Z [INFO] 
2022-03-09T09:46:20.8783168Z [ERROR] Failures: 
2022-03-09T09:46:20.8805769Z [ERROR]   TestHiveDynamicPartitionPruningTest>BaseDynamicPartitionPruningTest.testJoinWithEmptyBuildSide:119 expected [true] but found [false]
2022-03-09T09:46:20.8806376Z [INFO] 
2022-03-09T09:46:20.8832222Z [ERROR] Tests run: 3124, Failures: 1, Errors: 0, Skipped: 92

Let's wait for other builds:
https://github.com/trinodb/trino/actions?query=branch%3Assheikin%2F24%2Foss%2Fmvnw++

kokosing · 2022-03-09T19:45:19Z

Please create a github issue for the flaky test and annotate it with bug and test labels.

ssheikin · 2022-03-10T08:42:44Z

Flaky:
#11407
#11408

...lake/src/test/java/io/trino/plugin/deltalake/AbstractTestDeltaLakeCreateTableStatistics.java

kokosing · 2022-03-10T10:55:39Z

mvnw

@@ -199,6 +203,85 @@ if [ -z "$BASE_DIR" ]; then
  exit 1;
 fi

+##########################################################################################
+# Extension to allow automatically downloading the maven-wrapper.jar from Maven-central


Maybe we should remove the jar then?

I don't have strong personal opinion.
If the maven-wrapper.jar is not present the version which should be downloaded is specified by wrapperUrl in maven-wrapper.properties.

What is recommended by maven wrapper? We download maven anyway, so downloading one more jar is not a big deal.

True, but I didn't find any best-practices regarding it.

The official documentation https://maven.apache.org/wrapper/ states

Usage without Binary JAR

By default, the Maven Wrapper JAR archive is added to the using project as small binary file .mvn/wrapper/maven-wrapper.jar. It is used to bootstrap the download and invocation of Maven from the wrapper shell scripts.

If your project is not allowed to contain binary files like this, you can configure your version control system to exclude checkin/commit of the wrapper jar.

If the JAR is not found to be available by the scripts they will attempt to download the file from the URL specified in .mvn/wrapper/maven-wrapper.properties under wrapperUrl and put it in place. The download is attempted via curl, wget and, as last resort, by compiling the ./mvn/wrapper/MavenWrapperDownloader.java file and executing the resulting class.

The mvnw file itself has code

if [ -r "$BASE_DIR/.mvn/wrapper/maven-wrapper.jar" ]; then echo "Found .mvn/wrapper/maven-wrapper.jar" else ...download...

Taking to the account all the above I'd say there is no such recommendation to remove maven-wrapper.jar.

Ok. Let's keep the verbose copy. It is part of the maven wrapper tool, so we should not change it. It is kind of accidental that it is bash.

mvnw copied from https://mvnrepository.com/artifact/org.apache.maven.wrapper/maven-wrapper-distribution/3.1.0 maven-wrapper.jar copied from https://mvnrepository.com/artifact/org.apache.maven.wrapper/maven-wrapper/3.1.0 Source code: https://github.com/apache/maven-wrapper `3.1.0` is the first version released after The Maven Wrapper has officially been released from the Apache Maven Project! `0.5.6` locations: https://github.com/takari/maven-wrapper https://mvnrepository.com/artifact/io.takari/maven-wrapper/0.5.6

kokosing

% comments

kokosing · 2022-03-10T13:54:08Z

mvnw

@@ -199,6 +203,85 @@ if [ -z "$BASE_DIR" ]; then
  exit 1;
 fi

+##########################################################################################
+# Extension to allow automatically downloading the maven-wrapper.jar from Maven-central


Ok. Let's keep the verbose copy. It is part of the maven wrapper tool, so we should not change it. It is kind of accidental that it is bash.

.github/workflows/milestone.yml

kokosing · 2022-03-10T19:54:22Z

.github/workflows/milestone.yml

@@ -12,9 +12,7 @@ jobs:
      - name: Checkout code
        uses: actions/checkout@v2
      - name: Get milestone from pom.xml
-        run: |
-          .github/bin/retry ./mvnw -v


Retry here was to retry maven download in case of network issues.

I don't believe this was the case here. It downloads only maven jar, without project dependencies.
Have you got an experience that maven jar download is not stable?

Yes. That was a case in the past. Also "maven jar" consists of plenty of jars.

Yes. That was a case in the past.

Interesting. We have much more artifacts to download from repo1.maven.org/maven2 and I could bet maven zip itself is not a bottle neck.

Also "maven jar" consists of plenty of jars.

But they are compressed to one file:

distributionUrl=https://repo1.maven.org/maven2/org/apache/maven/apache-maven/3.8.4/apache-maven-3.8.4-bin.zip

https://github.com/apache/maven-wrapper/blob/master/maven-wrapper/src/main/java/org/apache/maven/wrapper/Installer.java

I'd leave these line removed If only you don't insist to revert a change.

Assuming the network to be flaky is a better assumption that the other. Retries don't hurt, so we should keep it here IMO.

kokosing · 2022-03-10T19:55:09Z

.github/workflows/milestone.yml

@@ -12,9 +12,7 @@ jobs:
      - name: Checkout code


Remove obsolete hotfix -> this is very generic title, can you please have a bit more specific title?

Do you mean commit message or the title itself of the commit message?
If first, the title is described in the body of the commit message.
if second - it's hard for me to find better laconic title.

I mean the title of the commit message. I see it is described, but anyway better title would be good. Like Do not display maven version.

BTW. Displaying maven version is also useful when diagnosing issues in CI.

Do not display maven version this is how it was done, but not what.

I've applied your comment.

ssheikin · 2022-03-14T12:14:52Z

All comments have been addressed. Please accept.

hashhar

Do I understand correctly that mvnw doesn't log output when downloading if not present? (2nd commit).

LGTM % my confusion

Remove obsolete hotfix If maven is not downloaded for maven-wrapper, `./mvnw` downloads it with `org.apache.maven.wrapper.Installer`. `org.apache.maven.wrapper.Installer` in version `0.1.2-SNAPSHOT` which we had some day before (upgraded in 3e35fc4) logs to the standard output. The newest version of maven-wrapper.jar is `3.1.0` and it logs to log https://mvnrepository.com/artifact/org.apache.maven.wrapper/maven-wrapper To overcome this issue `./mvnw -v` construction was used to skip output before any actual `./mvnw` invocation which returned valuable text output.

ssheikin · 2022-03-14T13:22:27Z

Do I understand correctly that mvnw doesn't log output when downloading if not present? (2nd commit).

LGTM % my confusion

I've updated the commit message.

0.1.2-SNAPSHOT logs to the standard output.
3.1.0 logs to log.
The problem was that shell command substitution takes standard output as a value (and it was polluted)

kokosing

Let me know, once CI is green.

cla-bot bot added the cla-signed label Mar 9, 2022

ssheikin requested review from hashhar, kokosing and Praveen2112 March 9, 2022 08:26

Praveen2112 reviewed Mar 9, 2022

View reviewed changes

ssheikin force-pushed the ssheikin/24/oss/mvnw branch 2 times, most recently from 6c7c931 to a2e1385 Compare March 9, 2022 14:14

ssheikin force-pushed the ssheikin/24/oss/mvnw branch from a2e1385 to ed09fbd Compare March 10, 2022 10:14

kokosing reviewed Mar 10, 2022

View reviewed changes

kokosing approved these changes Mar 10, 2022

View reviewed changes

Praveen2112 approved these changes Mar 10, 2022

View reviewed changes

ssheikin force-pushed the ssheikin/24/oss/mvnw branch from ed09fbd to 1399ad6 Compare March 10, 2022 14:20

github-actions bot added the tests:hive label Mar 10, 2022

kokosing reviewed Mar 10, 2022

View reviewed changes

ssheikin force-pushed the ssheikin/24/oss/mvnw branch from 8dc75e9 to 2fa6c29 Compare March 14, 2022 12:11

hashhar approved these changes Mar 14, 2022

View reviewed changes

ssheikin added 2 commits March 14, 2022 14:18

Improve maven property evaluation

a079541

ssheikin force-pushed the ssheikin/24/oss/mvnw branch from 2fa6c29 to a079541 Compare March 14, 2022 13:18

kokosing approved these changes Mar 14, 2022

View reviewed changes

hashhar merged commit 21d4224 into trinodb:master Mar 14, 2022

mosabua mentioned this pull request Mar 14, 2022

Add Trino 374 release notes #11417

Merged

github-actions bot added this to the 374 milestone Mar 14, 2022

martint mentioned this pull request Mar 16, 2022

Release fails when building RPM #11531

Closed

ssheikin deleted the ssheikin/24/oss/mvnw branch October 24, 2022 19:38

Update maven wrapper 0.5.6 -> 3.1.0 #11387

Update maven wrapper 0.5.6 -> 3.1.0 #11387

Conversation

ssheikin commented Mar 9, 2022 • edited Loading

kokosing commented Mar 9, 2022

kokosing commented Mar 9, 2022

Choose a reason for hiding this comment

ssheikin Mar 9, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ssheikin commented Mar 9, 2022

kokosing commented Mar 9, 2022

ssheikin commented Mar 10, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kokosing left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ssheikin commented Mar 14, 2022

hashhar left a comment

Choose a reason for hiding this comment

ssheikin commented Mar 14, 2022

kokosing left a comment

Choose a reason for hiding this comment

Update maven wrapper `0.5.6` -> `3.1.0` #11387

Update maven wrapper `0.5.6` -> `3.1.0` #11387

ssheikin commented Mar 9, 2022 •

edited

Loading

ssheikin Mar 9, 2022 •

edited

Loading