Skip to content

[SPARK-15542][SparkR] Make error message clear for script './R/install-dev.sh' when R is missing on Mac#13308

Closed
keypointt wants to merge 5 commits intoapache:masterfrom
keypointt:SPARK-15542
Closed

[SPARK-15542][SparkR] Make error message clear for script './R/install-dev.sh' when R is missing on Mac#13308
keypointt wants to merge 5 commits intoapache:masterfrom
keypointt:SPARK-15542

Conversation

@keypointt
Copy link
Contributor

@keypointt keypointt commented May 25, 2016

https://issues.apache.org/jira/browse/SPARK-15542

What changes were proposed in this pull request?

When running./R/install-dev.sh in Mac OS EI Captain environment, I got

mbp185-xr:spark xin$ ./R/install-dev.sh
usage: dirname path

This message is very confusing to me, and then I found R is not properly configured on my Mac when this script is using $(which R) to get R home.

I tried similar situation on CentOS with R missing, and it's giving me very clear error message while MacOS is not.
on CentOS:

[root@ip-xxx-31-9-xx spark]# which R
/usr/bin/which: no R in (/usr/local/sbin:/usr/local/bin:/sbin:/bin:/usr/sbin:/usr/bin:/usr/lib/jvm/java-1.7.0-openjdk.x86_64/bin:/root/bin)

but on Mac, if not found then nothing returned and this is causing the confusing message for R build failure and running R/install-dev.sh:

mbp185-xr:spark xin$ which R
mbp185-xr:spark xin$

Here I just added a clear message for this miss configuration for R when running R/install-dev.sh.

mbp185-xr:spark xin$ ./R/install-dev.sh
Cannot find R home by running 'which R', please make sure R is properly installed.

How was this patch tested?

Manually tested on local machine.

@SparkQA
Copy link

SparkQA commented May 25, 2016

Test build #59305 has finished for PR 13308 at commit 52ea7cc.

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

@keypointt
Copy link
Contributor Author

Hi @jkbradley do you mind have a look at this one? thanks a lot

R/install-dev.sh Outdated
else
else
# if command 'which R' finds no R home, then exit
if ! which R >/dev/null; then
Copy link
Member

Choose a reason for hiding this comment

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

I think "command" is the preferred way to do this. See how java is detected in spark-class.

@SparkQA
Copy link

SparkQA commented May 26, 2016

Test build #59338 has finished for PR 13308 at commit 07806de.

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

@SparkQA
Copy link

SparkQA commented May 26, 2016

Test build #59340 has finished for PR 13308 at commit 88319c0.

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

@SparkQA
Copy link

SparkQA commented May 26, 2016

Test build #59342 has finished for PR 13308 at commit cbd5163.

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

R/install-dev.sh Outdated
else
else
# if system wide R_HOME is not found, then exit
if ! [ `command -v R` ]; then
Copy link
Member

Choose a reason for hiding this comment

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

Is it not [ ! instead of ! [ -- were you able to test this? I doubt the integration tests have a path where R doesn't exist.

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 just tried on my local machine (I removed R installation), and either way is working, giving me the same message

RENs-MacBook-Pro:spark xin$ ./R/install-dev.sh
Cannot find 'R_HOME'. Please specify 'R_HOME' or make sure R is properly installed.

if [ ! is a better practise I can change to it, I'm just know very basic bash :P

Copy link
Member

Choose a reason for hiding this comment

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

CC @nchammas but I have only seen [ !

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, we typically put the ! after the test:

(In Bash, [ ... ] and test are synonyms.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks a lot for this, I've corrected it to be [ !

@SparkQA
Copy link

SparkQA commented May 26, 2016

Test build #59392 has finished for PR 13308 at commit fb27337.

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

@keypointt
Copy link
Contributor Author

Hi @srowen is there anything extra I should do?

asfgit pushed a commit that referenced this pull request May 27, 2016
…l-dev.sh' when R is missing on Mac

https://issues.apache.org/jira/browse/SPARK-15542

## What changes were proposed in this pull request?

When running`./R/install-dev.sh` in **Mac OS EI Captain** environment, I got
```
mbp185-xr:spark xin$ ./R/install-dev.sh
usage: dirname path
```
This message is very confusing to me, and then I found R is not properly configured on my Mac when this script is using `$(which R)` to get R home.

I tried similar situation on CentOS with R missing, and it's giving me very clear error message while MacOS is not.
on CentOS:
```
[rootip-xxx-31-9-xx spark]# which R
/usr/bin/which: no R in (/usr/local/sbin:/usr/local/bin:/sbin:/bin:/usr/sbin:/usr/bin:/usr/lib/jvm/java-1.7.0-openjdk.x86_64/bin:/root/bin)
```
but on Mac, if not found then nothing returned and this is causing the confusing message for R build failure and running R/install-dev.sh:
```
mbp185-xr:spark xin$ which R
mbp185-xr:spark xin$
```

Here I just added a clear message for this miss configuration for R when running `R/install-dev.sh`.
```
mbp185-xr:spark xin$ ./R/install-dev.sh
Cannot find R home by running 'which R', please make sure R is properly installed.
```

## How was this patch tested?
Manually tested on local machine.

Author: Xin Ren <iamshrek@126.com>

Closes #13308 from keypointt/SPARK-15542.

(cherry picked from commit 6ab973e)
Signed-off-by: Sean Owen <sowen@cloudera.com>
@srowen
Copy link
Member

srowen commented May 27, 2016

Merged to master/2.0

@asfgit asfgit closed this in 6ab973e May 27, 2016
@keypointt
Copy link
Contributor Author

Thank you Sean :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants