Skip to content

Require java 11#2385

Merged
rv-jenkins merged 4 commits intomasterfrom
java11
Dec 16, 2021
Merged

Require java 11#2385
rv-jenkins merged 4 commits intomasterfrom
java11

Conversation

@dwightguth
Copy link
Copy Markdown
Contributor

We have discovered issues where kompile does not correctly create symlinks if you are running K on Java 8. The simplest solution is simply to require Java 11 in order to use K.

We perform the following changes:

  • Change the Maven build to target Java 11
  • Test that java 11 features can be used in source code
  • Update documentation

@dwightguth dwightguth marked this pull request as ready for review December 16, 2021 19:00
@ehildenb
Copy link
Copy Markdown
Member

I approved, but would like @radumereuta to look at the var ... usage and see if it's OK with his styling choice as well.

It seems like we should use it sparingly, specifically only when (i) it's very clear from visual inspection what type the var will get, and (ii) when it actually makes the code more readable/less verbose. I guess the example here meets both those criteria.

@dwightguth
Copy link
Copy Markdown
Contributor Author

The reason I chose to use var in the spot I did to test the feature was because the type of the variable was effectively redundant. I agree we should limit usage of the feature to cases where it is either redundant or extremely verbose.

Copy link
Copy Markdown
Contributor

@radumereuta radumereuta left a comment

Choose a reason for hiding this comment

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

lgtm.
The var thing was just a personal opinion. I prefer typed variables, even though it's more verbose.
In this case, I guess it's necessary to force the upgrade to java 11.

@dwightguth
Copy link
Copy Markdown
Contributor Author

Technically it's not necessary because we are actually targeting java 11 bytecode and we are using the maven enforcer plugin to reject older versions of java during source builds, but I would prefer to have at least one java 11 feature in the codebase somewhere in order to make sure that something breaks if we aren't properly supporting java 11.

Also, while your preference may be for more verbose code, there's no reason to enforce that preference on all K developers if the automatically typed variable is still obviously of a certain type.

Regardless, this ought to merge as soon as the tests pass.

@radumereuta
Copy link
Copy Markdown
Contributor

Also, while your preference may be for more verbose code, there's no reason to enforce that preference on all K developers if the automatically typed variable is still obviously of a certain type.

Let me reiterate. It's just a personal preference of mine. I am not enforcing it on anyone. I will not mention it on any code review. The JVM team knows better.

I would prefer to have at least one java 11 feature in the codebase somewhere in order to make sure that something breaks if we aren't properly supporting java 11.

That's what I wanted to say. It will force my IDE to target java 11 also. I don't think you can force that through the setup files.

@rv-jenkins rv-jenkins merged commit aeb2611 into master Dec 16, 2021
@rv-jenkins rv-jenkins deleted the java11 branch December 16, 2021 22:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants