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

fixed Java version check in bash template #1111

Merged
merged 2 commits into from
Mar 23, 2018

Conversation

jubecker
Copy link
Contributor

The current Java version check in 'bash-template' relies on a string comparison for numbers.
As far as I could investigate this check fails at least for bash 4.3.48 on a xenial/sid based docker images.

It fails for the latest JDK release 10, as "10" is not greater than "1.6" when compared as string in bash.
Later bash version seem to treat this check differently.

I changed the version check to compare numbers. For versions 1.x.y, the check extracts the minor version "x" and compares agains "6". If the major version is unequal to "1", the major version is compared against "6".

Using docker openjdk images I added scripted test.

Tested with bash 4.3.48 and 4.4.19, docker 17.12.0-ce.

@muuki88 muuki88 added the universal Zip, tar.gz, tgz and bash issues label Mar 23, 2018
Copy link
Contributor

@muuki88 muuki88 left a comment

Choose a reason for hiding this comment

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

Thanks a lot for the well done pull request 😍

The newly written test seems to fail due to the doc task.

[info] [error] 'jvm-1.8' is not a valid choice for '-target'
[info] [error] bad option: '-target:jvm-1.8'
[info] [error] 'jvm-1.8' is not a valid choice for '-target'
[info] [error] bad option: '-target:jvm-1.8'
[info] [error] (jdk8/compile:doc) Scaladoc generation failed
[info] [error] (jdk8/compile:compileIncremental) Compilation failed

Otherwise everything looks super smooth :)

@@ -0,0 +1,31 @@
val basename = "jdk-versions"

scalacOptions in ThisBuild := Seq("-target:jvm-1.8")
Copy link
Contributor

Choose a reason for hiding this comment

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

it seems the scala doc compiler doesn't like this setting. I never actually fought
the docs compiler (only my colleagues), so sorry for not being a greater help here 😞

IIRC you can either turn the scaladoc generation off or do some research on how
to fix this 😎 I'm fine with either way 👍

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 for pointing this out. Will push a fix. Normally limiting the scope should fix this.

Copy link
Contributor

Choose a reason for hiding this comment

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

Awesome, thanks :)

@lightbend-cla-validator

Hi @jubecker,

Thank you for your contribution! We really value the time you've taken to put this together.

Before we proceed with reviewing this pull request, please sign the Lightbend Contributors License Agreement:

http://www.lightbend.com/contribute/cla

@muuki88 muuki88 merged commit 08a7a65 into sbt:master Mar 23, 2018
@muuki88
Copy link
Contributor

muuki88 commented Mar 23, 2018

Awesome :)

jochenschneider pushed a commit to advancedtelematic/ota-tuf that referenced this pull request May 25, 2018
thesamet added a commit to scalapb/ScalaPB that referenced this pull request May 27, 2018
iSignal added a commit to yugabyte/yugabyte-db that referenced this pull request Dec 10, 2019
Summary:
Wesley hit the bug mentioned in sbt/sbt-native-packager#1111 while
running on a Amazon Linux 2 image. Upgrading the packaging plugin sbt-native-packager to a later
version fixes the issue

Test Plan:
Build with this change, deploy to an Amazon Linux 2 imaged c5.large, verify that
yugabyted comes up normally

Reviewers: ram, wesley

Reviewed By: wesley

Subscribers: jenkins-bot, yugaware

Differential Revision: https://phabricator.dev.yugabyte.com/D7683
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
universal Zip, tar.gz, tgz and bash issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants