[SPARK-35458][BUILD] Use `> /dev/null` to replace `-q` in shasum #32604

Yikun · 2021-05-20T07:43:58Z

What changes were proposed in this pull request?

Use > /dev/null to replace -q in shasum validation.

Why are the changes needed?

In PR #32505 , added the shasum check on maven. The shasum -a 512 -q -c xxx.sha is used to validate checksum, the -q args is for "don't print OK for each successfully verified file", but -q arg is introduce in shasum 6.x version.

So we got the Unknown option: q.

➜  ~ uname -a
Darwin MacBook.local 19.6.0 Darwin Kernel Version 19.6.0: Mon Apr 12 20:57:45 PDT 2021; root:xnu-6153.141.28.1~1/RELEASE_X86_64 x86_64
➜  ~ shasum -v
5.84
➜  ~ shasum -q
Unknown option: q
Type shasum -h for help

it makes ARM CI failed:
[1] https://amplab.cs.berkeley.edu/jenkins/job/spark-master-test-maven-arm/

Does this PR introduce any user-facing change?

No

How was this patch tested?

shasum -a 512 -c wrong.sha > /dev/null return code 1 without print
shasum -a 512 -c right.sha > /dev/null return code 0 without print
e2e test:

rm -f build/apache-maven-3.6.3-bin.tar.gz
rm -r build/apache-maven-3.6.3-bin
mvn -v

Yikun · 2021-05-20T07:44:33Z

cc @srowen

SparkQA · 2021-05-20T08:38:16Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/43256/

gengliangwang · 2021-05-20T09:02:57Z

build/mvn

      # Assuming SHA512 here for now
      echo "Veryfing checksum from ${local_checksum}" 1>&2
-      if ! shasum -a 512 -q -c "${local_checksum}" ; then
+      if ! shasum -a 512 -c "${local_checksum}" > /dev/null 2>&1 ; then


Shall we remove "2>&1"? So that the error message can be shown

Make sense, just let wrong message print.

please update the PR description, too.

SparkQA · 2021-05-20T09:11:12Z

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/43256/

SparkQA · 2021-05-20T10:38:17Z

Test build #138734 has finished for PR 32604 at commit 3417c22.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

maropu · 2021-05-20T12:23:06Z

lgtm

gengliangwang

LGTM

SparkQA · 2021-05-20T12:58:57Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/43271/

SparkQA · 2021-05-20T13:34:58Z

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/43271/

srowen · 2021-05-20T13:50:56Z

Weird, I have 6.02 on my Macbook so it works. Yeah seems fine.

srowen · 2021-05-20T13:56:15Z

@Yikun you don't have to take this on here, but it was also noted that shasum may not be present, like on some Amazon Linux distros. We could also skip validation if it's not present by adding an additional test here.

Yikun · 2021-05-20T14:05:01Z

Weird, I have 6.02 on my Macbook so it works. Yeah seems fine.

I think the -q arg is added in shasum 6.x, but I didn't see any release note about shasum.

I also found the ubuntu 18.04 arm is using shasum 5.x, so it raise unsupport message. And my Mac is shasum 5.x is also unsupport.

And i found it because the arm ci is breaked, so I thought > /dev/null is a better and safe way to address shasum different version behaviors.

Yikun · 2021-05-20T14:13:47Z

@Yikun you don't have to take this on here, but it was also noted that shasum may not be present, like on some Amazon Linux distros. We could also skip validation if it's not present by adding an additional test here.

OK, got it, I'd like to also add a 'command -v shasum' to check the shasum is installed or not, before shasum check.

But i still think /dev/null is usefull to make 5.x and 6.x shasum work, and your idea?

srowen · 2021-05-20T14:15:46Z

I think I have shasum 6.x via brew so that's why I didn't catch it. Yes removing -q seems fine.
Yes the second idea is separate - just wondering if you had a second to add that during this change. I don't mind warning if it's not found, and proceeding.

SparkQA · 2021-05-20T14:31:34Z

Test build #138749 has finished for PR 32604 at commit 23eee59.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

Yikun · 2021-05-20T14:45:12Z

Yes the second idea is separate - just wondering if you had a second to add that during this change. I don't mind warning if it's not found, and proceeding.

If it's not found, the build mvn would be breaked with exit 2. Then, the developer can install the shasum mannually.

So, command -v is not very important in here, I prefer to keep current change in here.

( Sorry for typing slowly in mobile phone, and if any other followup, I will address after 12 hours when I back to my dev PC, or you can feel free to help to change this pr. : ) )

srowen · 2021-05-20T16:10:45Z

The change here is fine. yes the whole idea would be to not fail outright, but, maybe you're right that we can't let people skip this step.

srowen · 2021-05-20T17:24:27Z

Jenkins test this please

SparkQA · 2021-05-20T18:47:32Z

Kubernetes integration test unable to build dist.

exiting with code: 1
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/43283/

SparkQA · 2021-05-20T20:25:40Z

Test build #138761 has finished for PR 32604 at commit 23eee59.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

## What changes were proposed in this pull request? Use ` > /dev/null` to replace `-q` in shasum validation. ### Why are the changes needed? In PR #32505 , added the shasum check on maven. The `shasum -a 512 -q -c xxx.sha` is used to validate checksum, the `-q` args is for "don't print OK for each successfully verified file", but `-q` arg is introduce in shasum 6.x version. So we got the `Unknown option: q`. ``` ➜ ~ uname -a Darwin MacBook.local 19.6.0 Darwin Kernel Version 19.6.0: Mon Apr 12 20:57:45 PDT 2021; root:xnu-6153.141.28.1~1/RELEASE_X86_64 x86_64 ➜ ~ shasum -v 5.84 ➜ ~ shasum -q Unknown option: q Type shasum -h for help ``` it makes ARM CI failed: [1] https://amplab.cs.berkeley.edu/jenkins/job/spark-master-test-maven-arm/ ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? `shasum -a 512 -c wrong.sha > /dev/null` return code 1 without print `shasum -a 512 -c right.sha > /dev/null` return code 0 without print e2e test: ``` rm -f build/apache-maven-3.6.3-bin.tar.gz rm -r build/apache-maven-3.6.3-bin mvn -v ``` Closes #32604 from Yikun/patch-5. Authored-by: Yikun Jiang <[email protected]> Signed-off-by: Sean Owen <[email protected]> (cherry picked from commit 38fbc0b) Signed-off-by: Sean Owen <[email protected]>

srowen · 2021-05-20T21:00:50Z

Merged to master/3.1/3.0

dongjoon-hyun · 2021-05-20T21:14:36Z

Let me rebase and check this, @srowen . Thanks, @srowen .

dongjoon-hyun · 2021-05-20T21:15:48Z

Well, it's the same to me.

root@a0e001a6e50f:/spark# build/mvn clean
exec: curl --silent --show-error -L https://downloads.lightbend.com/scala/2.12.10/scala-2.12.10.tgz
exec: curl --silent --show-error -L https://www.apache.org/dyn/closer.lua/maven/maven-3/3.6.3/binaries/apache-maven-3.6.3-bin.tar.gz?action=download
exec: curl --silent --show-error -L https://archive.apache.org/dist/maven/maven-3/3.6.3/binaries/apache-maven-3.6.3-bin.tar.gz.sha512
Veryfing checksum from /spark/build/apache-maven-3.6.3-bin.tar.gz.sha512
build/mvn: line 81: shasum: command not found
Bad checksum from https://archive.apache.org/dist/maven/maven-3/3.6.3/binaries/apache-maven-3.6.3-bin.tar.gz.sha512

dongjoon-hyun · 2021-05-20T21:18:23Z

@Yikun and @srowen . We cannot enforce to install shasum additionally. That's another requirement.

If it's not found, the build mvn would be breaked with exit 2. Then, the developer can install the shasum mannually.

If we can ask the users to install additionally, why do we need to download Maven via wrapper on-demand by wasting lots of resources. We can just fail at Maven-enforcer plugin and add documents. It's not that easy to add another requirements.

srowen · 2021-05-20T21:21:33Z

Yes, this is not related to your issue - this makes it work with shasum 5.x

I don't know if I agree with your assessment here - AFAICT almost all distros have shasum, just stripped down ones like AWS Linux, so this is only a new requirements for a few developers (? I think). But I don't disagree with the conclusion, just warn and skip if not found.

## What changes were proposed in this pull request? Use ` > /dev/null` to replace `-q` in shasum validation. ### Why are the changes needed? In PR apache#32505 , added the shasum check on maven. The `shasum -a 512 -q -c xxx.sha` is used to validate checksum, the `-q` args is for "don't print OK for each successfully verified file", but `-q` arg is introduce in shasum 6.x version. So we got the `Unknown option: q`. ``` ➜ ~ uname -a Darwin MacBook.local 19.6.0 Darwin Kernel Version 19.6.0: Mon Apr 12 20:57:45 PDT 2021; root:xnu-6153.141.28.1~1/RELEASE_X86_64 x86_64 ➜ ~ shasum -v 5.84 ➜ ~ shasum -q Unknown option: q Type shasum -h for help ``` it makes ARM CI failed: [1] https://amplab.cs.berkeley.edu/jenkins/job/spark-master-test-maven-arm/ ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? `shasum -a 512 -c wrong.sha > /dev/null` return code 1 without print `shasum -a 512 -c right.sha > /dev/null` return code 0 without print e2e test: ``` rm -f build/apache-maven-3.6.3-bin.tar.gz rm -r build/apache-maven-3.6.3-bin mvn -v ``` Closes apache#32604 from Yikun/patch-5. Authored-by: Yikun Jiang <[email protected]> Signed-off-by: Sean Owen <[email protected]> (cherry picked from commit 38fbc0b) Signed-off-by: Sean Owen <[email protected]>

Use > /dev/null 2>&1 to replace -q in shasum

3417c22

github-actions bot added the BUILD label May 20, 2021

gengliangwang reviewed May 20, 2021

View reviewed changes

Update mvn

23eee59

Yikun changed the title ~~[SPARK-35458][BUILD] Use > /dev/null 2>&1 to replace -q in shasum~~ [SPARK-35458][BUILD] Use > /dev/null to replace -q in shasum May 20, 2021

gengliangwang approved these changes May 20, 2021

View reviewed changes

srowen closed this in 38fbc0b May 20, 2021

dongjoon-hyun mentioned this pull request May 20, 2021

[SPARK-35463][BUILD] Skip checking checksum on a system without shasum #32613

Closed

[SPARK-35458][BUILD] Use > /dev/null to replace -q in shasum #32604

[SPARK-35458][BUILD] Use > /dev/null to replace -q in shasum #32604

Uh oh!

Conversation

Yikun commented May 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

Uh oh!

Yikun commented May 20, 2021

Uh oh!

SparkQA commented May 20, 2021

Uh oh!

gengliangwang May 20, 2021

Choose a reason for hiding this comment

Uh oh!

Yikun May 20, 2021

Choose a reason for hiding this comment

Uh oh!

maropu May 20, 2021

Choose a reason for hiding this comment

Uh oh!

Yikun May 20, 2021

Choose a reason for hiding this comment

Uh oh!

SparkQA commented May 20, 2021

Uh oh!

SparkQA commented May 20, 2021

Uh oh!

maropu commented May 20, 2021

Uh oh!

gengliangwang left a comment

Choose a reason for hiding this comment

Uh oh!

SparkQA commented May 20, 2021

Uh oh!

SparkQA commented May 20, 2021

Uh oh!

srowen commented May 20, 2021

Uh oh!

srowen commented May 20, 2021

Uh oh!

Yikun commented May 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Yikun commented May 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

srowen commented May 20, 2021

Uh oh!

SparkQA commented May 20, 2021

Uh oh!

Yikun commented May 20, 2021

Uh oh!

srowen commented May 20, 2021

Uh oh!

srowen commented May 20, 2021

Uh oh!

SparkQA commented May 20, 2021

Uh oh!

SparkQA commented May 20, 2021

Uh oh!

srowen commented May 20, 2021

Uh oh!

dongjoon-hyun commented May 20, 2021

Uh oh!

dongjoon-hyun commented May 20, 2021

Uh oh!

dongjoon-hyun commented May 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

srowen commented May 20, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

[SPARK-35458][BUILD] Use `> /dev/null` to replace `-q` in shasum #32604

[SPARK-35458][BUILD] Use `> /dev/null` to replace `-q` in shasum #32604

Yikun commented May 20, 2021 •

edited

Loading

Yikun commented May 20, 2021 •

edited

Loading

Yikun commented May 20, 2021 •

edited

Loading

dongjoon-hyun commented May 20, 2021 •

edited

Loading