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

Add JdkInstaller #315

Merged
merged 7 commits into from
Sep 9, 2020
Merged

Conversation

joycetoh8
Copy link
Collaborator

If JDK 8 is not already installed, JdkInstaller will download
the binary and source code at given path.

If JDK 8 is not already installed, JdkInstaller will download
the binary and source code at given path.
@joycetoh8 joycetoh8 marked this pull request as draft September 3, 2020 08:10
@andreban andreban added the enhancement New feature or request label Sep 7, 2020
@andreban andreban requested a review from chenlevy24 September 7, 2020 06:48
packages/core/src/lib/jdk/JdkInstaller.ts Show resolved Hide resolved
packages/core/src/lib/jdk/JdkInstaller.ts Outdated Show resolved Hide resolved
packages/core/src/lib/jdk/JdkInstaller.ts Outdated Show resolved Hide resolved
const JDK_SRC_ZIP = `jdk${JDK_VERSION}.zip`;

/**
* Checks whether JDK 8 is installed. If not installed,
Copy link
Collaborator

Choose a reason for hiding this comment

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

This comment is wrong isn't it? I can't see where this class checks whether JDK 8 is installed.

packages/core/src/lib/jdk/JdkInstaller.ts Outdated Show resolved Hide resolved
packages/core/src/lib/jdk/JdkInstaller.ts Outdated Show resolved Hide resolved
@andreban
Copy link
Member

andreban commented Sep 7, 2020

I'm wondering if we should assume the "default" flow as installing the JDK to a default place. This would make the user flow easier. Something like:

Do you want Bubblewrap to download / install a JDK now? (Answer No to re-use an existing JDK installation). (Y/n).

If the user answers yes (default), we can just install that into $HOME/.bubblewrap/jdk to make it simpler. If the user answers No, we ask them for the JDK Path.

@joycetoh8 joycetoh8 marked this pull request as ready for review September 8, 2020 21:00
@joycetoh8
Copy link
Collaborator Author

@PEConn thanks for the review! I think I've addressed your comments now.

@andreban I also considered that but ultimately went with letting the user choose the location to have more choice. But I think you're right. Making it install in a default location is simpler for the user. So I've changed it to do this instead.

Copy link
Member

@andreban andreban left a comment

Choose a reason for hiding this comment

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

LGTM, with a nit on the date on the header of JdkInstaller.

Thank you!

@@ -0,0 +1,94 @@
/*
* Copyright 2019 Google Inc. All Rights Reserved.
Copy link
Member

Choose a reason for hiding this comment

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

nit: wrong year for the copyright header.

Suggested change
* Copyright 2019 Google Inc. All Rights Reserved.
* Copyright 2020 Google Inc. All Rights Reserved.

@andreban andreban merged commit b288972 into GoogleChromeLabs:master Sep 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants