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

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

Merged
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.

@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

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

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
<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?

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.

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
velo added a commit that referenced this pull request Oct 7, 2024
* Adds explicit module descriptors to a subset of modules (#1943)

Fixes #1357

* Configure CI to run with Java 11

* Configure moditect for all modules, enable only on those that required it

* Do not skip moditect when running tests

* Only skip modules that don't work with moditect plugin

---------

Co-authored-by: Marvin Froeder <velo@users.noreply.github.com>
Co-authored-by: Marvin Froeder <velobr@gmail.com>
velo added a commit that referenced this pull request Oct 8, 2024
* Adds explicit module descriptors to a subset of modules (#1943)

Fixes #1357

* Configure CI to run with Java 11

* Configure moditect for all modules, enable only on those that required it

* Do not skip moditect when running tests

* Only skip modules that don't work with moditect plugin

---------

Co-authored-by: Marvin Froeder <velo@users.noreply.github.com>
Co-authored-by: Marvin Froeder <velobr@gmail.com>
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.

2 participants