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.

Sign up for GitHub

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

Jump to bottom

Add module-info support and run CI with Java 11 #1948

Merged
merged 7 commits into from Feb 26, 2023
Merged

Add module-info support and run CI with Java 11 #1948

merged 7 commits into from Feb 26, 2023

Conversation

aalmiray
Copy link
Contributor

Related to #1943

I can confirm that ./mvnw verify -Dmoditect.skip=true works with Java 8. This additional flag is set when running tests.
The CNFE seen at #1943 no longer occurs when running the build with Java 8.

@aalmiray aalmiray mentioned this pull request Feb 20, 2023
Merged
@velo
Copy link
Member

velo commented Feb 20, 2023

Release/snapshot process happens on jdk8... only a couple of modules are released using jdk11

@aalmiray
Copy link
Contributor Author

aalmiray commented Feb 20, 2023 •
edited

If release/snapshot happens on jdk8 then moditect cannot be used and thus no module infos.
By setting maven.compiler.source, maven.compiler.target, and maven.compiler.release (when running for sure on Java11+) the generated bytecode remains Java8 compatible even if the build uses a JDK that's not jdk8.

$ ./mvnw --version
Using maven at '/Users/aalmiray/dev/github/feign/mvnw' to run buildFile '/Users/aalmiray/dev/github/feign/pom.xml':
Apache Maven 3.8.7 (b89d5959fcde851dcb1c8946a785a163f14e1e29)
Maven home: /Users/aalmiray/.m2/wrapper/dists/apache-maven-3.8.7-bin/678cc9d4/apache-maven-3.8.7
Java version: 11.0.17, vendor: Azul Systems, Inc., runtime: /Users/aalmiray/.sdkman/candidates/java/11.0.17-zulu/zulu-11.jdk/Contents/Home
Default locale: en_US, platform encoding: UTF-8
OS name: "mac os x", version: "11.6.5", arch: "x86_64", family: "mac"

$ jarviz bytecode show --file core/target/feign-core-12.2-SNAPSHOT.jar 
Unversioned classes. Bytecode version: 52 total: 156
Unversioned classes. Bytecode version: 53 total: 1

$ jarviz module descriptor --file core/target/feign-core-12.2-SNAPSHOT.jar 
name: feign.core
version: 12.2-SNAPSHOT
open: false
automatic: false
exports:
  feign
  feign.auth
  feign.codec
  feign.optionals
  feign.querymap
  feign.stream
  feign.template
  feign.utils
requires:
  java.base mandated
  java.desktop
  java.logging

velo
velo requested changes Feb 25, 2023
View reviewed changes
Copy link
Member

@velo velo left a comment

Choose a reason for hiding this comment

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

New toolchain build in place.... build passes just have a few improvements ideas.

.circleci/config.yml Outdated Show resolved Hide resolved
annotation-error-decoder/pom.xml Outdated
<build>
<plugins>
<plugin>
<groupId>org.moditect</groupId>
Copy link
Member

Choose a reason for hiding this comment

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

Move to the top pom, and skip plugin on the one project that can't be excuted

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK. This adding the moditect plugin to the parent, enabled by default, skip on no modular projects, right?

velo
velo requested changes Feb 25, 2023
View reviewed changes
pom.xml Outdated
@@ -66,6 +66,7 @@
<project.build.sourceEncoding>UTF-8</project.build.sourceEncoding>
<project.build.resourceEncoding>UTF-8</project.build.resourceEncoding>

<moditect.skip>true</moditect.skip>
Copy link
Member

Choose a reason for hiding this comment

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

Can we make default false and only declare skip = true on modules that need it?

So most submodules will be unchanged

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Be aware that the root must skip otherwise it will trigger an error, that's why the default is set to true.

velo
velo approved these changes Feb 26, 2023
View reviewed changes
pom.xml
Comment on lines +465 to +494
<plugin>
<groupId>org.moditect</groupId>
<artifactId>moditect-maven-plugin</artifactId>
<version>${moditect-maven-plugin.version}</version>
<executions>
<execution>
<id>add-module-infos</id>
<phase>package</phase>
<goals>
<goal>add-module-info</goal>
</goals>
<configuration>
<skip>${moditect.skip}</skip>
<overwriteExistingFiles>true</overwriteExistingFiles>
<module>
<moduleInfo>
<!-- module name will be derived from filename -->
<!-- export everything -->
<exports>*;</exports>
<!-- declare services consumed by the artifact -->
<addServiceUses>true</addServiceUses>
</moduleInfo>
</module>
<jdepsExtraArgs>
<arg>--multi-release=9</arg>
</jdepsExtraArgs>
</configuration>
</execution>
</executions>
</plugin>
Copy link
Member

Choose a reason for hiding this comment

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

can we move this whole section to line 533 (outside pluginManagement), I see no reason to keep it in here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it can be safely moved.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah. I moved, push and merged... didn't realized the push got a conflict and didn't moved 🤦‍♂️

@velo velo merged commit 191fea1 into OpenFeign:master Feb 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@velo velo velo approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@aalmiray @velo