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

chore(deps): update jdt to 3.27 #4164

Merged
merged 2 commits into from
Sep 29, 2021
Merged

Conversation

SirYwell
Copy link
Collaborator

@SirYwell SirYwell commented Sep 15, 2021

Supersedes #4161.

This JDT release adds support for Java 17. There was one breaking change with spoon that was simple to fix as the representation was just unified.

Edit: Oh well, that worked locally because I'm running it with Java 16. Seems like JDT now requires Java 11 as minimum version. How do we want to deal with that? @monperrus

Blocked by #4169 #4173

@slarse
Copy link
Collaborator

slarse commented Sep 15, 2021

Edit: Oh well, that worked locally because I'm running it with Java 16. Seems like JDT now requires Java 11 as minimum version. How do we want to deal with that? @monperrus

Well. That's a problem. Here's a discussion about it: https://www.eclipse.org/lists/cross-project-issues-dev/msg18507.html

As I see it, we don't have much more of a choice than to drop support for running Spoon with Java 8. That's unfortunate as Java 8 is supposed to have LTS support from Oracle until 2030. But it's either that, or we don't support any features after Java 16 (i.e we stagnate). Given the rapid release cycle of Java versions, I just don't see that as an option.

@SirYwell
Copy link
Collaborator Author

From my personal view, Java 11 is totally fine! From a project view, statistics of java versions spoon actually runs with would be interesting. This is likely something you have more information than me.
Looking into projects in https://github.com/INRIA/spoon/network/dependents might be useful (though it only brings up projects using maven afaik), but even then you'd need to filter projects that weren't updated in a while I guess.

@monperrus
Copy link
Collaborator

I agree, it's more strategic to move on together with JDT. Ok for me to drop Java 8.

spoon-pom/pom.xml Outdated Show resolved Hide resolved
@slarse
Copy link
Collaborator

slarse commented Sep 16, 2021

From my personal view, Java 11 is totally fine! From a project view, statistics of java versions spoon actually runs with would be interesting. This is likely something you have more information than me.
Looking into projects in https://github.com/INRIA/spoon/network/dependents might be useful (though it only brings up projects using maven afaik), but even then you'd need to filter projects that weren't updated in a while I guess.

In the open source world, I would imagine moving to Java 11 is a non-issue. It's potential commercial use cases where JDK8 still lives and thrives. And we know that there are commercial users of Spoon as we sometimes get bug reports where the reporters are unable to share the actual projects.

But like @monperrus said, we move with JDT. And from a Spoon dev point of view it's going to be really nice to have some of the QOL features that have been introduced since Java 8.

@SirYwell
Copy link
Collaborator Author

I changed a few things now:

  • Github Actions are running with Java 11 and Java 16 right now, using the Temurin builds now. There are no Java 17 builds from Adoptium yet, we might want to wait for that and add it too?
  • Extra checks and Coverage run with Java 16, as Adoptium does not provide Java 15 builds and AdoptOpenJDK won't have Java 17 builds
  • Dataflow was a little bit harder to make it work with Java 16, as I needed to update it to Gradle 7 (thanks @MartinWitt)

There are more things to do, which I would think are more appropriate in separate PRs:

  • Remove tests that only run with Java <11
  • General refactoring (I found
    private static Method _method_isDefault;
    static {
    try {
    _method_isDefault = Method.class.getMethod("isDefault");
    } catch (NoSuchMethodException | SecurityException e) {
    //spoon is running with java 7 JDK
    _method_isDefault = null;
    }
    }
    private static boolean _java8_isDefault(Method method) {
    if (_method_isDefault == null) {
    //spoon is running with java 7 JDK, all methods are not default, because java 7 does not have default methods
    return false;
    }
    try {
    return (Boolean) _method_isDefault.invoke(method);
    } catch (IllegalAccessException | IllegalArgumentException e) {
    throw new SpoonException("Calling of Java8 Method#isDefault() failed", e);
    } catch (InvocationTargetException e) {
    throw new SpoonException("Calling of Java8 Method#isDefault() failed", e.getTargetException());
    }
    }
    which seems to be even pre Java 8)
  • Implement support for Java 17 features in spoon

@SirYwell SirYwell changed the title wip: chore(deps): update jdt to Java 17 review: chore(deps): update jdt to Java 17 Sep 16, 2021
@monperrus
Copy link
Collaborator

That's really good work @SirYwell, kudos an thanks. The PR now contains many different things.

Can we put the Gradle update in a separate PR? And maybe the CI update as well? (both to be merged before this one)

Thanks!

@SirYwell SirYwell changed the title review: chore(deps): update jdt to Java 17 wip: chore(deps): update jdt to Java 17 Sep 17, 2021
@SirYwell
Copy link
Collaborator Author

Can we put the Gradle update in a separate PR? And maybe the CI update as well?

Sure. This PR now only contains the initial commits - it should build once the other PRs #4168 and #4169 are merged (and applied to this branch)

@monperrus
Copy link
Collaborator

@SirYwell could you rebase to see how it looks here? Thanks!

@SirYwell
Copy link
Collaborator Author

@monperrus done, it seems like LGTM runs with Java 8 and therefore fails to build. Everything else looks fine I guess

@monperrus
Copy link
Collaborator

LTGM, OK to merge.

(LGTM does not LGTM)

@I-Al-Istannen
Copy link
Collaborator

I-Al-Istannen commented Sep 28, 2021

We can probably make LGTM look gooder to us by setting the java version manually (see here)?

i.e. sth like:lgtm.yml

extraction:
  java:
    index:
      java_version: 11

@monperrus monperrus merged commit d54673c into INRIA:master Sep 29, 2021
@monperrus
Copy link
Collaborator

Thanks a lot @I-Al-Istannen

@monperrus
Copy link
Collaborator

We can probably make LGTM look gooder to us by setting the java version manually (see here)?

Good idea, would you give it a try?

@ggsurrel
Copy link

The title of the pull request is misleading, as I have the following stack when using the last spoon beta on a Java 17 project (I do not use Java 17 features for now) :

java.lang.IllegalArgumentException: Unrecognized option : -17
    at org.eclipse.jdt.internal.compiler.batch.Main.configure (Main.java:2985)
    at spoon.support.compiler.jdt.JDTBasedSpoonCompiler.buildUnits (JDTBasedSpoonCompiler.java:415)
    at spoon.support.compiler.jdt.JDTBasedSpoonCompiler.buildUnitsAndModel (JDTBasedSpoonCompiler.java:369)
    at spoon.support.compiler.jdt.JDTBasedSpoonCompiler.buildSources (JDTBasedSpoonCompiler.java:335)
    at spoon.support.compiler.jdt.JDTBasedSpoonCompiler.build (JDTBasedSpoonCompiler.java:116)
    at spoon.support.compiler.jdt.JDTBasedSpoonCompiler.build (JDTBasedSpoonCompiler.java:99)
    at spoon.Launcher.buildModel (Launcher.java:781)
    ...

It seems that jdt 3.27.0 only supports Java up to version 16.

@SirYwell
Copy link
Collaborator Author

It seems that jdt 3.27.0 only supports Java up to version 16.

Indeed, that's an oversight on my part.

It seems like they ship Eclipse with a JDT version that is not published to maven central. I don't know why, but I guess that's something we need to live with.

@monperrus monperrus changed the title wip: chore(deps): update jdt to Java 17 wip: chore(deps): update jdt to 3.27 Oct 19, 2021
@monperrus
Copy link
Collaborator

fixed PR title. thanks for notifying the problem.

@monperrus monperrus changed the title wip: chore(deps): update jdt to 3.27 chore(deps): update jdt to 3.27 Oct 25, 2021
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