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

Set JAVA_HOME env var to OpenJDK 8 for Android builds #638

Merged
merged 1 commit into from Apr 18, 2017

Conversation

@aneeshusa
Copy link
Member

aneeshusa commented Apr 17, 2017

The new gradle builds require Java 8,
and the existing ant builds also work with Java 8.

This is easier than running many update-alternatives calls from Salt.
Moreover, this allows keeping Java 7 installed together with Java 8.

Needed for servo/servo#15773.
Follow-up to to #617 and #629.


This change is Reviewable

@aneeshusa
Copy link
Member Author

aneeshusa commented Apr 17, 2017

@MortimerGoro can you please confirm that JAVA_HOME works with the ant build, so we don't break the current Android build?

@MortimerGoro
Copy link
Contributor

MortimerGoro commented Apr 18, 2017

@aneeshusa I ran some tests in my machine:

Gradle

  • Compiles ok with JAVA_HOME set to Java8 (/usr/lib/jvm/java-8-openjdk-amd64)
  • Fails as expected with JAVA_HOME set to Java 7 (/usr/lib/jvm/java-7-openjdk-amd64): Unsupported major.minor version 52.0

Ant

  • Compiles ok with JAVA_HOME set to Java8 (/usr/lib/jvm/java-8-openjdk-amd64)
  • Compiles ok with JAVA_HOME set to Java 7 (/usr/lib/jvm/java-7-openjdk-amd64)
  • Fails as expected when setting an invalid JAVA_HOME folder: Error: JAVA_HOME is not defined correctly. This confirms that ant takes JAVA_HOME into account.

My environment:

  • Both java7 and java8 installed on Ubuntu 16.04.
  • Both android-18 and android-25 APIs installed.
  • Android tools v25.2.5. Important: Android tools >= v25.3.x breaks the old build system because they deprecated android cli command.
@MortimerGoro
Copy link
Contributor

MortimerGoro commented Apr 18, 2017

Based on these tests setting JAVA_HOME env variable seems safe.

@aneeshusa
Copy link
Member Author

aneeshusa commented Apr 18, 2017

Thanks so much @MortimerGoro! This is ready for review, r? @larsbergstrom

@larsbergstrom
Copy link
Contributor

larsbergstrom commented Apr 18, 2017

This means that we're probably not going to support cross-compiling from OSX, right? @fabricedesre Is that OK with you?

I know that the OSX cross builds have been broken for a little while (don't test => eventually breaks), but this seems to explicitly move us down a road of Linux-only cross builds. Unless that directory should also be present on OSX, too?

Otherwise, I'm fine with this, assuming it passes a try!

@fabricedesre
Copy link
Contributor

fabricedesre commented Apr 18, 2017

I don't personally care about cross compiling from OSX, being a linux user. go ahead!

@aneeshusa aneeshusa force-pushed the aneeshusa:set-java-home branch from f97c1b9 to ba46c80 Apr 18, 2017
The new gradle builds require Java 8,
and the existing ant builds also work with Java 8.

This is easier than running many `update-alternatives` calls from Salt.
Moreover, this allows keeping Java 7 installed together with Java 8.
@aneeshusa aneeshusa force-pushed the aneeshusa:set-java-home branch from ba46c80 to ca86297 Apr 18, 2017
@aneeshusa
Copy link
Member Author

aneeshusa commented Apr 18, 2017

@larsbergstrom JAVA_HOME is not required in general, we're only using it to point to Java 8 instead of Java 7 on the builders. macOS builds should still work if only Java 8 is installed, or else I believe there are similar directories JAVA_HOME can be set to.

It does make sense to template this in the future, though, so I've added a comment to that effect.
@bors-servo r=larsbergstrom

@bors-servo
Copy link
Contributor

bors-servo commented Apr 18, 2017

📌 Commit ca86297 has been approved by larsbergstrom

@bors-servo
Copy link
Contributor

bors-servo commented Apr 18, 2017

Testing commit ca86297 with merge de820a3...

bors-servo added a commit that referenced this pull request Apr 18, 2017
Set JAVA_HOME env var to OpenJDK 8 for Android builds

The new gradle builds require Java 8,
and the existing ant builds also work with Java 8.

This is easier than running many `update-alternatives` calls from Salt.
Moreover, this allows keeping Java 7 installed together with Java 8.

Needed for servo/servo#15773.
Follow-up to to #617 and #629.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/saltfs/638)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Apr 18, 2017

☀️ Test successful - status-travis
Approved by: larsbergstrom
Pushing de820a3 to master...

@bors-servo bors-servo merged commit ca86297 into servo:master Apr 18, 2017
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
You can’t perform that action at this time.