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 option to populate module path with all maven dependencies #91

Closed
HGuillemet opened this issue Jun 22, 2020 · 18 comments
Closed

Add option to populate module path with all maven dependencies #91

HGuillemet opened this issue Jun 22, 2020 · 18 comments
Assignees

Comments

@HGuillemet
Copy link

HGuillemet commented Jun 22, 2020

This issue is close to Issue 89.

If I understand well, the javafx-maven-plugin reconstructs the module graph from the module-infos and populated the module path only with the required modules. That's a good behaviour in the majority of cases.

But if the application needs modules that are determined dynamically by maven dependency system and cannot be added statically in module-info, this doesn't work. We would need to add all maven dependencies to the module path and use, e.g, the special --add-modules ALL-MODULE-PATH command line argument.
This concerns both run and jlink goals.

Could you consider adding this option ?

EDIT: The Apache maven-jlink-plugin has this option (but not the one that reads the module-info). Unfortunately the development of this plugin seems to have stopped.

@HGuillemet HGuillemet changed the title Add option to populate module path with maven dependencies Add option to populate module path with all maven dependencies Jun 22, 2020
@abhinayagarwal
Copy link
Collaborator

If I get it correctly, you need a configuration in the plugin which can be used to identify if all the dependency needs to be on the module-path while using javafx:run and javafx:jlink goals.

We have a similar issues #72 which helps to run an application on the classpath. May be its better to update the PR #73 to introduce a config which can be used to either accept classpath or modulepath or both (default).

@betanzos Do you have any thoughts on this?

@HGuillemet
Copy link
Author

That's it.
Something like:

<includeAllDependenciesInModulePath>true</includeAllDependenciesInModulePath>

And something to set --add-modules to all modules:

<AddAllModulePath>true</AddAllModulePatgh> 

Or maybe to some arbitrary value. I'm not sure if it's currently possible to add a service provider to the module graph with the plugin:

<AddModule>some.service.provider</AddModule>

This could replace the AddAllModulePath configuration with:

<AddModule>ALL-MODULE-PATH</AddModule>

@abhinayagarwal
Copy link
Collaborator

abhinayagarwal commented Jun 24, 2020

I am trying to fuse #72 and #91 by introducing one additional configuration. Due to a lack of a better name, I call it runtimePath. It can accept 3 values:

  1. MODULEPATH
  2. CLASSPATH
  3. DEFAULT

The options are self explanatory and helps to run an application by switching dependencies to either MODULEPATH or CLASSPATH.

Changes are available at https://github.com/abhinayagarwal/javafx-maven-plugin/tree/classpath https://github.com/abhinayagarwal/javafx-maven-plugin/tree/issue-91

Feedback is appreciated.

@abhinayagarwal
Copy link
Collaborator

abhinayagarwal commented Jun 24, 2020

I have opened PR #92 to make it easier to review the changes

@HGuillemet
Copy link
Author

What is exactly the DEFAULT runtimePath ? Put all modules from the reconstructed graph in the module path and the rest in the class path ?

I have tested and it seems to work well for the run goal, but there is an issue with the jlink goal: the command line arguments specified in <options> are used for launching the application, not the jlink command, so something like:

<option>--add-modules</option>
<option>ALL-MODULE-PATH</option>

doesn't appli to jlink, and the modules added to the module path are not added to the jlink image.

What about adding another set of <options> for the jlink command, or adding some upper-level <AddModule> configuration that applies to both jlink command and launcher script, and maybe to the run goal too ?

@abhinayagarwal
Copy link
Collaborator

What is exactly the DEFAULT runtimePath ?

module-path is constructed from the module-info.java file. If this file is absent, only the JavaFX jars are put on the module-path.

I have tested and it seems to work well for the run goal, but there is an issue with the jlink goal

The way this new configuration works is by defining a runtimePath for the plugin:

<plugin>
    <groupId>org.openjfx</groupId>
    <artifactId>javafx-maven-plugin</artifactId>
    <version>0.0.4-SNAPSHOT</version>
    <configuration>
         ...
        <runtimePath>MODULEPATH</runtimePath>
    </configuration>
</plugin>

jlink only works when a module-info.java file is present. Does your sample has this file? If yes, can you please let me know what exactly doesn't work?

@HGuillemet
Copy link
Author

jlink only works when a module-info.java file is present. Does your sample has this file? If yes, can you please let me know what exactly doesn't work?

It has a module-info.java.
I need to add to the jlink image and to the module graph modules that are in the maven dependencies but not in the requires of the module-info.

Your change adds the needed modules to the module path, but that's not enough :

  • for jlink to include the module to the image
  • for the application to access these modules at runtime

They need to be brought into the module graph, either from module-info requires, or from command line --add-modules.
With the plugin we currently can add such --add-modules with the <option> configuration, but that works for runtime only, not for jlink.

Is there something still unclear ?

@HGuillemet
Copy link
Author

HGuillemet commented Jun 24, 2020

Maybe I should precise what ALL-MODULE-PATH does. It's not a well-known feature.
From the JEP 261:

It is occasionally necessary to add modules to the default root set in order to ensure that specific platform, library, or service-provider modules will be present in the resulting module graph. In any phase the option
--add-modules <module>(,<module>)*
where <module> is a module name, adds the indicated modules to the default set of root modules. This option may be used more than once.
As a special case at run time, if <module> is ALL-DEFAULT [...]
As a further special case at run time, if <module> is ALL-SYSTEM then [...]
As a final special case, at both run time and link time, if <module> is ALL-MODULE-PATH then all observable modules found on the relevant module paths are added to the root set. ALL-MODULE-PATH is valid at both compile time and run time. This is provided for use by build tools such as Maven, which already ensure that all modules on the module path are needed. It is also a convenient means to add automatic modules to the root set.

@abhinayagarwal
Copy link
Collaborator

Hi @HGuillemet ,

Thank you for the comment with JEP. I wasn't aware of this JVM option :)

Do you have a sample against which I can run some tests?

@johanvos
Copy link
Contributor

@HGuillemet Does #92 work for you?

@HGuillemet
Copy link
Author

HGuillemet commented Jun 25, 2020

As explained above, it works but there is still something missing to configure --add-modules during the jlink image creation.

I have built a sample test case, attached to this message.

  • dep1 is a named module containing some resource
  • dep2 is an automatic module containing some resource
  • app is an application depending on dep1 and dep2 but not specifying either in its module-info. The main function lists the visible modules and try to find the resource in each.

With a jdk 9+, run mvn install on dep1 and dep2, then mvn javafx:run on app. It works.
Try mvn package : it works, but the resulting image doesn't contain dep1 and dep2 (use target/image/bin/java --list-modules or target/image/bin/go).
If you run jlink manually adding --add-modules dep1, it works, resource is found.
If you run jlink manually adding --add-modules dep2, it doesn't work, because jlink dislike automatic modules.

So we need the ability to add the modules in the module path to the root set during jlink, and it could be nice, if not complicated, to add a 4th option to runtimePath in order to only add named modules from the maven dependencies to the module path.

sample.zip

@abhinayagarwal
Copy link
Collaborator

Hi @HGuillemet,

If we use ALL-MODULE-PATH to jlink and it contains an automatic module, then the jlink with fail. I hope this is expected. Or, do you want the plugin to somehow patch the jar to make this work?

@HGuillemet
Copy link
Author

That's why I wrote above:

If you run jlink manually adding --add-modules dep2, it doesn't work, because jlink dislikes automatic modules.

[...], and it could be nice, if not complicated, to add a 4th option to runtimePath in order to only add named modules from the maven dependencies to the module path.

As for patching the jar of dependencies to add module-info, that would be nice, but I guess you can leave that to moditect.

Also I'm realizing than ALL-MODULE-PATH doesn't work at runtime when running the image. So we need to be able to clearly disinguish options for jlink (--add-modules ALL-MODULE-PATH here) and options for the launcher (--add-modules ALL-SYSTEM here).

@abhinayagarwal
Copy link
Collaborator

The reason why <option>--add-modules ALL-MODULE-PATH</option> doesn't work with javafx:jlink is because all javafx:run options are not compatible with javafx:jlink. This was reported in #22 .

So, now any <option> declared in the plugin configuration is moved the launcher script under JLINK_VM_OPTIONS. This was done as a part of #23.

I have introduced a new parameter jlinkOptions which allows a user to define options for the jlink executable directly. options can still be used to pass arguments to the launcher.

@HGuillemet
Copy link
Author

Tested and works well.
Thank you for your reactivity.

BTW, this plugin becomes less and less specific to JavaFX. Do you have plan to reorganize things ?

@HGuillemet
Copy link
Author

One additional request : the maven <scope> should be taken into account when copying the maven dependencies to classpath or modulepath. Dependencies with compile scope for instance shouldn't be added to the path when running the application or building the jlink image.

@abhinayagarwal
Copy link
Collaborator

Hi @HGuillemet ,

Do you need a scope which can be used at compile time but not to be used during runtime?

@HGuillemet
Copy link
Author

Sorry for this late answer. Forget my last message: the compile scope should be included in the runtime path, and it is. I forgot it was the default scope and it means runtime too.

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

No branches or pull requests

3 participants