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 17 as minimal version, 2024-03 as minimal platform, updated BOM and minimal dependencies #3233

Merged
merged 14 commits into from
Nov 26, 2024

Conversation

LorenzoBettini
Copy link
Contributor

Moreover,

  • the wizards use Java 17 as the minimal version
  • in the BOM I used properties when feasible
  • explanation for ce2c171: now that Java 17 is enabled for byte code, strictfp tests had to be adapted (I was wrong about failure only in Java 21: strictfp has been removed from byte code since Java 17); Concerning org.eclipse.xtext.common.types.access.impl.AbstractTypeProviderTest.testMethods_publicStrictFpMethod_01(), I removed it, because in this test project we also use project with sources and 1.8 compilation level. I preferred to keep things as they are, and simply removed this test that wouldn't make sense anymore, anyway.

@cdietrich
Copy link
Contributor

i assume the following is planned for later

  • javaversion enum
  • wizard tests
  • other adaptations, default values / references of JavaVersion.11 usages, isAtLeast calls etc
  • reflection switches against older tps eg m2e

@LorenzoBettini
Copy link
Contributor Author

i assume the following is planned for later

  • javaversion enum

Do you mean remove old versions?

  • wizard tests

That's already implemented in this PR

  • other adaptations, default values / references of JavaVersion.11 usages, isAtLeast calls etc

If you mean remove old tests, then yes, it'll be in another PR

  • reflection switches against older tps eg m2e

Yes for switches. I don't understand m2e

@cdietrich
Copy link
Contributor

cdietrich commented Oct 29, 2024

javaversion enum
here we deprecate stuff

m2e:
org.eclipse.xtext.m2e.XtextProjectConfigurator.call(T, String, String)

@LorenzoBettini
Copy link
Contributor Author

javaversion enum here we deprecate stuff

m2e: org.eclipse.xtext.m2e.XtextProjectConfigurator.call(T, String, String)

Sorry, I don't understand

@cdietrich
Copy link
Contributor

regarding wizard.
still see
grafik

@cdietrich
Copy link
Contributor

i guess we also need to check the java 8 removal to find out what else we did back then

@cdietrich
Copy link
Contributor

@HannesWell might be able to explain the m2e stuff or we need to search the old pr
there is different m2e in current eclipses and the old minimal target platform
so we do reflection crap to deal with it

grafik

@LorenzoBettini
Copy link
Contributor Author

regarding wizard. still see grafik

The two projects are slightly different, concerning some configurations, and the first one uses Java 17 now anyway.

@cdietrich
Copy link
Contributor

am blind ?!?
grafik

@LorenzoBettini
Copy link
Contributor Author

am blind ?!? grafik

Then I got confused by another one. I'll remove the second one.

@cdietrich
Copy link
Contributor

cdietrich commented Oct 29, 2024

regaerding emf. this reminds me of the last emf updated.
there we also had a cascade in fragments genmodels etc
regarding minimal emf version.
https://github.com/eclipse/xtext-core/pull/2033/files

am not sure if we should do everything in one go given the time constraints

@LorenzoBettini
Copy link
Contributor Author

regaerding emf. this reminds me of the last emf updated. there we also had a cascade in fragments genmodels etc regarding minimal emf version. https://github.com/eclipse/xtext-core/pull/2033/files

am not sure if we should do everything in one go given the time constraints

I'd postpone that, and we'll also have to regenerate the languages

@cdietrich
Copy link
Contributor

@iloveeclipse @mehmet-karaman i wonder if you could test this branch in your env

@iloveeclipse
Copy link
Contributor

i wonder if you could test this branch in your env

@mehmet-karaman : if you can change our Xtext build to use this branch instead of master, I could try to build platform with that on next Monday and run tests.

@cdietrich : I'm out of PC till monday.

Copy link

github-actions bot commented Oct 29, 2024

Test Results

  4 850 files  ± 0    4 850 suites  ±0   2h 35m 42s ⏱️ + 4m 15s
 43 238 tests  -  4   42 653 ✅ + 5    584 💤  - 10   1 ❌ +1 
127 736 runs   - 15  125 974 ✅ +17  1 752 💤  - 30  10 ❌  - 2 

For more details on these failures, see this check.

Results for commit 8b5474b. ± Comparison against base commit e3ec7fb.

This pull request removes 11 and adds 7 tests. Note that renamed tests count towards both.
org.eclipse.xtext.common.types.access.jdt.JdtTypeProviderTest ‑ testMethods_publicStrictFpMethod_01
org.eclipse.xtext.common.types.access.jdt.SourceBasedJdtTypeProviderTest ‑ testMethods_publicStrictFpMethod_01
org.eclipse.xtext.xtext.wizard.WizardConfigurationTest ‑ allBuildSystemsUseJava11
org.eclipse.xtext.xtext.wizard.WizardConfigurationTest ‑ allBuildSystemsUseOtherJava
org.eclipse.xtext.xtext.wizard.WizardConfigurationTest ‑ p2ProjectsEnablesSourceGenerationWithTychoWhenMavenBuiltIsEnabledJ11
org.eclipse.xtext.xtext.wizard.cli.CliWizardIntegrationTest ‑ testProjectCreation[11: Maven|Plain|HIERARCHICAL|Fat Jar]
org.eclipse.xtext.xtext.wizard.cli.CliWizardIntegrationTest ‑ testProjectCreation[12: Gradle|Plain|HIERARCHICAL|Fat Jar]
org.eclipse.xtext.xtext.wizard.cli.CliWizardIntegrationTest ‑ testProjectCreation[14: Maven|Plain|HIERARCHICAL|Regular]
org.eclipse.xtext.xtext.wizard.cli.CliWizardIntegrationTest ‑ testProjectCreation[15: Gradle|Plain|HIERARCHICAL|Regular]
org.eclipse.xtext.xtext.wizard.cli.CliWizardIntegrationTest ‑ testProjectCreation[8: Maven|Plain|HIERARCHICAL|None]
…
org.eclipse.xtext.xtext.wizard.WizardConfigurationTest ‑ allBuildSystemsUseJava17
org.eclipse.xtext.xtext.wizard.WizardConfigurationTest ‑ p2ProjectsEnablesSourceGenerationWithTychoWhenMavenBuiltIsEnabled
org.eclipse.xtext.xtext.wizard.cli.CliWizardIntegrationTest ‑ testProjectCreation[11: Gradle|Plain|HIERARCHICAL|Fat Jar]
org.eclipse.xtext.xtext.wizard.cli.CliWizardIntegrationTest ‑ testProjectCreation[12: Maven|Plain|HIERARCHICAL|Regular]
org.eclipse.xtext.xtext.wizard.cli.CliWizardIntegrationTest ‑ testProjectCreation[14: Gradle|Plain|HIERARCHICAL|Regular]
org.eclipse.xtext.xtext.wizard.cli.CliWizardIntegrationTest ‑ testProjectCreation[8: Gradle|Plain|HIERARCHICAL|None]
org.eclipse.xtext.xtext.wizard.cli.CliWizardIntegrationTest ‑ testProjectCreation[9: Maven|Plain|HIERARCHICAL|Fat Jar]

♻️ This comment has been updated with latest results.

@HannesWell
Copy link
Contributor

@HannesWell might be able to explain the m2e stuff or we need to search the old pr
there is different m2e in current eclipses and the old minimal target platform
so we do reflection crap to deal with it

The changes are

So if you use m2e-2.0+ in your minimal target now you can just call ProjectConfigurationRequest.mavenProjectFacade() and ProjectConfigurationRequest.mavenProject() and remove the reflection code.

@LorenzoBettini
Copy link
Contributor Author

@cdietrich I've updated the project wizard tests 5fdf15d

I removed the duplicate mavenTychoP2J17 and renamed another one that uses JUnit 5 (that was the project I mentioned above, concerning a slightly different configuration).

@cdietrich
Copy link
Contributor

thx @LorenzoBettini
there still seem to be files e.g. launches, manifest.mf_gen mentioning
JavaSE-11

@cdietrich
Copy link
Contributor

is it ok when i push the cleanup of TargetPlatformProject?

@LorenzoBettini
Copy link
Contributor Author

is it ok when i push the cleanup of TargetPlatformProject?

What do you mean?

@cdietrich
Copy link
Contributor

-                                       <AB>IF config.javaVersion.isAtLeast(JavaVersion.JAVA17)<BB>
-                                               <repository location="https://download.eclipse.org/releases/2024-12"/>
-                                       <AB>ELSE<BB>
-                                               <repository location="https://download.eclipse.org/releases/2023-03"/>
-                                       <AB>ENDIF<BB>
+                                       <repository location="https://download.eclipse.org/releases/2024-12"/>

@LorenzoBettini
Copy link
Contributor Author

-                                       <AB>IF config.javaVersion.isAtLeast(JavaVersion.JAVA17)<BB>
-                                               <repository location="https://download.eclipse.org/releases/2024-12"/>
-                                       <AB>ELSE<BB>
-                                               <repository location="https://download.eclipse.org/releases/2023-03"/>
-                                       <AB>ENDIF<BB>
+                                       <repository location="https://download.eclipse.org/releases/2024-12"/>

Nice catch! Yes, that needs to be updated as well.

@cdietrich
Copy link
Contributor

@LorenzoBettini i have a commit i can just push on your branch

@LorenzoBettini
Copy link
Contributor Author

@LorenzoBettini i have a commit i can just push on your branch

Please go right ahead

@cdietrich
Copy link
Contributor

done

@cdietrich
Copy link
Contributor

My proposal is to bring this in. Update the reference projects.
Check the mwe and xpect builds and once these are green build a m2 milestone

wdyt @szarnekow @LorenzoBettini

@LorenzoBettini
Copy link
Contributor Author

My proposal is to bring this in. Update the reference projects. Check the mwe and xpect builds and once these are green build a m2 milestone

wdyt @szarnekow @LorenzoBettini

I agree!

@cdietrich
Copy link
Contributor

@szarnekow are you available this week?

@LorenzoBettini
Copy link
Contributor Author

So we can build a milestone this week?

@cdietrich
Copy link
Contributor

I really had hoped for Sebastian to have a look. If we want to bring it in and considering open points and my availability 2nd half of this week it needs to be asap

@cdietrich
Copy link
Contributor

cdietrich commented Nov 5, 2024

To check

  • org.eclipse.xtext.ui/src/org/eclipse/xtext/ui/util/JREContainerProvider.java
  • org.eclipse.xtext.xtext.generator/src/org/eclipse/xtext/xtext/generator/model/ManifestAccess.xtend

maybe

  • EMFGeneratorFragment2
  • complianceLevels in genmodels

@cdietrich cdietrich added this to the Release_2.38 milestone Nov 11, 2024
org.eclipse.ui.ide;bundle-version="3.13.1",
org.eclipse.ui.console;bundle-version="3.11.100",
org.eclipse.core.filesystem;bundle-version="1.9.300",
org.eclipse.core.filesystem;bundle-version="1.10.300",
org.eclipse.jdt.ui;bundle-version="3.26.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

We should try to be more consistent with the require bundle dependencies, e.g. jdt.ui 3.26 is unlikely to match jdt.core 3.37

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@szarnekow I only updated the dependencies when there was a matching one in the dev BOM, IIRC.

In general, I'm OK with keeping in sync minimal versions for the same dependencies that we have in the BOM.
For the other ones, I'd like to avoid such a massive maintenance burden...

org.eclipse.xtext.common.types.ui;bundle-version="2.37.0",
org.eclipse.emf.common;bundle-version="2.24.0",
org.eclipse.emf.common;bundle-version="2.30.0",
org.eclipse.xtext.ui;bundle-version="2.37.0",
org.eclipse.xtext.ui.shared;bundle-version="2.37.0",
org.eclipse.ui.editors;bundle-version="3.14.300",
Copy link
Contributor

@szarnekow szarnekow Nov 13, 2024

Choose a reason for hiding this comment

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

Do you know whether there exists a utility that would update all require-bundle versions to match a selected target platform? cc @HannesWell @merks @iloveeclipse

Copy link
Contributor

Choose a reason for hiding this comment

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

Not that I know of, no.

Copy link
Contributor

Choose a reason for hiding this comment

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

Do you know whether there exists a utility that would update all require-bundle versions to match a selected target platform?

Would be nice small feature in manifest editor. We have something similar in the feature editor, so may be with some advanced copy/paste work this would be doable.

Copy link
Contributor

Choose a reason for hiding this comment

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

It would indeed be nice. E.g. API-Tools could suggest to use the current version from the TP as lower bound if the bundle has changed it's version.
Or if no internals are used at all API-Tools could even try to be smart and compute the lower bound from @since tags.

@szarnekow
Copy link
Contributor

Minor pending things

  • org.eclipse.xtext.util.JavaRuntimeVersion.determineJavaVersion(String) should default to 17 and not 11
  • org.eclipse.xtext.ui.util.JREContainerProvider.PREFERRED_BREE
  • org.eclipse.xtext.xtext.generator.model.ManifestAccess.getContent()

@cdietrich
Copy link
Contributor

created #3247 for the emf update

@LorenzoBettini LorenzoBettini merged commit 7ecb428 into main Nov 26, 2024
14 of 17 checks passed
@LorenzoBettini LorenzoBettini deleted the lb_java17 branch November 26, 2024 17:06
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.

6 participants