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.

Sign up for GitHub

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

Update maven wrapper 0.5.6 -> 3.1.0 #11387

Merged
merged 3 commits into from
Mar 14, 2022
Merged

Update maven wrapper 0.5.6 -> 3.1.0 #11387

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1399ad6
Update maven wrapper `0.5.6` -> `3.1.0`
ssheikin Mar 9, 2022
2322e2c
Do not display maven version
ssheikin Mar 10, 2022
a079541
Improve maven property evaluation
ssheikin Mar 14, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/workflows/milestone.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ jobs:
- name: Get milestone from pom.xml
run: |
.github/bin/retry ./mvnw -v
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

2:1

echo "MILESTONE_NUMBER=$(./mvnw -q -Dexec.executable=echo -Dexec.args='${project.version}' -N exec:exec | cut -d- -f1)" >> $GITHUB_ENV
echo "MILESTONE_NUMBER=$(./mvnw help:evaluate -Dexpression=project.version -q -DforceStdout | cut -d- -f1)" >> $GITHUB_ENV
- name: Set milestone to PR
uses: actions/github-script@v4
with:
Expand Down
Binary file modified .mvn/wrapper/maven-wrapper.jar
View file
Open in desktop
Binary file not shown.
89 changes: 87 additions & 2 deletions mvnw
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,10 @@

if [ -z "$MAVEN_SKIP_RC" ] ; then

if [ -f /usr/local/etc/mavenrc ] ; then
. /usr/local/etc/mavenrc
fi

if [ -f /etc/mavenrc ] ; then
. /etc/mavenrc
fi
Expand Down Expand Up @@ -145,7 +149,7 @@ if [ -z "$JAVACMD" ] ; then
JAVACMD="$JAVA_HOME/bin/java"
fi
else
JAVACMD="`which java`"
JAVACMD="`\\unset -f command; \\command -v java`"
fi
fi

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

##########################################################################################
# Extension to allow automatically downloading the maven-wrapper.jar from Maven-central
Copy link
Member

Choose a reason for hiding this comment

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

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 ?

Copy link
Contributor Author

@ssheikin ssheikin Mar 9, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

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, ok, it's still takes in effect only if the jar is not present.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe we should remove the jar then?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

# This allows using the maven wrapper in projects that prohibit checking in binary data.
##########################################################################################
if [ -r "$BASE_DIR/.mvn/wrapper/maven-wrapper.jar" ]; then
if [ "$MVNW_VERBOSE" = true ]; then
echo "Found .mvn/wrapper/maven-wrapper.jar"
fi
else
if [ "$MVNW_VERBOSE" = true ]; then
echo "Couldn't find .mvn/wrapper/maven-wrapper.jar, downloading it ..."
fi
if [ -n "$MVNW_REPOURL" ]; then
jarUrl="$MVNW_REPOURL/org/apache/maven/wrapper/maven-wrapper/3.1.0/maven-wrapper-3.1.0.jar"
else
jarUrl="https://repo.maven.apache.org/maven2/org/apache/maven/wrapper/maven-wrapper/3.1.0/maven-wrapper-3.1.0.jar"
fi
while IFS="=" read key value; do
case "$key" in (wrapperUrl) jarUrl="$value"; break ;;
esac
done < "$BASE_DIR/.mvn/wrapper/maven-wrapper.properties"
if [ "$MVNW_VERBOSE" = true ]; then
echo "Downloading from: $jarUrl"
fi
wrapperJarPath="$BASE_DIR/.mvn/wrapper/maven-wrapper.jar"
if $cygwin; then
wrapperJarPath=`cygpath --path --windows "$wrapperJarPath"`
fi

if command -v wget > /dev/null; then
if [ "$MVNW_VERBOSE" = true ]; then
echo "Found wget ... using wget"
fi
if [ -z "$MVNW_USERNAME" ] || [ -z "$MVNW_PASSWORD" ]; then
wget "$jarUrl" -O "$wrapperJarPath" || rm -f "$wrapperJarPath"
else
wget --http-user=$MVNW_USERNAME --http-password=$MVNW_PASSWORD "$jarUrl" -O "$wrapperJarPath" || rm -f "$wrapperJarPath"
fi
elif command -v curl > /dev/null; then
if [ "$MVNW_VERBOSE" = true ]; then
echo "Found curl ... using curl"
fi
if [ -z "$MVNW_USERNAME" ] || [ -z "$MVNW_PASSWORD" ]; then
curl -o "$wrapperJarPath" "$jarUrl" -f
else
curl --user $MVNW_USERNAME:$MVNW_PASSWORD -o "$wrapperJarPath" "$jarUrl" -f
fi

else
if [ "$MVNW_VERBOSE" = true ]; then
echo "Falling back to using Java to download"
fi
javaClass="$BASE_DIR/.mvn/wrapper/MavenWrapperDownloader.java"
# For Cygwin, switch paths to Windows format before running javac
if $cygwin; then
javaClass=`cygpath --path --windows "$javaClass"`
fi
if [ -e "$javaClass" ]; then
if [ ! -e "$BASE_DIR/.mvn/wrapper/MavenWrapperDownloader.class" ]; then
if [ "$MVNW_VERBOSE" = true ]; then
echo " - Compiling MavenWrapperDownloader.java ..."
fi
# Compiling the Java class
("$JAVA_HOME/bin/javac" "$javaClass")
fi
if [ -e "$BASE_DIR/.mvn/wrapper/MavenWrapperDownloader.class" ]; then
# Running the downloader
if [ "$MVNW_VERBOSE" = true ]; then
echo " - Running MavenWrapperDownloader.java ..."
fi
("$JAVA_HOME/bin/java" -cp .mvn/wrapper MavenWrapperDownloader "$MAVEN_PROJECTBASEDIR")
fi
fi
fi
fi
##########################################################################################
# End of extension
##########################################################################################

export MAVEN_PROJECTBASEDIR=${MAVEN_BASEDIR:-"$BASE_DIR"}
if [ "$MVNW_VERBOSE" = true ]; then
echo $MAVEN_PROJECTBASEDIR
Expand Down Expand Up @@ -226,6 +309,8 @@ WRAPPER_LAUNCHER=org.apache.maven.wrapper.MavenWrapperMain

exec "$JAVACMD" \
$MAVEN_OPTS \
$MAVEN_DEBUG_OPTS \
-classpath "$MAVEN_PROJECTBASEDIR/.mvn/wrapper/maven-wrapper.jar" \
"-Dmaven.home=${M2_HOME}" "-Dmaven.multiModuleProjectDirectory=${MAVEN_PROJECTBASEDIR}" \
"-Dmaven.home=${M2_HOME}" \
"-Dmaven.multiModuleProjectDirectory=${MAVEN_PROJECTBASEDIR}" \
Copy link
Member

Choose a reason for hiding this comment

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

nit: Can we move this to new commit ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No. #11387 (comment)

${WRAPPER_LAUNCHER} $MAVEN_CONFIG "$@"
3 changes: 0 additions & 3 deletions plugin/trino-hive-hadoop2/conf/hive-tests-defaults.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,5 @@
#!/usr/bin/env bash

# Download maven wrapper if it is not present so next command can have clean output
./mvnw --version >/dev/null

DEFAULT_DOCKER_VERSION=$(./mvnw help:evaluate -Dexpression=dep.docker.images.version -q -DforceStdout)

if [ -z "$DEFAULT_DOCKER_VERSION" ];
Expand Down