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

Download and install Zulu Embedded Java from .tar #459

Merged
merged 109 commits into from
Jan 10, 2019

Conversation

mstormi
Copy link
Contributor

@mstormi mstormi commented Nov 2, 2018

We need to change the installation method of Zulu Java as Azul systems does not maintain Java repos any more.

Signed-off-by: Markus Storm [email protected] (github: mstormi)

@mstormi
Copy link
Contributor Author

mstormi commented Nov 2, 2018

To fix #457 and #453.
See also #450.

if is_arm; then
JAVA=zulu8.31.1.122-jdk1.8.0_181-linux_aarch32hf
else
JAVA=zulu8.31.0.1-jdk8.0.181-linux_x64
Copy link
Member

Choose a reason for hiding this comment

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

Should we assume here that anything not arm is 64-bit?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We have no check for 32/64 bit and latest Zulu is not available in 64 bit on x86 so I think it is somewhat safe to assume that we want this to be 64bit always, wdyt?

else
JAVA=zulu8.31.0.1-jdk8.0.181-linux_x64
fi
querytext="Downloading Zulu implies to agree to Azul Systems' Terms of Use. Display them now or proceed with downloading."
Copy link
Member

Choose a reason for hiding this comment

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

Quick check from @kaikreuzer, is copying the terms of use and displaying them here sufficient in your opinion?

Copy link
Contributor Author

@mstormi mstormi Nov 13, 2018

Choose a reason for hiding this comment

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

note that as part of the install the user also has to confirm he read the terms

Copy link
Member

@BClark09 BClark09 left a comment

Choose a reason for hiding this comment

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

Hi @mstormi, thanks for this. I had a few comments and checks before merging.

functions/java-jre.sh Outdated Show resolved Hide resolved
functions/java-jre.sh Outdated Show resolved Hide resolved
@EliasGabrielsson
Copy link
Contributor

Thanks @BClark09 for the reviewing!
I have some time this upcoming weekend for test and merge PR:s.

@EliasGabrielsson EliasGabrielsson added this to the Image v1.5 milestone Nov 15, 2018
@EliasGabrielsson EliasGabrielsson removed this from the Image v1.5 milestone Dec 19, 2018
@BClark09
Copy link
Member

@mstormi would you be willing to rebase your PR or resolve the conflicts?

Copy link
Member

@ThomDietrich ThomDietrich left a comment

Choose a reason for hiding this comment

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

Hey @mstormi thanks for handling the issue! I've added a few comments but I hope that none of them take much time to attend to! Enjoy your holidays! ;)

Azul is not obligated to provide you with any support for Zulu (unless you have entered into a separate written agreement for such support,
in which case such separate agreement shall apply).
You also represent and warrant that you do not intend to distribute the software in a manner that is not compliant with relevant export control
laws or regulations administered by the U.S. Commerce Department, OFAC, or any other government agency.
Copy link
Member

Choose a reason for hiding this comment

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

Is this file copy-pasted from here? https://www.azul.com/products/zulu-and-zulu-enterprise/zulu-terms-of-use/
If so I'd suggest to (1) reformat it and (2) additionally show the link as a source and for users to follow.

local FILE="/var/tmp/.zulu.$$"
local INSTALLROOT=/opt/jdk

if is_arm; then
Copy link
Member

Choose a reason for hiding this comment

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

Please add a comment with the following URL above: https://www.azul.com/downloads/zulu-embedded

I'm not at all a fan of adding the version number as a fixed string. Could you please reach out their forum to find out if there is a static redircting link. You can also use the chance to ask about their Terms of Use.
This request should not block the merge of this PR.

functions/java-jre.sh Outdated Show resolved Hide resolved
fi
querytext="Downloading Zulu implies to agree to Azul Systems' Terms of Use. Display them now or proceed with downloading."
while ! (whiptail --title "Download Zulu Java" --yes-button "Download and Install" --no-button "Read Terms of Use" --defaultno --yesno "$querytext" 10 80) ; do
whiptail --textbox /opt/openhabian/docs/azul-zulu-license.md --scrolltext 27 116
Copy link
Member

@ThomDietrich ThomDietrich Dec 26, 2018

Choose a reason for hiding this comment

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

$BASEDIR/docs

functions/java-jre.sh Outdated Show resolved Hide resolved
cd ${INSTALLROOT}; tar xpzf $FILE
cond_redirect update-alternatives --install /usr/bin/java java ${INSTALLROOT}/${JAVA}/bin/java 1083000
cond_redirect update-alternatives --install /usr/bin/javac java ${INSTALLROOT}/${JAVA}/bin/javac 1083000
rm -f $FILE
Copy link
Member

Choose a reason for hiding this comment

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

Please rearange so that the script

  1. Downloads temporary file, upon success
  2. Extracts archive to temporary location, upon success
  3. Deletes old versions (rm -f $INSTALLROOT/*)
  4. Move temporary to final location
  5. Call update-alternatives

Catching and handling errors is important :)

@ThomDietrich
Copy link
Member

Thanks ;) @BClark09 @EliasGabrielsson anything else you want to comment on?

else
local JAVA=zulu8.33.0.1-jdk8.0.192-linux_x64
fi
whiptail --textbox $BASEDIR/includes/azul-zulu-license.md --scrolltext 27 116
Copy link
Member

Choose a reason for hiding this comment

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

"docs"? Please test your code before committing changes

Copy link
Contributor Author

@mstormi mstormi Dec 29, 2018

Choose a reason for hiding this comment

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

Yes it was working, pointing to docs which is where I had put the license.
I moved it to includes now, that's how I understood your comment.

Do you want me remove Oracle as part of this PR, too ?

@mstormi
Copy link
Contributor Author

mstormi commented Jan 3, 2019

it does, it's using the x86 file if not is_arm()
you still want me to refactor that ? Wouldn't make sense to me, that's more lines of code.

@EliasGabrielsson
Copy link
Contributor

it does, it's using the x86 file if not is_arm()
you still want me to refactor that ? Wouldn't make sense to me, that's more lines of code.

Compared to the zulu8-embedded software is zulu8 still updated at the apt repository. That means we shall keep the current routine as that ensures updates are pushed out.

So there must be two methods even if it's more line of codes.
I can see if I will get some time next weekend to code on this and help you out with that refactoring / part!

@mstormi
Copy link
Contributor Author

mstormi commented Jan 4, 2019

Compared to the zulu8-embedded software is zulu8 still updated at the apt repository.

Did you verify that ?
Just got another reply pointing out they provide no guarantees whatsoever to the community.

@ThomDietrich
Copy link
Member

@mstormi I just realized that this should be a good tradeoff solution:

curl -s https://www.azul.com/downloads/zulu-embedded  | grep -Eo "http://[a-zA-Z0-9./?=_-]*aarch32hf.tar.gz"

Of course you need to add a simple check to see if the result is an empty string and then this should be fine to use also for future releases

@mstormi
Copy link
Contributor Author

mstormi commented Jan 9, 2019

@mstormi I just realized that this should be a good tradeoff solution:

Using the latest code isn't always the best choice as it can be buggy. Using older Java on the other hand (hypothetically whenever a next release comes out but we don't notice) does not do harm. Remember every openHABian user has been installing u152 from the repo with the existing code.
My current code is more conservative but I feel it is the safer bet so prefer to leave it like that.

@ThomDietrich
Copy link
Member

Using the latest code isn't always the best choice as it can be buggy.

😳 if Azul is confident enough to publish a new release on their webpage, we have to trust in their judgement and follow through. That is the way we follow with every apt repository we install packages from. Be it the deprecated azul repo or any other. Please reconsider.

@mstormi
Copy link
Contributor Author

mstormi commented Jan 9, 2019

I'm a bit more conservative there, maybe due to my job where I keep seeing this can often be a problem.
So no I don't want to change that so please don't ask me to. Why not merge my PR and if you really really want to change this then make another followup PR yourself.

@EliasGabrielsson
Copy link
Contributor

I also think Thomas's script is the way to go for zulu8-embedded. This reduce code maintenance similar to using an apt repository. As our voluntary resources are few we should devote them to more important stuff. I even think we shall push it even further and call this java installation function from the menu option 02 | Upgrade system as well.

@ThomDietrich
Copy link
Member

Good point Elias.

@mstormi I understand your concern and your request for a merge. I agree, we should not keep this important PR waiting any longer. I'll give it a last review.

Copy link
Member

@ThomDietrich ThomDietrich left a comment

Choose a reason for hiding this comment

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

Everything seems fine, thanks for this important improvement! 😉 👏

@ThomDietrich ThomDietrich merged commit 6f6154d into openhab:master Jan 10, 2019
@EliasGabrielsson
Copy link
Contributor

A hint for next time please rebase on master instead of merge the code.
I also noticed that the script is broken for x86 installation as the download link is wrong, pointing to zulu-embedded instead of zulu.

@ThomDietrich
Copy link
Member

ThomDietrich commented Jan 10, 2019

Could you clarify? In general it's almost always the better choice to "Squash merge" a PR, especially when it consists of multiple commits that you do not want in the master branch.

About x86: 😕 Could one of you please create a PR?

@EliasGabrielsson
Copy link
Contributor

Don't meant to be picky when reviewing stuff but look at this commit message: 6f6154d
Of course could we as maintainers go over everything by hand but I would prefer that the authors of PRs just do a rebase on master and squash theirs' commits.

@ThomDietrich
Copy link
Member

ThomDietrich commented Jan 11, 2019

Ah now I get it. I admit, the history of this branch/PR is a bit messed up and I could have cleaned the commit message. On the other side I have to say that I hate it when contributors rewrite the history of a branch in the middle of the review process. Rewriting git history is evil - doing it during the merge into master is the best time for this ritual.

If I would have to do it again, I'd again choose the squash merge option but actively reduce the commit message to its essence. Agreed?

@EliasGabrielsson
Copy link
Contributor

Sounds fair, it's in line with maintainer rules for the organization.
https://github.com/orgs/openhab/teams/maintainers/discussions/1

I know you care for details and this was a miss so lets do better next time.

ThomDietrich pushed a commit that referenced this pull request Jan 11, 2019
Signed-off-by: Thomas Dietrich <[email protected]>
EliasGabrielsson pushed a commit that referenced this pull request Jan 12, 2019
Signed-off-by: Thomas Dietrich <[email protected]>
@mstormi mstormi deleted the patch-12 branch May 8, 2019 20:20
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.