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

[ENHANCEMENT] Upgrade default JavaSE to latest LTS JDK #516

Merged
merged 4 commits into from
Jan 7, 2022
Merged

[ENHANCEMENT] Upgrade default JavaSE to latest LTS JDK #516

merged 4 commits into from
Jan 7, 2022

Conversation

RoiArthurB
Copy link
Contributor

Switching from JavaSE-11 to new JavaSE-17 which is the latest LTS (from September 2021)

@github-actions
Copy link

github-actions bot commented Jan 7, 2022

Unit Test Results

144 files  144 suites   36m 2s ⏱️
229 tests 226 ✔️ 3 💤 0

Results for commit b921c2b.

♻️ This comment has been updated with latest results.

@mickaelistria
Copy link
Contributor

IMO, this is the good path forward. It's totally fine that in default case, Tycho uses latest supported Java version instead of contributing everyone to be glued in past releases. The default behavior can be opinionated, and my opinion is that people use latest release of everything possible.

@laeubi
Copy link
Member

laeubi commented Jan 7, 2022

It's totally fine that in default case, Tycho uses latest supported Java version

I don't think so. This will break everyone using currently Java 11. Assuming that everyone always uses latest is just wrong (or will platform instantly upgrade to J17 now?). The default should be the minimum required version to run Tycho (if we ever need such a default).

instead of contributing everyone to be glued in past releases

Instead of a hard-coded default it should be the version of the running JVM, that maps to the "default JRE" in eclipse JDT and PDE-Target definition.

@laeubi
Copy link
Member

laeubi commented Jan 7, 2022

I'm attaching here a screenshot of the environment setting from the PDE target editor:

grafik

As one can see there a three options:

  1. Default (maps to the default JVM in JDT and should map to the JVM running the build in Tycho)
  2. an explicitly configured JVM (should map to an ID/name in toolchains, not yet supported by Tycho)
  3. an EE (maps to an ID in toolchains, should be improved see Tycho should find Toolchain JRE by version #276)

@mickaelistria
Copy link
Contributor

This will break everyone using currently Java 11.

I would only affect people who do not set executionEnvironment anyway, so those people are already opening the door to be broken (as some warning should mention) and it's fine to break them.
And even if we change EE to Java 17 instead of Java 11, as long as Java 17 is compatible with Java 11, it wouldn't really affect the results negatively; most things would still compile and resolve correctly, according to their BREE. In the worst case, some bundle resolution would fail because of some removed/added packages requiring to adjust the target-platform or product definition, but then people know it's an issue and can take a decision.
Keep in mind that the goal of a build tool is really not just to build things, but also to fail as early as possible when project have an error. By "being nice" and silencing some interesting erroneous behavior that is valuable to most users, we actually reduce the value/quality of the tool...

will platform instantly upgrade to J17 now

When Java 11 is EOL, in 1 year and 8 months.

@laeubi
Copy link
Member

laeubi commented Jan 7, 2022

I would only affect people who do not set executionEnvironment anyway, so those people are already opening the door to be broken (as some warning should mention) and it's fine to break them.

As outlined before I strongly disagree here. Assuming one every want do define an fixed EE is simply wrong and just a bad habit to claim those people are doing things "wrong". Especially if we just change the default to help people assuming Java-17 as their default.

When Java 11 is EOL, in 1 year and 8 months.

Then this should be the time where we switch tycho as well.

By "being nice" and silencing some interesting erroneous behavior

This is neither interesting nor erroneous. The target already defines the environment where I want to build (or my build environment) requiring explicit/duplicate configuration is not what I want.

@mickaelistria
Copy link
Contributor

This is neither interesting nor erroneous.

It is erroneous or interesting. Maybe you didn't have yet encountered the experiences where this behavior has been helpful and saved hours of debugging downstream, but I often and still have as I work with many projects and people and environments.
As mentioned on other ticket, this discussion is not solving the initial issue anyway.

@akurtakov
Copy link
Member

When Java 11 is EOL, in 1 year and 8 months.

Then this should be the time where we switch tycho as well.

This is 1 year and 8 months at latest. But we are also dependent on other projects e.g. last time move to Java 11 was driven by Jetty dependency. So in case of such event it might happen earlier than that. But there will be at least 6 months notice period so as of today the earliest possible (but very unlikely!) release that could require Java 17 is 2022-12.

@laeubi
Copy link
Member

laeubi commented Jan 7, 2022

Maybe you didn't have yet encountered the experiences where this behavior has been helpful and saved hours of debugging downstream,

Maybe but for me the error is clear from first second and I don't want a warning where it does not apply! This just makes people think warnings are something to not consider.

If I run a build that requires J17 with a J11 jvm and get an error I can easily know whats wrong there is no need for "tool support".

If I run a build that requires J11 with J11 and get warnings about using the default J11 I'm really mad. If I get additional warnings complaining about J16, 17, ... as its currently with tycho then I really think WTF?? and never "thanks for this useless warning"...

@laeubi
Copy link
Member

laeubi commented Jan 7, 2022

You used an anonymous address 16764085+RoiArthurB@****.noreplywith your commits this is not supported by ECA.

@mickaelistria
Copy link
Contributor

Please also add a comment to TargetPlatformConfigurationMojo#executionEnvironments describing that if nothign is set current Java runtime is used.
I believe some tests will also need a fix to adpat to this new behavior.

@RoiArthurB
Copy link
Contributor Author

You used an anonymous address

@laeubi I revalidate it and it's passing :)

Please also add a comment to TargetPlatformConfigurationMojo#executionEnvironments

@mickaelistria I don't know where / how the documentation is managed 🙊🙊

@RoiArthurB RoiArthurB requested a review from laeubi January 7, 2022 10:03
@laeubi
Copy link
Member

laeubi commented Jan 7, 2022

I don't know where / how the documentation is managed speak_no_evilspeak_no_evil

Documentation is generated from the javadoc at this mojo:

https://github.com/eclipse/tycho/blob/master/target-platform-configuration/src/main/java/org/eclipse/tycho/target/TargetPlatformConfigurationMojo.java

@RoiArthurB
Copy link
Contributor Author

Doc updated and code tested on my side.

Based on the Jenkins' configuration (installing only a JDK 11), every unit test should still work fine (as my modification should return JavaSE-11 in that case).

So, I think that you can restart the full test pipeline now before merging my PR 🤗

@mickaelistria mickaelistria merged commit b921c2b into eclipse-tycho:master Jan 7, 2022
@mickaelistria
Copy link
Contributor

Thanks!

@mickaelistria mickaelistria added this to the 2.7 milestone Jan 7, 2022
@Bananeweizen
Copy link
Contributor

it should be the version of the running JVM

Just wanted to mention this will be a source of failures. Developers building on different machines will see different results for the exact same git checkout (due to accidentally running with a different JRE).

Do you really want to spend time on issues raised because of that?

@laeubi
Copy link
Member

laeubi commented Jan 16, 2022

due to accidentally running with a different JRE

How can I 'accidentally' running with an incompatible JVM? Git never has put any contracts on the runtime environment, thats a different topic. If special considerations are required this needs to be documented somewhere or asserted in the pom.xml.

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.

None yet

5 participants