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

[#519] Add moditect-generated module-info.java files and a simple IT. #722

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

bmarwell
Copy link

@bmarwell bmarwell commented Apr 20, 2022

Fixes #519

@bmarwell
Copy link
Author

Did a quick test:
./mvnw install on JDK 8, then ./mvnw verify -pl integration-tests/unsigned-jackson on JDK11 which passed.
This should probably be added to the CI.

@bmarwell bmarwell marked this pull request as ready for review April 20, 2022 10:37
@lhazlewood
Copy link
Contributor

Just a quick note, and this is probably obvious, but this can't be merged until after a 1.0 release due to the requirement for JDK 8.

@lhazlewood lhazlewood modified the milestones: 0.12.0, 1.0 Apr 20, 2022
@bmarwell
Copy link
Author

bmarwell commented Apr 20, 2022

@lhazlewood this only requires Java 8 at RELEASE time, not at runtime.

Are you sure this is what you intended to say? 🤔

Talked to @bdemers the other day about it. We were thinking of a release profile which included an enforcer check for JDK8.
That would produce JDK7 code in all cases.

I really don't see why this needs to go into the next major version and couldn't be included in 0.12.0.

@bmarwell
Copy link
Author

Also, you might just have read past my comments mentioning 1.7 profiles and you probably just forgot to start the CI jobs.
I'm positive it builds on all jobs including JDK7 (just without module-info.java).

@bmarwell
Copy link
Author

Oohhhh I see what you mean now.
I wrote "requires JDK8 for release".

But it's only for building a release with the moditect files included. It still is running on (read carefully): JRE 7!

I updated the description to emphasize the difference between a JDK and JRE and build time and runtime. Of course you know, but I put the blame on me not being a native speaker. 🙈

@bmarwell
Copy link
Author

Huh… I cannot get the CI to work.

Groovy is generating this class jjwt/impl/target/generated-sources/groovy-stubs/test/io/jsonwebtoken/impl/io/FakeServiceDescriptorClassLoader.java:

public class NoServiceDescriptorClassLoader
        extends java.lang.ClassLoader implements
        groovy.lang.GroovyObject {
    ;

    public NoServiceDescriptorClassLoader
            (java.lang.ClassLoader parent) {
        super((java.lang.String) null, (java.lang.ClassLoader) null);
    }
}

... oddly this fails on zulu-11 and temurin-11, but not on IBM Semeru-11.

This is the Semeru Constructor:

protected ClassLoader(String classLoaderName, ClassLoader parentLoader) {
	this(checkSecurityPermission(), classLoaderName, parentLoader);
}

This for Zulu/Temurin:

    protected ClassLoader(String name, ClassLoader parent) {
        this(checkCreateClassLoader(name), name, parent);
    }

Will dig into it later.

@TriNextDennis
Copy link

@bmarwell can you revisit this PR?
I'd highly appreciate module support!

@bmarwell
Copy link
Author

Only if @lhazlewood approves. Otherwise I'm not investing time here...

@lhazlewood
Copy link
Contributor

@bmarwell you said:

Will dig into it later.

But we didn't hear anything further, so I never spent any further time reviewing since apparently it was still a work in progress.

Additionally, we only use JDK 7 at release time, so this will have to wait until JDK 7 is dropped anyway.

@bmarwell
Copy link
Author

Additionally, we only use JDK 7 at release time, so this will have to wait until JDK 7 is dropped anyway.

This is the actual blocker. I forgot the reason you did not wand to have JDK 9, 11 or 17 for build (which can compile to 7 just fine). I think this PR added a profile which enabled compiling on 9+, so the module-info would only have been present when the build would have been done with JDK 9+ (otherwise it was just missing, which is not good).

Anyway,

Will dig into it later.

But we didn't hear anything further, so I never spent any further time reviewing since apparently it was still a work in progress.

Probably just an update of dependencies as of now. Feel free to ping me if you are open to building with at least JDK 9 (target/release does not matter). Sorry I forgot to look into it.

Will do a quick rebase anyway, but it will include JDK11 for building the JDK 7 release, just as a PoC. Can change that later.

@bmarwell bmarwell force-pushed the 519_modules branch 3 times, most recently from 5066c68 to 1395d95 Compare July 18, 2024 18:10
@bmarwell
Copy link
Author

New rebased and squashed uses module-info in src/main/java9, which is a better approach. This keeps working when just running test-compile or test, while moditect runs in package phase only (modifies the jar) and thus test will not working on the integration test module.

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.

Enable JDK9+ module-info
3 participants