Compatibility with m2e 1.x - 2.x #451

protoism · 2023-02-17T01:21:43Z

Added M2e 1.x support, required for Eclipse 2022-06

Fixes #450

protoism · 2023-02-17T01:23:16Z

...m.gwtplugins.gdt.eclipse.maven/src/com/google/gdt/eclipse/maven/configurators/M2eCompat.java

+ *
+ * Adaptation layer for m2e 1.x / 2.x API
+ */
+public class M2eCompat {


This file is duplicated, but a clean solution would require an entire new module, which was a bit... overkill

Possibly a dumb question, but wouldnt it be less overkill to do it the other way?

2 java files, identical, we now have to update one when the other changes

1 java file and one pom.xml, a dependency on maven core, m2e core (and the reflection util class can live there too)

Or am I missing something, and due to the "eclipse-project" nature that new module needs to have?

It's a OSGi environment, I'm simply not sure how long it will take... I'm not the right man to do the job the way you say it.
Even though I agree that it stinks.
The other solution I can think of, is just removing this M2eCompat stuff, and inlining it, so that it is exactly the same, but stinks less

i have some experience with plugin development. maybe i could help here.

Could you make a PR against this branch in foosoftsrl/gwt-eclipse-plugin with the sorts of setup one would need to do this?

Alternatively, bake in to the ReflectionUtilities API a way to pass the params to build the reflected Methods (class+methodname pairs etc), and let it return a single Method/MethodHandle or throw, and then we don't need even one copy of this class at all.

niloc132 · 2023-02-17T03:00:58Z

Build failed - if I'm reading this right, your pre-built jar was built with --release 17, and while this plugin is built with java 17, it is built with --release 11 so that it can be used with java 11.

protoism · 2023-02-17T10:26:48Z

Yes, I've seen the build failure.
The problem is tricky, and I really don't know how to fix it.
The point is that m2e is using records, and this is probably the reason why we need jdk 17 to build.

Technically speaking;

We have a class WtpMavenProjectConfigurator, derived from WTPProjectConfigurator, which overrides this method:

  public void configure(ProjectConfigurationRequest request, IProgressMonitor monitor) throws CoreException

but ProjectConfigurationRequest is a record in m2e

As long as we use methods of this record, such as mavenProject(), everything is fine, but as soon as we try to use reflection, or even invoke getClass() the compiler complains because it says that we're referring java.lang.Record

There is no real reason why the compilation must fail: WtpMavenProjectConfigurator does not need to know anything about records, and working bytecode could be generated.
Proof is that the main branch is happily consuming this record and the generated bytecode does not containing anything record-related.

So, I'd say that this compilation failure is more of a (very reasonable) sanity check: the compiler tells that this thing won't work on JDK11, because java.lang.Record won't be there. Why this sanity check is not performed when we simply invoke a record method is beyond me.

I can't think of a clean and simple way to handle this problem... I did not find any workaround. The only solution seems to be the one proposed by @keinhaar

protoism · 2023-02-17T13:18:20Z

Not that I really understand why, but using generics fixed the compilation issue.

Tested on 2022-06 and 2022-12

Does anyone want to do more testing?

niloc132 · 2023-02-24T13:24:12Z

...ins/com.gwtplugins.gdt.eclipse.core/src/com/google/gdt/eclipse/core/ReflectionUtilities.java

@@ -0,0 +1,16 @@
+/**
+ *


Populate header?

niloc132 · 2023-02-24T13:24:21Z

...ins/com.gwtplugins.gdt.eclipse.core/src/com/google/gdt/eclipse/core/ReflectionUtilities.java

+package com.google.gdt.eclipse.core;
+
+/**
+ * @author luca


Add a real note here?

niloc132 · 2023-02-24T13:30:21Z

...m.gwtplugins.gdt.eclipse.maven/src/com/google/gdt/eclipse/maven/configurators/M2eCompat.java

+ *
+ * Adaptation layer for m2e 1.x / 2.x API
+ */
+public class M2eCompat {


Possibly a dumb question, but wouldnt it be less overkill to do it the other way?

2 java files, identical, we now have to update one when the other changes

1 java file and one pom.xml, a dependency on maven core, m2e core (and the reflection util class can live there too)

Or am I missing something, and due to the "eclipse-project" nature that new module needs to have?

niloc132 · 2023-02-24T13:36:48Z

...ins/com.gwtplugins.gdt.eclipse.core/src/com/google/gdt/eclipse/core/ReflectionUtilities.java

+ */
+public class ReflectionUtilities {
+
+  public static <T, R> R invoke(T obj, String method) throws ReflectiveOperationException {


Since we always throw away the error (except when we wrap it with IllegalStateException, but we don't need to do that, since we already threw away the other error...), maybe just return Optional instead, so that these can be chained?

Also, T doesn't serve a purpose here, probably remove it.

Finally, I'm not clear how often these methods are called, but if it is more than "once when you create the project" it might be worth caching the Method that succeeded so we can just call it for free next time, or even using a MethodHandle and caching that instead? We could fail on the lookup that way (during static init of M2eCompat), and never have to try/catch when actually doing the work.

The 'T' is what makes the whole thing work. If we cast a Record to an Object, the compiler complains, if it's a generic it does not. I wish I knew why.
The correct solution is having a JDK 17 build, and a JDK 11 build, which is probaby not what we want.

MethodHandle and Optionals... it's not a bad idea.
About caching the handle at construction time, would you throw immediately or, say... use an Optional in order to fail later?

i think you should not optimize code until you have a performance problem.

The optimization here isn't for performance, but for complexity and brittleness - simpler code is easier for future maintainers to come back in 5+ years and understand what on earth we were thinking (which is very much a problem in other areas, see #453 for example). Avoiding duplication or checked-in compiled code makes it easier to only fix a bug in one place rather than have to search for other common patterns and clean them all up.

Pre-computing the method or methodhandle lets us fail fast, and, indicating right away that there is a problem in the constructor (or possibly slightly earlier) if the code we're runtime-linked against doesn't match either expected API. Assuming construction doesn't happen so early or in such a weird way that it otherwise breaks eclipse (that seems very unlikely to me, but worth testing).

Making that change makes the Optional not really matter at all, since you already know the method exists (...as long as it doesn't throw, but we can unwrap such an exception and rethrow, at least according to the current contract of the two variants).

@niloc132,
Quoting myself:

As long as we use methods of this record, such as mavenProject(), everything is fine, but as soon as we try to use reflection, or even invoke getClass() the compiler complains because it says that we're referring java.lang.Record

I'm really afraid that your proposal won't work. Not because it is wrong or whatever, but because we're using JDK 17 objects in JDK 11.

Hmm. You're calling m.getClass() already though?

The org.eclipse.m2e.core.project.configurator.ProjectConfigurationRequest class is definitely being loaded and is initialized by the time you get to the assorted configure() methods, since you have an instance of it.

Can you share the initialization error, or how to reproduce?

Here the error, it happens at compile time, the class is definitely not loaded yet :)

Error: Failed to execute goal org.eclipse.tycho:tycho-compiler-plugin:3.0.1:compile (default-compile) on project com.gwtplugins.gdt.eclipse.maven: Compilation failure: Compilation failure: Error: /home/runner/work/gwt-eclipse-plugin/gwt-eclipse-plugin/plugins/com.gwtplugins.gdt.eclipse.maven/src/com/google/gdt/eclipse/maven/configurators/M2eCompat.java:[1] Error: package com.google.gdt.eclipse.maven.configurators; Error: ^ Error: The type java.lang.Record cannot be resolved. It is indirectly referenced from required .class files Error: -> [Help 1]

https://github.com/gwt-plugins/gwt-eclipse-plugin/actions/runs/4199750062/jobs/7285005991#step:4:6450

The point is that you can invoke getClass() on a ProjectConfigurationRequest, but only if you do it in a generic method.
As least... this is what I saw

Hi @niloc132,
This is the commit which does not compile:

df22901

This is the commit where I find the generic trick (sorry they've been split by mistake)

f3bab46
e00e9cc

There's nothing smart in these commits, I'm just passing the ProjectConfigurationRequest to a generic function which does not trigger the compilation error.

So that's a compilation error, from referencing the class literal. As you're using the 2.x m2e jar on the classpath, but telling Java 17 that it should build to Java 11, it knows that this won't work - because it doesn't know that when using Java 11 there will be a 1.x jar for m2e instead.

Instead, try Class.forName("...ProjectConfigReq") to get the Class instance up front. This probably can even be done in the core project, since it doesn't explicitly reference the class by name, and then you can test each method to see if it is there.

…layer

I get a ClassCastException probably related to https://bugs.openjdk.org/browse/JDK-8277620

protoism · 2024-07-22T07:09:52Z

Closing this PR as it is targets a very old Eclipse version and it's too hacky

Compatibility with m2e 1.x - 2.x

df22901

protoism requested a review from niloc132 February 17, 2023 01:21

protoism commented Feb 17, 2023

View reviewed changes

protoism mentioned this pull request Feb 17, 2023

Maven compatibility #421

Closed

protoism added 2 commits February 17, 2023 12:14

Generics to the rescue?

f3bab46

Forgotten file

e00e9cc

niloc132 reviewed Feb 24, 2023

View reviewed changes

protoism added 6 commits February 28, 2023 01:17

More elegant (and compact) solution for the M2e 1.0 / 2.0 adaptation …

b22bbdd

…layer

Going back from MethodHandles to MethodHandle.

28bc90b

I get a ClassCastException probably related to https://bugs.openjdk.org/browse/JDK-8277620

Fixed build.properties unwanted change

64f88a6

Restored manifest file moved on error

ae12a72

Restored build.properties file

ad3ca30

Renaming classes after switching back from MethodHandle to Method

1b7a9a4

protoism closed this Jul 22, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Compatibility with m2e 1.x - 2.x #451

Compatibility with m2e 1.x - 2.x #451

protoism commented Feb 17, 2023

protoism Feb 17, 2023

niloc132 Feb 24, 2023

protoism Feb 24, 2023

keinhaar Feb 26, 2023

niloc132 Feb 26, 2023

niloc132 commented Feb 17, 2023

protoism commented Feb 17, 2023

protoism commented Feb 17, 2023

niloc132 Feb 24, 2023

niloc132 Feb 24, 2023

niloc132 Feb 24, 2023

niloc132 Feb 24, 2023

protoism Feb 24, 2023

keinhaar Feb 26, 2023

niloc132 Feb 26, 2023

protoism Feb 27, 2023

niloc132 Feb 27, 2023

protoism Feb 28, 2023

protoism Feb 28, 2023

protoism Feb 28, 2023

niloc132 Feb 28, 2023

protoism commented Jul 22, 2024

Compatibility with m2e 1.x - 2.x #451

Compatibility with m2e 1.x - 2.x #451

Conversation

protoism commented Feb 17, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

niloc132 commented Feb 17, 2023

protoism commented Feb 17, 2023

protoism commented Feb 17, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

protoism commented Jul 22, 2024