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

update JAVA_HOME to adoptopenjdk #67

Merged
merged 1 commit into from
Apr 27, 2019
Merged

Conversation

sravyamks
Copy link
Contributor

@sravyamks sravyamks commented Apr 1, 2019

@prakashsurya
Copy link
Contributor

Instead of making this change, can we just remove the setting of JAVA_HOME from this repository? It's awkward to do this here, for a couple reasons:

  1. This packages doesn't depend on Java at all. So if one was to only install "delphix-platform" (i.e. without "delphix-virutualization"), they'd get a system with JAVA_HOME configured, but without Java installed.

  2. Since linux has the "alternatives" command/system, I think we can more simply use /usr/bin/java which will be a symlink to the correct path (e.g. /usr/lib/jvm/...) for the java command. Thus, consumers of a "default" java can simply use /usr/bin/java instead of $JAVA_HOME/bin/java.

So, I'd suggest we change consumers to stop relying on JAVA_HOME (i.e. change the app-gate) if possible.

If this isn't possible, then I'd rather us move where JAVA_HOME into the "delphix-virtualization" package, since that's where the dependency on Java is set, and how java gets installed. Or better yet, into the java package itself, since this is a package we currently build (via the "linux-pkg" repository).

If we don't want to do either of these, then I'm OK with this change. This makes things no worse than they are today, but if we're already making changes to this code, I think it'd be a good time to reduce the technical debt here.

Sorry for the long reply; please let me know if I should clarify on anything.

Copy link
Contributor

@pzakha pzakha left a comment

Choose a reason for hiding this comment

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

I think the advantage of getting rid of JAVA_HOME is that we then do not need to modify it whenever we switch Java versions. That said we still need to change the java package dependency that each java user package (virtualization, masking, sso) has, so it won't gain us that much. We could create a new light-weight package, something like delphix-java, which can be pulled as a dependency by the other packages and would be responsible of setting JAVA_HOME (or even get rid of JAVA_HOME as Prakash has suggested). That said, I think this can be done as a follow-up work, so I don't block this.

Regarding the PR description, It would be useful to add a section which explains the various dependencies that this change has on other repositories, as well as an appliance-build-orchestrator run with the appropriate changes.

@bors
Copy link
Contributor

bors bot commented Apr 1, 2019

✌️ sravyamks can now approve this pull request

Copy link
Contributor

@prakashsurya prakashsurya left a comment

Choose a reason for hiding this comment

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

My prior comment still stands, but if we want to go forward with this approach, I approve this as-is.

bors delegate+

@basil
Copy link

basil commented Apr 2, 2019

I agree with @prakashsurya's suggestion to get rid of our reliance on JAVA_HOME. However, I think now is not the best time to break this dependency.

We still need to support illumos, and the version of java(1) on the PATH in our existing illumos-based appliance is not guaranteed to be the version that we use to build our code. On illumos-based appliances, we have the correct version installed in /opt/jdk. So to eliminate our reliance on JAVA_HOME today, we'd need to find all callers of Java (including Virtualization and Masking launch scripts, utility launch scripts, Jenkins jobs, and the Ant/Gradle builds) and update them to use /opt/jdk/bin/java on illumos and [/usr/bin/]java otherwise. Then once we drop support for illumos, we'd still need to go back through that code and remove the references to /opt/jdk, which wouldn't be necessary once illumos support is dropped. So we'd have to do two refactorings: once to transform JAVA_HOME usages to [/usr/bin/]java usages, and once to remove illumos-specific /opt/jdk references.

In contrast, if we wait until after we drop support for illumos, then we can just remove all references to /opt/jdk/bin/java and JAVA_HOME in one fell swoop. This would kill two birds with one stone.

Based on this reasoning, I think we should postpone the refactoring proposed here until after we drop support for illumos.

Copy link

@basil basil left a comment

Choose a reason for hiding this comment

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

The delphix-platform change looks good to me. I have some feedback on the Ansible changes to dlpx-app-gate and dms-core-gate, but I will leave those on Review Board once those reviews are posted there.

@prakashsurya
Copy link
Contributor

Thanks @basil. That sounds reasonable to me.

@sravyamks
Copy link
Contributor Author

Thank you everyone for reviewing - below are the updated review requests and builds

Testing:

saml-app -
changes : http://reviews.delphix.com/r/48313/

app-gate -
changes : http://reviews.delphix.com/r/48310/
build : http://selfservice.jenkins.delphix.com/job/dlpx-app-gate/job/master/job/build-package/job/pre-push/447/

masking -
changes : http://reviews.delphix.com/r/48311/
build - http://selfservice.jenkins.delphix.com/job/dms-core-gate/job/master/job/build-package/job/pre-push/73/

linux-pkg -
changes : delphix/linux-pkg#30
build : http://selfservice.jenkins.delphix.com/job/devops-gate/job/master/job/linux-pkg-build/job/master/job/userland/job/pre-push/15/

appliance-build-orchestrator-pre-push : http://selfservice.jenkins.delphix.com/job/devops-gate/job/master/job/appliance-build-orchestrator-pre-push/806/

Test java version on a VM from image created (dlpx-sravya.meda-master) -

delphix@ip-10-110-226-192:~$ java -version
openjdk version "1.8.0_202"
OpenJDK Runtime Environment (AdoptOpenJDK)(build 1.8.0_202-b08)
OpenJDK 64-Bit Server VM (AdoptOpenJDK)(build 25.202-b08, mixed mode)
delphix@ip-10-110-226-192:~$ javac -version
javac 1.8.0_202
delphix@ip-10-110-226-192:~$ echo $JAVA_HOME
/usr/lib/jvm/adoptopenjdk-java8-jdk-amd64
delphix@ip-10-110-226-192:~$ cd /usr/lib/jvm/
delphix@ip-10-110-226-192:/usr/lib/jvm$ ls
adoptopenjdk-java8-jdk-amd64

@sravyamks
Copy link
Contributor Author

bors r+

bors bot added a commit that referenced this pull request Apr 27, 2019
67: update JAVA_HOME to adoptopenjdk r=sravyamks a=sravyamks

update JAVA_HOME to adoptopenJDK

**Testing:**
Linux-pkg run using forked branch of `delphix-platform` - http://selfservice.jenkins.delphix.com/job/devops-gate/job/master/job/linux-pkg-build/job/master/job/userland/job/pre-push/15/

appliance-build-orchestrator-pre-push : http://selfservice.jenkins.delphix.com/job/devops-gate/job/master/job/appliance-build-orchestrator-pre-push/792/ (still running)

Other related changes for appliance-build-orchestrator run - 

app-gate -
changes : https://gitlab.delphix.com/sravya.meda/dlpx-app-gate/commit/d382bb892ff50a936a44d824941099b0143ed037
build : http://selfservice.jenkins.delphix.com/job/dlpx-app-gate/job/master/job/build-package/job/pre-push/442/

masking -
changes : https://gitlab.delphix.com/sravya.meda/dms-core-gate/commit/eddc19da356052cecd5a14ead45f838e07fae321
build - http://selfservice.jenkins.delphix.com/job/dms-core-gate/job/master/job/build-package/job/pre-push/71/

saml-app -
changes : https://gitlab.delphix.com/sravya.meda/saml-app/commit/66aabbcee3b47171f625a8a5da9f3a9b0ab5f54e

Verify JAVA_HOME on a VM from appliance build image -
```
delphix@ip-10-110-236-58:~$ echo $JAVA_HOME
/usr/lib/jvm/adoptopenjdk-java8-jdk-amd64
```

Co-authored-by: Sravya Meda <sravya.mks@gmail.com>
@bors
Copy link
Contributor

bors bot commented Apr 27, 2019

Build succeeded

  • continuous-integration/travis-ci/push

@bors bors bot merged commit 7656a2f into delphix:master Apr 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

5 participants