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

Java 11 #3

Closed
wants to merge 16 commits into from
Closed

Java 11 #3

wants to merge 16 commits into from

Conversation

kaifox
Copy link
Member

@kaifox kaifox commented Jun 19, 2019

Hi guys, this is a branch which makes the thing compatible with java 11 (and has to be released with java 11 as in Fontawesome there are already class files of version 55.

I would need a release of this version for my demo project for gsi ...

I see the options:

  1. simply merge back to master and continue releasing normally (knowing that this wold be the first version for java 11)
  2. produce some new releases from the branch: e.g. 0.0.10-11 (this would mean to change some logic in the deployment script)

Simplest would of course be option 1 .... (I will create a pull request... let me know what you think)

@codecov
Copy link

codecov bot commented Jun 19, 2019

Codecov Report

Merging #3 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master       #3   +/-   ##
=========================================
  Coverage     23.20%   23.20%           
  Complexity       76       76           
=========================================
  Files            51       51           
  Lines           737      737           
  Branches         76       76           
=========================================
  Hits            171      171           
  Misses          550      550           
  Partials         16       16           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 08ded24...015e38e. Read the comment docs.

@andreacalia
Copy link
Member

andreacalia commented Jun 20, 2019

If the problem is only the dependencies + configuration, we could use a similar approach as jmad-modelpack-fx :)

In the build.gradle you can fetch one dep or the other depending on the jdk version :)

if(JavaVersion.current() <= JavaVersion.VERSION_1_8){
    compile group: 'org.controlsfx', name: 'controlsfx', version: '8.40.14'
} else {
    compile group: 'org.controlsfx', name: 'controlsfx', version: '9.0.0'
}

This could solve it temporarily until we have a clear decision for CERN dependency..

@kaifox
Copy link
Member Author

kaifox commented Jun 21, 2019

This sounds good ... We still need a release per java version, right? ... because this 'if' will not work for transitive projects...

@andreacalia
Copy link
Member

exactly. We would still release for jdk8 for the moment if you need it for a demo. When would you need a released version of this?
I hope a decision will be made soon at CERN so we could move directly to Java 11.

Anyway, we could also have a separate repository in Bintray (not synchronized with JCenter) and fetch the Java 11 version from there. The release mechanism will be similar but the dependant projects will need to add this specific repo on the build.gradle.. Also this will be likely a new branch...

I would say if you can wait a bit would be better, but we should definitely tackle this in case the decision here takes more time then anticipated or we have to stay with jdk8..

@kaifox
Copy link
Member Author

kaifox commented Jun 21, 2019

Mhmm .. I think I would like to get something working soon ... However, probably a new repo is a good idea.... the additional "-11" suffix you do not like too much, I assume? (This is what I am trying right now ...)... (I blocked the mavencentral sync for those for the moment)

@kaifox
Copy link
Member Author

kaifox commented Jun 21, 2019

One point is: Even i CERN moves to java 11 .. there might still be some need for java 8 deps...

@kaifox
Copy link
Member Author

kaifox commented Jun 21, 2019

I just messed up the releases (by typing 1.1.1 instead of 0.1.1....) ... But the principle seems to work: Having versions like 1.1.1 for java 8, 1.1.1-11 for java 11 .... Not sure what the version: '+' woould take in this case ....

Would you think something like this would be ok? (Actually it is borrowed from Fontawesome ;-) ... If it is ok, we could merge the branch ... as the same branch now can deliver both versions (via 2 travis builds)

@andreacalia
Copy link
Member

Hi Kajetan, don't worry for the release, if it is not synched with maven central I think we could simply remove the artifact from bintray and the release from github :)

Anyway, ok if you need it soon then I would tend to use the -8 since is the jdk8 version that is getting behind :) What do you think?

The two repositories option is a temporary solution in order to have only one version published to the public.. so in your apps you would have to put the other repository to fetch minifx. But it would need some tests since I never did it..

@@ -25,6 +33,12 @@ buildscript {
}
}


plugins {
Copy link
Member

Choose a reason for hiding this comment

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

Not 100% sure, but this could be removed since it is an alternative syntax of apply plugin that is done in the if jdk >= 11

Copy link
Member Author

Choose a reason for hiding this comment

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

I think that in the apply plugin syntax, there is no way to specify a version ... I will check

Copy link
Member Author

@kaifox kaifox left a comment

Choose a reason for hiding this comment

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

Concerning the -8 version ... I am not sure ... it seems if the two versions exist:
1.1.1
1.1.1-10

then a 'version: "+"' picks up the 1.1.1-10 .... So i wonder if it would do the same with 1.1.1-8 (did not try) ... In the end this concept might not be ideal as there is no well defined order anymore ... so probably your proposal with 2 repos is better ... However, not sure if a temporary solution is good enough ...

I did not find the button to remove the versions.... Can you point me to it?

@@ -25,6 +33,12 @@ buildscript {
}
}


plugins {
Copy link
Member Author

Choose a reason for hiding this comment

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

I think that in the apply plugin syntax, there is no way to specify a version ... I will check

@andreacalia
Copy link
Member

Link shared :)
Yes, I agree that having two release paths in the same repository can be misleading, this is the reason why I proposed another repo.. this solution will be temporary since only one can be synchronized with jcenter, so at some point we have to decide which one. Not a straight forward decision since if CERN stays with jdk8 then the external artifact importer will not work with custom repositories.. we can discuss this over skype at some point :)
I really hope we can move forward soon

@kaifox
Copy link
Member Author

kaifox commented Jun 24, 2019

  1. Damn! Unfortunately, it seems that the minifx-workbench version (for java 8) was synced with sonatype... So I fear, we cannot go back from there ... So I think we have to continue counting up from 1.1.1.... It is a pity, but not a tragedy... Sorry for this mistake!

@kaifox
Copy link
Member Author

kaifox commented Jun 24, 2019

I searched a bit more and somehow it looks as a proper way to do these different jars would be a 'classifier'.
See https://maven.apache.org/pom.html (search for classifier). I think this is exactly our use-case...
what would you think?

In gradle one would then have to use:

compile group: 'org.minifx', name: 'minifx-workbench', version: '1.1.1', classifier: 'jdk11'

(I am not sure which one is picked, if the classifier is not given)

@smac89
Copy link

smac89 commented Jun 26, 2019

Is this ready to be released? I just took a look at the java-11 branch and I don't see any module-info.java. The only changes seem to be in the build.gradle

@kaifox
Copy link
Member Author

kaifox commented Jun 26, 2019

Dear @smac89, from the functionality point of view it is ready to be released. (For the moment it is a struggle with libraries, where some only work with java8, others only with java 11).
However, you are right we did not take care about the module system yet, as we still do not use it ...
Are you familiar with this? Would you like to contribute?
For the moment I am still experimenting how to correctly (or as correctly as possible ;-) release 2 different jars with qualifiers for the same version of code ;-)

kaifox added 2 commits June 26, 2019 23:04
classifier system property now directly queried in deployment-script
@kaifox
Copy link
Member Author

kaifox commented Jun 26, 2019

I tell you ... again STUUPID me ;-)
classifiers will NOT work! It is not the jar which is different in our case, but the transient deps ;-).... So there is no way around a version suffix.... In the end (after some reading), there is even a name for it: It's called "qualifier" (not classifier ;-) ....
https://docs.oracle.com/middleware/1212/core/MAVEN/maven_version.htm#MAVEN8905

From the documentation there are 2 main things to keep in mind:

  1. It must not be a sole number, as in this case it would be interpreted as a build number (and it behaves exactly as I described before (1.1.1-10 > 1.1.1)
  2. versions with a qualifier are considered older than versions without .... (1.1.1-jdk8 < 1.1.1) ;-)

especially because of (2), @andreacalia s remark is totally valid: we should qualify the old version not the new one.

==> Therefore, I propose:

  • '-jdk8' as qualifier for the java8 compatible version
  • the bare version for jdk11 and above...

What do you think?

@kaifox
Copy link
Member Author

kaifox commented Jun 26, 2019

A first trial release of this can be found in bintray with version 0.1.1 (I know it is lower than 1.1.1).

In case you all agree with this solution, I would then:

  1. merge this branch into master
  2. potentially delete the 0.1.1 version
  3. re-enable sync to maven central
  4. release v1.1.2 which would then have the right qualifiers.

Concerning the module-info.java, I created a separate issue #4 . I know it is a bit inconsistent to not do it directly, but the thing serves the urgent need for the moment ;-)

@kaifox kaifox mentioned this pull request Jun 26, 2019
@andreacalia
Copy link
Member

andreacalia commented Jul 1, 2019

Ciao kajetan! Sorry for the delay..
Let's just think about the naming again haha is it possible that in jdk14 (next lts) there will be again breaking changes? Just asking because otherwise the naming convention will not work anymore since we would need a jdk11 for the previous artifacts (that we cannot rename)..

I think using -jdk8 for the old ones is only valid if then we don't want to maintain backward compatibility with the main release path.. (just aiming for the current lts jdk)

What do you think? I think we can try with what you propose kajetan

@kaifox
Copy link
Member Author

kaifox commented Jul 5, 2019

Hi again, so Andrea, what would you propose as naming?

@smac89
Copy link

smac89 commented Jul 6, 2019

If I'm understanding this naming issue correctly, you guys are looking for a way to release the jars so that you can target different java versions?

Have you considered developing something called a multi-release jar? This will allow you to continue with the current naming scheme (i.e. no changes to the name), but still target the correct java version using just one jar.

There are many nice articles online on how to do this, but this one seems the most promising:
https://blog.codefx.org/tools/multi-release-jars-multiple-java-versions/#Creating-Multi-release-JARs

@kaifox
Copy link
Member Author

kaifox commented Jul 7, 2019

I think our problem is not only to release jars for different jdk versions, but that the tranient dependencies have to be different for different jdk versions ... this means that we need different poms for different jdks... Therefore , I think a multi-release jar would not help. Or would it?

@andreacalia
Copy link
Member

andreacalia commented Jul 15, 2019

Hi @smac89 , thanks! I didn't know this feature 🙂
Unfortunately this will not help us I think, as @kaifox pointed out.. but maybe I'm missing something... do you think this will help with the dependency problem?

@kaifox
Copy link
Member Author

kaifox commented Apr 23, 2020

Not needed anymore, as we desided to go to java11 directly.
The potential useful module-info.java description is tracked in issue #4

@kaifox kaifox closed this Apr 23, 2020
@kaifox kaifox deleted the java-11 branch April 23, 2020 20:24
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.

3 participants