-
Notifications
You must be signed in to change notification settings - Fork 28.5k
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
[SPARK-31713][INFRA] Make test-dependencies.sh detect version string correctly #28532
Conversation
Hi, @srowen . Could you review this PR? This PR will remove the weird |
dev/test-dependencies.sh
Outdated
@@ -47,7 +47,7 @@ OLD_VERSION=$($MVN -q \ | |||
-Dexec.executable="echo" \ | |||
-Dexec.args='${project.version}' \ | |||
--non-recursive \ | |||
org.codehaus.mojo:exec-maven-plugin:1.6.0:exec) | |||
org.codehaus.mojo:exec-maven-plugin:1.6.0:exec | grep -e '[0-9]\.[0-9]\.[0-9]') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I used [0-9]
because \d
is accepted only on Mac OS.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would still match 3.1.0-SNAPSHOT
wouldn't it - was the idea not to match it?
You could exclude "SNAPSHOT" matches but that might be just a band-aid.
You might need [0-9]+
in each case to account for double-digit versions.
\d
might work with -E
but not worth it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks . Yes. It matches SNAPSHOT, too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is only grepping lines with version like string.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, just to be clear, what are you trying to match or not match here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the script, $(...)
merges two lines into one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BTW, is the following working in grep
on Mac?
You might need [0-9]+ in each case to account for double-digit versions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It works but you need -E
as that is 'extended' regex syntax, like many things. I think -E
is standard across GNU / BSD grep.
OK if you tell me this works, then I'm missing something, but it seems like this grep does not exclude the line I think you are trying to exclude, unless you want to print only the matching version from that line, and that's -o
I understand it's picking up "Falling" so I am assuming something needs to ignore this line or only extract the version. grep -v Falling
would just ignore that line
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. BTW, grep -v Falling
is not robust enough. If someone adds another warning output someday, this situation will repeat again.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll update to use grep -E '[0-9]+\.[0-9]+\.[0-9]+'
.
The falling back is happening in this PR's PR Builder like the following, but this PR builder passed. Please see
|
Test build #122636 has finished for PR 28532 at commit
|
Oh.. In Jenkins server, the new pattern seems not working. |
do you mean |
Ah, it's different issue. There exists some corrupted
|
It seems that we need to clean up |
Retest this please. |
Test build #122635 has finished for PR 28532 at commit
|
Test build #122638 has finished for PR 28532 at commit
|
|
@srowen . If you don't mind, can we merge this? This PR passed already on |
I trust your judgment; it can't really make things worse either if most PR builds are failing. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, same logic. LGTM
Thank you, @srowen and @HyukjinKwon ! |
Should we clean that specific correct corrupted one in |
…correctly ### What changes were proposed in this pull request? This PR makes `test-dependencies.sh` detect the version string correctly by ignoring all the other lines. ### Why are the changes needed? Currently, all SBT jobs are broken like the following. - https://amplab.cs.berkeley.edu/jenkins/view/Spark%20QA%20Test%20(Dashboard)/job/spark-branch-3.0-test-sbt-hadoop-3.2-hive-2.3/476/console ``` [error] running /home/jenkins/workspace/spark-branch-3.0-test-sbt-hadoop-3.2-hive-2.3/dev/test-dependencies.sh ; received return code 1 Build step 'Execute shell' marked build as failure ``` The reason is that the script detects the old version like `Falling back to archive.apache.org to download Maven 3.1.0-SNAPSHOT` when `build/mvn` did fallback. Specifically, in the script, `OLD_VERSION` became `Falling back to archive.apache.org to download Maven 3.1.0-SNAPSHOT` instead of `3.1.0-SNAPSHOT` if build/mvn did fallback. Then, `pom.xml` file is corrupted like the following at the end and the exit code become `1` instead of `0`. It causes Jenkins jobs fails ``` - <version>3.1.0-SNAPSHOT</version> + <version>Falling</version> ``` **NO FALLBACK** ``` $ build/mvn -q -Dexec.executable="echo" -Dexec.args='${project.version}' --non-recursive org.codehaus.mojo:exec-maven-plugin:1.6.0:exec Using `mvn` from path: /Users/dongjoon/APACHE/spark-merge/build/apache-maven-3.6.3/bin/mvn 3.1.0-SNAPSHOT ``` **FALLBACK** ``` $ build/mvn -q -Dexec.executable="echo" -Dexec.args='${project.version}' --non-recursive org.codehaus.mojo:exec-maven-plugin:1.6.0:exec Falling back to archive.apache.org to download Maven Using `mvn` from path: /Users/dongjoon/APACHE/spark-merge/build/apache-maven-3.6.3/bin/mvn 3.1.0-SNAPSHOT ``` **In the script** ``` $ echo $(build/mvn -q -Dexec.executable="echo" -Dexec.args='${project.version}' --non-recursive org.codehaus.mojo:exec-maven-plugin:1.6.0:exec) Using `mvn` from path: /Users/dongjoon/APACHE/spark-merge/build/apache-maven-3.6.3/bin/mvn Falling back to archive.apache.org to download Maven 3.1.0-SNAPSHOT ``` This PR will prevent irrelevant logs like `Falling back to archive.apache.org to download Maven`. ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? Pass the PR Builder. Closes #28532 from dongjoon-hyun/SPARK-31713. Authored-by: Dongjoon Hyun <dongjoon@apache.org> Signed-off-by: Dongjoon Hyun <dongjoon@apache.org> (cherry picked from commit cd5fbcf) Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>
…correctly ### What changes were proposed in this pull request? This PR makes `test-dependencies.sh` detect the version string correctly by ignoring all the other lines. ### Why are the changes needed? Currently, all SBT jobs are broken like the following. - https://amplab.cs.berkeley.edu/jenkins/view/Spark%20QA%20Test%20(Dashboard)/job/spark-branch-3.0-test-sbt-hadoop-3.2-hive-2.3/476/console ``` [error] running /home/jenkins/workspace/spark-branch-3.0-test-sbt-hadoop-3.2-hive-2.3/dev/test-dependencies.sh ; received return code 1 Build step 'Execute shell' marked build as failure ``` The reason is that the script detects the old version like `Falling back to archive.apache.org to download Maven 3.1.0-SNAPSHOT` when `build/mvn` did fallback. Specifically, in the script, `OLD_VERSION` became `Falling back to archive.apache.org to download Maven 3.1.0-SNAPSHOT` instead of `3.1.0-SNAPSHOT` if build/mvn did fallback. Then, `pom.xml` file is corrupted like the following at the end and the exit code become `1` instead of `0`. It causes Jenkins jobs fails ``` - <version>3.1.0-SNAPSHOT</version> + <version>Falling</version> ``` **NO FALLBACK** ``` $ build/mvn -q -Dexec.executable="echo" -Dexec.args='${project.version}' --non-recursive org.codehaus.mojo:exec-maven-plugin:1.6.0:exec Using `mvn` from path: /Users/dongjoon/APACHE/spark-merge/build/apache-maven-3.6.3/bin/mvn 3.1.0-SNAPSHOT ``` **FALLBACK** ``` $ build/mvn -q -Dexec.executable="echo" -Dexec.args='${project.version}' --non-recursive org.codehaus.mojo:exec-maven-plugin:1.6.0:exec Falling back to archive.apache.org to download Maven Using `mvn` from path: /Users/dongjoon/APACHE/spark-merge/build/apache-maven-3.6.3/bin/mvn 3.1.0-SNAPSHOT ``` **In the script** ``` $ echo $(build/mvn -q -Dexec.executable="echo" -Dexec.args='${project.version}' --non-recursive org.codehaus.mojo:exec-maven-plugin:1.6.0:exec) Using `mvn` from path: /Users/dongjoon/APACHE/spark-merge/build/apache-maven-3.6.3/bin/mvn Falling back to archive.apache.org to download Maven 3.1.0-SNAPSHOT ``` This PR will prevent irrelevant logs like `Falling back to archive.apache.org to download Maven`. ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? Pass the PR Builder. Closes #28532 from dongjoon-hyun/SPARK-31713. Authored-by: Dongjoon Hyun <dongjoon@apache.org> Signed-off-by: Dongjoon Hyun <dongjoon@apache.org> (cherry picked from commit cd5fbcf) Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>
Shall we file a JIRA and assign @shaneknapp? I think he asked this before :-). |
I left a comment on the existing JIRA. But, new JIRA sounds good. |
What changes were proposed in this pull request?
This PR makes
test-dependencies.sh
detect the version string correctly by ignoring all the other lines.Why are the changes needed?
Currently, all SBT jobs are broken like the following.
The reason is that the script detects the old version like
Falling back to archive.apache.org to download Maven 3.1.0-SNAPSHOT
whenbuild/mvn
did fallback.Specifically, in the script,
OLD_VERSION
becameFalling back to archive.apache.org to download Maven 3.1.0-SNAPSHOT
instead of3.1.0-SNAPSHOT
if build/mvn did fallback. Then,pom.xml
file is corrupted like the following at the end and the exit code become1
instead of0
. It causes Jenkins jobs failsNO FALLBACK
FALLBACK
In the script
This PR will prevent irrelevant logs like
Falling back to archive.apache.org to download Maven
.Does this PR introduce any user-facing change?
No.
How was this patch tested?
Pass the PR Builder.