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

use javac --release, java8 minimum version #899

Merged
merged 2 commits into from
Feb 7, 2022

Conversation

soloturn
Copy link
Contributor

@soloturn soloturn commented Feb 4, 2022

fixes #898, and 2 issues:

  • javac -target -source flags are not good enough guarantee compatiblity with old versions. use --release instead.
  • plantuml uses java.time, introduced in java8. it was built always for java7 and the tools did not notice that installing and using real java7 gives compile and runtime errors.

the cost of using the --release flag is that build environments must muse java11+. but we can be sure, it runs for 8 this time :)

i used the master branch on the fork to produce executables in case you want to test beforehand:
https://github.com/soloturn/plantuml/releases

@soloturn soloturn changed the title use javac --release use javac --release, java8 minimum version Feb 4, 2022
@arnaudroques
Copy link
Contributor

Here, I don't understand why you suppress java 8 from java_version: [ 8, 11, 17 ].
We really want to build on Java 8 here :-)

@soloturn
Copy link
Contributor Author

soloturn commented Feb 4, 2022

the flag does not exist in java8, it would error out. building with java11 or java17 will give a java8 binary as we desire. contrary to the source/target flags it REALLY checks the old api and fails. if not matched, as we saw by setting --release 7 compiling with java17.

the workflow runs for 17 are a little pointless with this pull request, as they compile a java8 binary. i am planning a pull request so the java compiler version can be passed as parameter, so java17 will be able to build a java17 binary again. just to make sure the compatibility stays high as it is now.

does this make sense?

@arnaudroques
Copy link
Contributor

the flag does not exist in java8, it would error out. building with java11 or java17 will give a java8 binary as we desire. contrary to the source/target flags it REALLY checks the old api and fails. if not matched, as we saw by setting --release 7 compiling with java17.

the workflow runs for 17 are a little pointless with this pull request, as they compile a java8 binary. i am planning a pull request so the java compiler version can be passed as parameter, so java17 will be able to build a java17 binary again. just to make sure the compatibility stays high as it is now.

does this make sense?

Yes, it makes sense and sounds fine to me.
However, this is far from my area of expertise here, so, if you don't mind, I would like to know if it sounds also good to @matthew16550 and/or @The-Lum who have also worked on the CI.

In the mean time, you may fix the conflict I've caused by merging the other pull request (sorry about that...)

Thanks for your help!

@matthew16550
Copy link
Collaborator

We discussed --release in #817 and decided against because it prevents use of --add-exports which will be a problem for some tests in #681. Today I thought more about it and am fairly sure we could compile with --release then in a later step run the tests with --add-exports so it should not be a problem.

--release is helpful, it would be a pity not to use it just because of a few tests that have not even been merged, so I have no objection to adding it here.

We support running on 8, so we should test it. Whether we compile with 8 is a separate question.

The GH workflow tries to support two things:

  1. Making a jar that we might release.
  2. Checking that users can compile PlantUML for themselves with various combinations of JDK / OS.

2 is mainly for highly security conscious orgs which might not have much java expertise and just use javac / jar.

The workflow mixes together 1 & 2, it does not make 2 obvious at all, it does not even use javac directly. I was always uncomfortable with how I made this bit.

Perhaps we could change the workflow to this:

jobs:
  build:
    # Using the newest LTS Java on Ubuntu, create a potentially releasable jar (compile, test, sign)

  matrix:
    # Test the jar from "build" job
    # Also javac, jar, test (to support "2")

  release:
    # Release the JAR from "build" job (maybe as snapshot)

javac since java9 has a new flat --release to build a binary which can be run
with an older java. the public API of that older release must be followed as
well.

the flags used before, -source, -target do not do this. therefor it got
unnoticed that plantuml did not compile and run in java7 any more, despite
beeing compiled for for java7.

plantuml#898
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.

use --release flag of recent java versions
3 participants