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

Multi-release jar v2 #351

Open
wants to merge 11 commits into
base: master
Choose a base branch
from
Open

Multi-release jar v2 #351

wants to merge 11 commits into from

Conversation

rnett
Copy link
Contributor

@rnett rnett commented Jul 12, 2021

Does multi-release jars in-module using an ant task. Includes a test and a CI task to run tests on JDK 8.

Currently the added release is 9, since the only thing I plan on using is Cleaner. We could bump it to 11, but since we only need APIs from 9 (so far) I don't see a reason to, if someone is using it on 9 then they should get the Java 9 version.

Not sure if we should merge this now, or wait until it's used somewhere.

rnett added 3 commits July 6, 2021 15:06
Signed-off-by: Ryan Nett <JNett96@gmail.com>
Signed-off-by: Ryan Nett <JNett96@gmail.com>
Signed-off-by: Ryan Nett <JNett96@gmail.com>
@karllessard
Copy link
Collaborator

Thanks @rnett , what's the reason for using an ant task? The maven compiler plugin does support multi-release jars within the same module, I got it working in my workspace before, I can share the config required if you want

@rnett
Copy link
Contributor Author

rnett commented Jul 12, 2021

Yeah, can you? That's the info I found in the maven docs, see the Single Project link.

@karllessard
Copy link
Collaborator

Can you try this?

     <plugin>
        <groupId>org.apache.maven.plugins</groupId>
        <artifactId>maven-compiler-plugin</artifactId>
        <version>3.8.1</version>
        <executions>
          <execution>
            <id>default-compile</id>
            <goals>
              <goal>compile</goal>
            </goals>
            <configuration>
              <source>1.8</source>
              <target>1.8</target>
            </configuration>
          </execution>
          <execution>
            <id>compile-java-9</id>
            <phase>compile</phase>
            <goals>
              <goal>compile</goal>
            </goals>
            <configuration>
              <release>9</release>
              <source>9</source>
              <target>9</target>
              <compileSourceRoots>
                <compileSourceRoot>${project.basedir}/src/main/java9</compileSourceRoot>
              </compileSourceRoots>
              <multiReleaseOutput>true</multiReleaseOutput>
            </configuration>
          </execution>
        </executions>
      </plugin>
      <plugin>
        <groupId>org.apache.maven.plugins</groupId>
        <artifactId>maven-jar-plugin</artifactId>
        <version>3.2.0</version>
        <configuration>
          <archive>
            <manifestEntries>
              <Multi-Release>true</Multi-Release>
            </manifestEntries>
          </archive>
        </configuration>
      </plugin>

Signed-off-by: Ryan Nett <JNett96@gmail.com>
@karllessard
Copy link
Collaborator

I'm totally fine for starting with Java 9 as well, even if most users will be on 11. But there is no reason for preventing JDK9-users to benefits from the cleaners if we start using them.

Now for merging this now or later... this change is now quite subtle, I'm fine merging it now. Were you planning to add cleaner code for the 0.4.0 release?

CC\ @Craigacp

@rnett
Copy link
Contributor Author

rnett commented Jul 12, 2021

Now for merging this now or later... this change is now quite subtle, I'm fine merging it now. Were you planning to add cleaner code for the 0.4.0 release?

Perhaps? But probably not given that we're almost two tensorflow releases behind. I wanted it as part of resource scopes, which I haven't had time to get to yet but hope to start soon.

@Craigacp
Copy link
Collaborator

The only thing I'd say wrt Java 9 is that it was only supported for 6 months, and so anyone using it in production probably moved to 11 pretty quickly.

.github/workflows/ci.yml Outdated Show resolved Hide resolved
@JimClarke5
Copy link
Contributor

Java 9 is no longer supported. The current "post Java 8" long term supported version is Java 11 and many open source projects are now supporting Java 11. Also, Java 17, another long term release, is schedule to come out in Sept.
I would favor doing this with Java 11 instead of 9. From my view of the world, very few people are on 9 any more.

@karllessard
Copy link
Collaborator

All right I'm moving to the Jim's and Adam's clan now :)

@rnett , would you agree to migrate your current PR to build for Java11 instead?

Signed-off-by: Ryan Nett <JNett96@gmail.com>
Signed-off-by: Ryan Nett <JNett96@gmail.com>
@rnett
Copy link
Contributor Author

rnett commented Jul 12, 2021

Since 8 doesn't support -release, and since we're using 11 I can test the base jar with 9.

@rnett
Copy link
Contributor Author

rnett commented Jul 12, 2021

What I really want to do is build on 11 and then test on 8 but I need to play with it some more.

Signed-off-by: Ryan Nett <JNett96@gmail.com>
@rnett
Copy link
Contributor Author

rnett commented Jul 12, 2021

Also, do we need the jdk11 profile any more? I don't remember the original rational for adding it, but it causes issues with this setup since if the main jar is compiled with --release 11, the multi-release 11 classes won't be used (as far as I can tell).

It might not actually be the profile, I'm having issues locally as well, now.

Signed-off-by: Ryan Nett <JNett96@gmail.com>
Signed-off-by: Ryan Nett <JNett96@gmail.com>
Signed-off-by: Ryan Nett <JNett96@gmail.com>
@rnett
Copy link
Contributor Author

rnett commented Jul 13, 2021

Huh, the actual issue was the test giving false positives: it's not working at all.

I can't figure out why, the jar is attached as a zip and it looks fine to me.

The file structure is:

tensorflow-core-api-0.4.0-SNAPSHOT.jar
    META-INF
        maven
        versions
            11
                org
                    tensorflow
                        MRTest
        MANIFEST.MF
    org
        tensorflow
            MRTest

which seems correct, and MANIFEST.MF has Multi-Release: true.

@JimClarke5
Copy link
Contributor

JimClarke5 commented Jul 13, 2021

I doubt we need the old jdk11 profile as a separate entity, but the build will require Java 11 javac at a minimum with this change.

The constraint before was that we wanted to make sure the jars worked with Java 8 in the field when developers use the jars for developing applications. If we properly set the source and target for 8, and the source/target/version for the 11 class files, we should be ok for either. So the main class files should compile to 8, while the Java11 subdirectory should compile to 11.

The java 8 class files should be in the main part of the jar file as before, while the Java 11 specific class files should be in a folder META-INF/versions/11 within the manifest for the jar. That way Java 8 will run the jar as it knows nothing of the new manifest entry for 11. There should also be a <Multi-Release>true</Multi-Release> in the Manifest.

I could not find a clear way to do this with Maven as the Maven compiler and jar plugins have changed a lot. I did find Multi-Release JAR Files with Maven helpful.

I'll pull down your fork, and make it locally to see if anything sticks out.

@rnett
Copy link
Contributor Author

rnett commented Jul 13, 2021

It seems to be a maven/surefire issue, manually running java with a test class works fine.

@karllessard
Copy link
Collaborator

It seems to be a maven/surefire issue, manually running java with a test class works fine.

Make sure Maven is using the version of Java you want to test (mvn -version) but I don't know if you can control it in the surefire plugin

@karllessard
Copy link
Collaborator

If unit-testing multi-release JAR is something too complicated, let's skip it, anyway there is no code being really tested but only a Maven config.

@rnett
Copy link
Contributor Author

rnett commented Jul 13, 2021

Yeah, I'm running it from intellij and I see the pah/to/java/15/java ... when it launches, so I think it is. I'll try using toolchains, but if that doesn't work I'll skip it.

@JimClarke5
Copy link
Contributor

Maven Multi Release talks a lot about issues with running tests. I am not sure what a good solution is.

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.

4 participants