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

334 - Fix module path and image classpath for native image generation #607

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

loiclefevre
Copy link

This PR should fix the issue related in #334.

This allows to simplify native image generation via Maven with such build args inside the pom.xml file:

                    <plugin>
                        <groupId>org.graalvm.buildtools</groupId>
                        <artifactId>native-maven-plugin</artifactId>
                        <version>${native.maven.plugin.version}</version>
                        <executions>
                            <execution>
                                <id>build-native</id>
                                <goals>
                                    <goal>compile-no-fork</goal>
                                </goals>
                                <phase>package</phase>
                            </execution>
                            <execution>
                                <id>test-native</id>
                                <goals>
                                    <goal>test</goal>
                                </goals>
                                <phase>test</phase>
                            </execution>
                        </executions>
                        <configuration>
                            <verbose>true</verbose>
                            <skip>false</skip>
                            <imageName>${imageName}</imageName>
                            <fallback>false</fallback>
                            <agent>
                                <enabled>false</enabled>
                            </agent>
                            <buildArgs>
                                --module-path target/connectme-1.0.0.jar
                                -Ob
                                -march=native
                                --initialize-at-build-time=com.oracle.connect/com.oracle.connect.Main
                                --add-modules javafx.controls
                                --add-modules javafx.fxml
                                --add-modules com.oracle.connect
                                --module com.oracle.connect/com.oracle.connect.Main
                            </buildArgs>
                        </configuration>
                    </plugin>

The modification includes:

  • detecting properly if a dependency is a module (analyze jar file and look for the presence of /module-info.class
  • if it is a module, then populate --module-path arg list else populate -cp arg list

@oracle-contributor-agreement oracle-contributor-agreement bot added the OCA Verified All contributors have signed the Oracle Contributor Agreement. label Jun 20, 2024
@melix
Copy link
Collaborator

melix commented Jun 20, 2024

This looks like a rather dangerous change to me. There are numerous examples of projects which simply break when things are added on the module path. Also this changes the classpath order, which can be a problem. At the bare minimum I would add a property to make this opt-in.

@loiclefevre
Copy link
Author

This looks like a rather dangerous change to me. There are numerous examples of projects which simply break when things are added on the module path. Also this changes the classpath order, which can be a problem. At the bare minimum I would add a property to make this opt-in.

Would that help if this is enabled if and only if the main artifact (not the dependencies) appears to be a module?

@loiclefevre
Copy link
Author

Ok, I've added a new plugin parameter: inferModulePath

                    <plugin>
                        <groupId>org.graalvm.buildtools</groupId>
                        <artifactId>native-maven-plugin</artifactId>
                        <version>${native.maven.plugin.version}</version>
                        <executions>
                            <execution>
                                <id>build-native</id>
                                <goals>
                                    <goal>compile-no-fork</goal>
                                </goals>
                                <phase>package</phase>
                            </execution>
                            <execution>
                                <id>test-native</id>
                                <goals>
                                    <goal>test</goal>
                                </goals>
                                <phase>test</phase>
                            </execution>
                        </executions>
                        <configuration>
                            <verbose>true</verbose>
                            <skip>false</skip>
                            <imageName>${imageName}</imageName>
                            <fallback>false</fallback>
                            <agent>
                                <enabled>false</enabled>
                            </agent>
                            <inferModulePath>true</inferModulePath>
                            <buildArgs>
                                --module-path target/connectme-1.0.0.jar
                                -Ob
                                -march=native
                                --initialize-at-build-time=com.oracle.connect/com.oracle.connect.Main
                                --add-modules javafx.controls
                                --add-modules javafx.fxml
                                --add-modules com.oracle.connect
                                --module com.oracle.connect/com.oracle.connect.Main
                            </buildArgs>
                        </configuration>
                    </plugin>

@fniephaus
Copy link
Member

In Native Image, we have:

Added ability to promote jars from the class-path to the module-path in the native image driver. Use ForceOnModulePath = ${module-name}. Promoting a module to the module-path is equivalent to specifying it on the module-path in combination with exporting the module using --add-modules ${module-name} to the unnamed module.

Is that something that could be used instead here?

@loiclefevre
Copy link
Author

Sorry to say that's not solving the issue.
Thanks

@fniephaus
Copy link
Member

That's ok, thanks for checking whether that mechanism could be used to solve your problem.

We've discussed this PR in the NBT meeting and the biggest obstacle seems to be that this is not easily reproducible on the Gradle side. And if we only support module inference in Maven, we introduce asymmetry in our plugins which we like to avoid as much as possible.
The other concern is how useful module inference is in practice. Often, users seems to put modules on the class path because that's what they are used to. According to the Spring team, the demand for module support is still too low to be prioritized at this point.

Nonetheless, we'd like to continue this discussion. One alternative to the module inference approach would be a simple switch that puts the full class path on the module path. While that is potentially easier to implement, it's unclear whether a) this would solve your JavaFX and b) this would be useful for other use cases.

Let's keep this PR open until we have reached a consensus on how to proceed with module support in the NBT.

Again, thanks for your work so far! Much appreciated :)

@JaroslavTulach
Copy link
Collaborator

a simple switch that puts the full class path on the module path

That's basically what I did in jtulach/NativeImageModularDemo@f61ebfd#diff-abb4e0d98dcc6e2742e9b46d0f75f831b0e0dbe1feac6a85ebf38167130528a9:

I am using exec:exec Maven plugin to achieve this as I am not sure how to configure NBT to achieve the same. Simple switch <useModulePath>true</useModulePath> for doing similar change (e.g. using -p instead of -cp) might be good enough solution for a lot of users.

@fniephaus
Copy link
Member

@loiclefevre would @JaroslavTulach's proposal work for your JavaFX application?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
OCA Verified All contributors have signed the Oracle Contributor Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants