Skip to content

Allow Multi-release to leverage Java 17 (11 minimum)#136

Closed
dsgrieve wants to merge 5 commits intomainfrom
dsgrieve/bump-jdk-to-version-17
Closed

Allow Multi-release to leverage Java 17 (11 minimum)#136
dsgrieve wants to merge 5 commits intomainfrom
dsgrieve/bump-jdk-to-version-17

Conversation

@dsgrieve
Copy link
Contributor

It would be nice to be able to take advantage of some of the language features that have been added since JDK 11.

@dsgrieve dsgrieve requested review from a user, brunoborges and karianna October 13, 2021 12:50
@brunoborges
Copy link
Member

The sort of PR where builds must fail :-)

@dsgrieve
Copy link
Contributor Author

The sort of PR where builds must fail :-)

Heh. Works on my machine.

Meant to be the sort of PR that engenders discussion.

@karianna
Copy link
Member

So this forces clients of GCToolKit to use 17 no?

@rokon12
Copy link
Contributor

rokon12 commented Oct 13, 2021

Does it mean we can refactor some code, e.g. replacing the old switch case with an enhanced switch case?

@brunoborges
Copy link
Member

Isn't it possible to write code in 17 and target 11?

Also, couldn't we use multi-release JARs ?

@karianna
Copy link
Member

Isn't it possible to write code in 17 and target 11?

using the --release targets in build should catch when you use something that can't run in an 11 JVM (which sadly will be most of the cool things).

Also, couldn't we use multi-release JARs?

If someone is willing to experiment and offer a seamless experience, why not!

@karianna karianna added the question Further information is requested label Oct 13, 2021
@karianna karianna marked this pull request as draft October 13, 2021 17:43
@dsgrieve
Copy link
Contributor Author

The point of moving up is to use some of the new features. But you can't back compile new features. Enhanced switch or instanceof pattern matching, for example, will fail compilation if you use --release 11. And using multi-release jars would me having to split the code (branches, src/11, src/17, or some such) whenever a new feature was used. Or we could use a class file re-writer. :)

Honestly, this will likely cause issues for another project I'm working on.

@ghost
Copy link

ghost commented Oct 13, 2021

As much as I'd like to move to 17, I'd first like to get Censum ported to the existing version before doing so. It's very likely that this is a safe jump.. but...

@brunoborges
Copy link
Member

@dsgrieve I'm talking about this: https://docs.oracle.com/en/java/javase/11/docs/specs/jar/jar.html#multi-release-jar-files

It allows us to only revert to an older version on specific .java files, not an entire project.

@dsgrieve
Copy link
Contributor Author

@brunoborges The issue is, if I change a file and use a 17 language feature, then I cannot compile that class to Java 11. So if you want to multi-release, you have to maintain a version 11 of that file and a version 17 of that file, in some fashion. If the changes are few, you could have a release-11 branch that applies the necessary diffs to the main branch. But this wouldn't be viable when you have good folks like @rokon12 chomping at the bit to improve the code base by using new features.

@ghost
Copy link

ghost commented Oct 13, 2021

I'm really on the fence on this one. I'm personally working in JDK 16 and will likely move to 17 very shortly. So, part of me is saying, let us run the tests in JDK 17 and if it's all green, let's do it. Then, I'm also thinking about some of the developers I know who expressed interest that are stuck using their corporate supplied laptops that have just been allowed to move up to JDK 11... so, working in JDK 17... where does it leave them. Then the other part of my brain says, do we even have any of these developers in our audience. The answer to that question... I don't know and if they are not here now, would moving to JDK 17 cause them to be excluded in the future... I simply don't know. We need a @brunoborges surgery :-)

@brunoborges
Copy link
Member

let us run the tests in JDK 17 and if it's all green, let's do it

@kirk-microsoft I guess you missed that we have been testing (and passing tests!) with Java 17 for about a week already.

https://github.com/microsoft/gctoolkit/runs/3833322712?check_suite_focus=true

@ghost
Copy link

ghost commented Oct 13, 2021

@brunoborges, I guess I have. How can we address the other concerns?

Pass the java version to an env var to extract inside Maven.
Pass Java version from the command line
@brunoborges
Copy link
Member

So the idea is that src/main/java is 11, while we can use some 17 features inside src/main/java17.

@karianna
Copy link
Member

I think I'd like to see this PR (or another) with a small example of this working E2E (WRT to multi-release jars)

@brunoborges brunoborges changed the title Bump JDK from 11 to 17 Allow Multi-release to leverage Java 17 (11 minimum) Oct 14, 2021
@dsgrieve
Copy link
Contributor Author

dsgrieve commented Oct 14, 2021

Commit 126ce85 follows https://github.com/meterware/multirelease-parent

I made one small change to DateTimeStamp.java and put it in api/src/main/java17

diff ./api/src/main/java/com/microsoft/gctoolkit/time/DateTimeStamp.java ./api/src/main/java17/com/microsoft/gctoolkit/time/DateTimeStamp.java 
132,133c132
<         if ( obj instanceof  DateTimeStamp) {
<             DateTimeStamp other = (DateTimeStamp)obj;
---
>         if (obj instanceof  DateTimeStamp other) {

The only testing I have done on this is ./mvnw -Dmulti_release clean package -DskipTests and

find . -type f -name DateTimeStamp.class
./api/target/classes/META-INF/versions/17/com/microsoft/gctoolkit/time/DateTimeStamp.class
./api/target/classes/com/microsoft/gctoolkit/time/DateTimeStamp.class

So, it seems to work as a PoC.

FWIW, here is my ~/.m2/toolchains.xml

<toolchains>
  <!-- JDK toolchains -->
  <toolchain>
    <type>jdk</type>
    <provides>
      <version>11</version>
    </provides>
    <configuration>
      <jdkHome>/Library/Java/JavaVirtualMachines/adoptopenjdk-11.jdk/Contents/Home</jdkHome>
    </configuration>
  </toolchain>
  <toolchain>
    <type>jdk</type>
    <provides>
      <version>17</version>
    </provides>
    <configuration>
      <jdkHome>/Library/Java/JavaVirtualMachines/microsoft-17.jdk/Contents/Home</jdkHome>
    </configuration>
  </toolchain>
 
</toolchains>

@karianna
Copy link
Member

I'm now more excited by this. Does the '1' JAR still get deployed to Maven central? If so I'm good with this as a concept and we can cleanup this PR and add some docs to CONTRIBUTING to make it clear developers have to support both versions for awhile.

@brunoborges
Copy link
Member

Does the '1' JAR still get deployed to Maven central?

Yes. JUnit 5 and Azure SDKs are good examples.

@karianna
Copy link
Member

OK, unless there are any other objections I'd say go for it! @dsgrieve - sorry to add extra burden but can I ask you to complete this PR (I think there's some tidying in the PMO file) and perhaps add the relevant docs to the CONTRIBUTING.md? I think the toolchains file will need to be mentioned for starters.

Showcasing modern Java though, I like it :-)

@karianna
Copy link
Member

I'm really on the fence on this one. I'm personally working in JDK 16 and will likely move to 17 very shortly. So, part of me is saying, let us run the tests in JDK 17 and if it's all green, let's do it. Then, I'm also thinking about some of the developers I know who expressed interest that are stuck using their corporate supplied laptops that have just been allowed to move up to JDK 11... so, working in JDK 17... where does it leave them. Then the other part of my brain says, do we even have any of these developers in our audience. The answer to that question... I don't know and if they are not here now, would moving to JDK 17 cause them to be excluded in the future... I simply don't know. We need a @brunoborges surgery :-)

This point is very valid, we may well have corporate folks (or others) who can't develop on 17. Let's keep this open and see what folks say. @brunoborges Twitter poll?

As a side note - this is what a mailing list would be useful for? I'm not sure how we can broadcast this sort of thing.

@dsgrieve
Copy link
Contributor Author

dsgrieve commented Nov 2, 2021

Moving forward with this could potentially create two code streams where any fix in one would have to be applied to the other.

@dsgrieve dsgrieve closed this Nov 2, 2021
@brunoborges brunoborges deleted the dsgrieve/bump-jdk-to-version-17 branch November 2, 2021 16:56
@karianna karianna added the wontfix This will not be worked on label Nov 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

question Further information is requested wontfix This will not be worked on

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants