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.

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-8558][BUILD] Script /dev/run-tests fails when _JAVA_OPTIONS env var set #6956

Closed
wants to merge 4 commits into from

Conversation

fe2s
Copy link
Contributor

@fe2s fe2s commented Jun 23, 2015

No description provided.

@JoshRosen
Copy link
Contributor

Jenkins, this is ok to test.

@@ -477,7 +477,15 @@ def determine_java_version(java_exe):

raw_output = subprocess.check_output([java_exe, "-version"],
stderr=subprocess.STDOUT)
raw_version_str = raw_output.split('\n')[0] # eg 'java version "1.8.0_25"'

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 maybe better still to look for the first line starting with "java version" instead? is that not maybe more explicit?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point! Will change.

@SparkQA
Copy link

SparkQA commented Jun 23, 2015

Test build #35560 has finished for PR 6956 at commit ad577d7.

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

fe2s added 2 commits June 23, 2015 22:38
…ng, but all have 'version' word. Surrounding with spaces for the case if version word appears in _JAVA_OPTIONS
@SparkQA
Copy link

SparkQA commented Jun 23, 2015

Test build #35576 has finished for PR 6956 at commit cd455ef.

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

@SparkQA
Copy link

SparkQA commented Jun 23, 2015

Test build #35580 has finished for PR 6956 at commit 7d781a0.

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

@srowen
Copy link
Member

srowen commented Jun 24, 2015

LGTM

raw_output_lines = raw_output.split('\n')

# find raw version string, eg 'java version "1.8.0_25"'
raw_version_str = next(str for str in raw_output_lines if " version " in str)
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: str is a built-in function, so using it as a variable name will lead to spurious warnings in some Python linters. I'd just use x or s instead.

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, fixed.

@SparkQA
Copy link

SparkQA commented Jun 24, 2015

Test build #35718 has finished for PR 6956 at commit 31b6edc.

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

@JoshRosen
Copy link
Contributor

Thanks for updating. LGTM, so I'm merging this into master.

@asfgit asfgit closed this in dca21a8 Jun 24, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants