RFE simplify usage of patched module [by Robert Scholte, from jdk-dev]

classic Classic list List threaded Threaded
29 messages Options
12
Reply | Threaded
Open this post in threaded view
|

RFE simplify usage of patched module [by Robert Scholte, from jdk-dev]

Christian Stein
Hi,

I took the freedom moving this thread to "jigsaw-dev", as
suggested by David Holmes, in order to reply to it in a
follow-up mail. I hope, Robert doesn't mind that I copy his
text from "jdk-dev" below to start with the same context.

Robert Scholte wrote in
http://mail.openjdk.java.net/pipermail/jdk-dev/2020-February/003880.html

background:
while developing the maven-compiler-plugin I've often resisted for adding
module descriptors to the test-classes.
Test classes won't result in a deliverable.
I've been able to construct the right set of arguments for the compiler
based on the pom.xml and the module descriptor and I hadn't seen an example
yet that should change my opinion.

However, recently I received a small project that required java.sql, but
only during test. Because they didn't really understand why the
test-compilation failed, this module was added to the module descriptor
with a comment ( /* only for test */ )
My advice was replacing it with the following to the maven-compiler-plugin:
<execution>
  <id>default-testCompile</id>
  <configuration>
    <compilerArgs>
      <compilerArg>--add-modules=java.sql</compilerArg>
    </compilerArgs>
  </configuration>
</execution>


But something similar needs to happen for surefire, to run the tests.

This is sadly one example where the user is forced to understand these
additional flags. I could try to solve this with a custom (Maven) solution
to avoid the need for flags configuration, but that would make it tool
specific, whereas I believe this should be solved by the JDK.

What might have helped would be something like this:
src/test/java/module-info.java
[....] {
  requires java.sql;
}

I've put [...] on purpose, because in this case I don't think the module
should have the same name as the "main" module descriptor, because it is
that module. An anonymous module is already better, but that is not allowed
in the descriptor.

One solution that might fit here is the introduction of a new modifier:
patch
patch module some.module {

   requires java.sql;
}
This describes clear the purpose of the module, and as the "patch" already
implies: it should be handled with care.
Allowing such module descriptor should help adopting the modular system
faster. It doesn't require the knowledge of the module related flags, the
buildtool can solve it. As the developer is already familiar with the main
module descriptor, adding a patched module descriptor when required is just
a simple step.

Both java and javac could benefit from this change, and you might consider
to not allow to package with a patched module descriptor.

regards,
Robert Scholte
Reply | Threaded
Open this post in threaded view
|

Re: RFE simplify usage of patched module [by Robert Scholte, from jdk-dev]

Christian Stein
// Hope this email will be technically associated as a reply for the
// "RFE simplify usage of patched module" thread.

I agree with Robert that a user-friendly and (build) tool-agnostic way
to express additional module directives and modifiers for testing
purposes only is ... a good thing to have.

As a workaround, I proposed, implemented and promoted a tool-agnostic
`module-info.test` file in [0] which, as of today, I no longer really like
that
much. The standard javac/java command options and arguments listed
as plain text lines in that file require users to know the module-related
options of javac/java meant to be primarily used by (build) tools.

Perhaps this discussion leads to a solution that supersedes the purpose
of "module-info.test" and renders
https://youtrack.jetbrains.com/issue/IDEA-222831
as no longer needed.

Robert Scholte wrote in

> http://mail.openjdk.java.net/pipermail/jdk-dev/2020-February/003880.html
>
> [...]
> What might have helped would be something like this:
> src/test/java/module-info.java
> [....] {
>   requires java.sql;
> }
>
> I've put [...] on purpose, because in this case I don't think the module
>
should have the same name as the "main" module descriptor, because it
>
is that module. [...]
>


This "solution" already works today. As described in [0] you may use the
existing module-info.java-DSL to describe such an additions (of extra
modules
needed for testing) by (I) repeating (most) directives from the "main"
module
descriptor and (II) add those needed for testing. This includes adding more
module modifiers, like `open` as well.

Using the same module name as `src/main/java/module-info.java`, of course.


> One solution that might fit here is the introduction of a new modifier:
> patch
> patch module some.module {
>
>    requires java.sql;
> }
> This describes clear the purpose of the module, and as the "patch" already
>
implies: it should be handled with care. [...]
>

Such a modifier would remedy the need to repeat directives from the "main"
module. It would effectively render step (I) from above not longer required.

Quote from [0]:
> Note: Copying parts from the main module descriptor manually is brittle.
> The “Java 9 compatible build tool” pro[1] solves this by auto-merging a
> main and test module descriptor on-the-fly.

Robert, you chose `some.module` as the name in this example? Is it
the same name as the "main" module?

Cheers,
Christian

[0]:
https://sormuras.github.io/blog/2018-09-11-testing-in-the-modular-world#white-box-modular-testing-with-module-infojava

[1]: https://github.com/forax/pro
Reply | Threaded
Open this post in threaded view
|

Re: RFE simplify usage of patched module [by Robert Scholte, from jdk-dev]

Christian Stein
On Wed, Feb 5, 2020 at 4:47 AM Christian Stein <[hidden email]> wrote:

> // Hope this email will be technically associated as a reply for the
> // "RFE simplify usage of patched module" thread.
>
> I agree with Robert that a user-friendly and (build) tool-agnostic way
> to express additional module directives and modifiers for testing
> purposes only is ... a good thing to have.
>
> As a workaround, I proposed, implemented and promoted a tool-agnostic
> `module-info.test` file in [/] which, as of today, I no longer really like
> that
> much. The standard javac/java command options and arguments listed
> as plain text lines in that file require users to know the module-related
> options of javac/java meant to be primarily used by (build) tools.
>
>
Here's a Maven-based project that demonstrates most (all?) interesting
main/test scope mixing combinations: https://github.com/micromata/sawdust
Instead of using the Surefire plugin to launch tests, it uses [0]. The
latter
supports the `module-info.test` configuration file, which is used in one of
the examples: [1]

Relevant in this context are the two examples whose names starts with
"modular-whitebox-".

With the "patch modifier"-proposal applied as suggested by Robert,
this manually constructed test module descriptor:

open module foo {
  exports foo;
  requires org.junit.jupiter.api;
}

from [2] would shrink to:

open patch module foo {
  requires org.junit.jupiter.api;
}

Only the additional "open" modifier and the additional requires directive
remain.
The other (here the single exports directive) directives are copied from the
non-patched module "foo".

Cheers,
Christian

[0]: https://github.com/sormuras/junit-platform-maven-plugin
[1]:
https://github.com/micromata/sawdust/blob/master/modular-whitebox-patch-runtime/src/test/resources/module-info.test
[2]:
https://github.com/micromata/sawdust/blob/master/modular-whitebox-patch-compile/src/test/java/module-info.java
Reply | Threaded
Open this post in threaded view
|

Re: RFE simplify usage of patched module [by Robert Scholte, from jdk-dev]

Simone Bordet
In reply to this post by Christian Stein
Hi,

> With the "patch modifier"-proposal applied as suggested by Robert,
> this manually constructed test module descriptor:
>
> open module foo {
>   exports foo;
>   requires org.junit.jupiter.api;
> }
>
> from [2] would shrink to:
>
> open patch module foo {
>   requires org.junit.jupiter.api;
> }

While I was involved in the original report, I have concerns about its security.

Would not anyone be able to patch an existing module without the
author's consent?
For example:

patch module org.eclipse.jetty.client {
  exports org.eclipse.jetty.client.internal;
  opens org.eclipse.jetty.client;
}

Doing the same on the command line keeps the end user in control,
rather than having the end user possibly scan hundreds of jar to see
if someone snuck in a patched module descriptor.

However, the need for such "test" module descriptor is evident.

What if patched module descriptors are only effective when a command
line option is present, say "--allow-patch-descriptors", or something
like that?

Thanks!

--
Simone Bordet
---
Finally, no matter how good the architecture and design are,
to deliver bug-free software with optimal performance and reliability,
the implementation technique must be flawless.   Victoria Livschitz
Reply | Threaded
Open this post in threaded view
|

Re: RFE simplify usage of patched module [by Robert Scholte, from jdk-dev]

Robert Scholte
Hi Simone,

I understand your concern, but the patched module descriptor doesn't have to (or should not) replace the --patch-module option. This proposal is about the additional options you now need to put on the commandline, but which already fit in the module descriptor.

thanks,
Robert
On 5-2-2020 08:19:21, Simone Bordet <[hidden email]> wrote:
Hi,

> With the "patch modifier"-proposal applied as suggested by Robert,
> this manually constructed test module descriptor:
>
> open module foo {
> exports foo;
> requires org.junit.jupiter.api;
> }
>
> from [2] would shrink to:
>
> open patch module foo {
> requires org.junit.jupiter.api;
> }

While I was involved in the original report, I have concerns about its security.

Would not anyone be able to patch an existing module without the
author's consent?
For example:

patch module org.eclipse.jetty.client {
exports org.eclipse.jetty.client.internal;
opens org.eclipse.jetty.client;
}

Doing the same on the command line keeps the end user in control,
rather than having the end user possibly scan hundreds of jar to see
if someone snuck in a patched module descriptor.

However, the need for such "test" module descriptor is evident.

What if patched module descriptors are only effective when a command
line option is present, say "--allow-patch-descriptors", or something
like that?

Thanks!

--
Simone Bordet
---
Finally, no matter how good the architecture and design are,
to deliver bug-free software with optimal performance and reliability,
the implementation technique must be flawless. Victoria Livschitz
Reply | Threaded
Open this post in threaded view
|

Re: RFE simplify usage of patched module [by Robert Scholte, from jdk-dev]

Simone Bordet
Hi Robert,

On Wed, Feb 5, 2020 at 8:38 AM Robert Scholte <[hidden email]> wrote:
>
> Hi Simone,
>
> I understand your concern, but the patched module descriptor doesn't have to (or should not) replace the --patch-module option. This proposal is about the additional options you now need to put on the commandline, but which already fit in the module descriptor.

I understand it does not replace --patch-module.
I understand it adds the additional "requires", "opens", etc.

But how do you stop a library that uses Jetty to ship a jar containing
a patched module file that exports and opens things in Jetty that the
Jetty authors did not want to export or open, without users knowing
it?

jetty-client.jar -> contains legit module-info.class
library.jar -> contains patched descriptor that patches jetty-client
app.jar -> my application with a legit module-info.class

java --module-path jetty-client.jar:library.jar:app.jar --module
app/com.app.Main

With this command line, does the Java runtime parse and enable the
patched descriptor contained in library.jar, opening up jetty-client?
If not, how would you enable it in Maven?

Am I missing something?

Thanks!

--
Simone Bordet
---
Finally, no matter how good the architecture and design are,
to deliver bug-free software with optimal performance and reliability,
the implementation technique must be flawless.   Victoria Livschitz
Reply | Threaded
Open this post in threaded view
|

Re: RFE simplify usage of patched module [by Robert Scholte, from jdk-dev]

Robert Scholte
Hi Simone,

to me the commandline doesn't change, so in your case the library.jar won't patch jetty-client.jar.

It will only be patch as you already would do right now:
java --module-path jetty-client.jar:library.jar:app.jar --module app/com.app.Main --patch-module jetty.client=/path/to/library.jar


For Maven plugins I don't expect a lot of changes. I should probably use the name in the patched module descriptor instead of automatically choosing the main module descriptor, but these are little adjustments.

Having a patched module descriptor in a jar might be awkward, hence maybe the packager shouldn't allow to add it, nor other tools to use it.
But these are details open for discussion.

Robert
On 5-2-2020 08:57:53, Simone Bordet <[hidden email]> wrote:
Hi Robert,

On Wed, Feb 5, 2020 at 8:38 AM Robert Scholte wrote:
>
> Hi Simone,
>
> I understand your concern, but the patched module descriptor doesn't have to (or should not) replace the --patch-module option. This proposal is about the additional options you now need to put on the commandline, but which already fit in the module descriptor.

I understand it does not replace --patch-module.
I understand it adds the additional "requires", "opens", etc.

But how do you stop a library that uses Jetty to ship a jar containing
a patched module file that exports and opens things in Jetty that the
Jetty authors did not want to export or open, without users knowing
it?

jetty-client.jar -> contains legit module-info.class
library.jar -> contains patched descriptor that patches jetty-client
app.jar -> my application with a legit module-info.class

java --module-path jetty-client.jar:library.jar:app.jar --module
app/com.app.Main

With this command line, does the Java runtime parse and enable the
patched descriptor contained in library.jar, opening up jetty-client?
If not, how would you enable it in Maven?

Am I missing something?

Thanks!

--
Simone Bordet
---
Finally, no matter how good the architecture and design are,
to deliver bug-free software with optimal performance and reliability,
the implementation technique must be flawless. Victoria Livschitz
Reply | Threaded
Open this post in threaded view
|

Re: RFE simplify usage of patched module [by Robert Scholte, from jdk-dev]

Christian Stein
I agree with you, that the user (the build tool in most of the cases) should
stay in control which module descriptor is in effect for every module.

As my domain is testing, and I guess the majority of uses-cases is within
the realm of intra-module (white-box) testing, I'll expound on the topic
with
a complete example.


# GIVEN

- src/foo/main/java/module-info.java

module foo {
  // here be dragons
}

- src/foo/test/java/module-info.java

open $PATCH module foo {
  // here be same dragons as "main" foo declares
  requires org.junit.jupiter.api;
  requires org.assertj.core;
  requires java.sql;
}

Note, without the new $PATCH feature, you may just copy and paste
the "main" foo directives to the "test" foo module already today.


# WHEN

Compile the "test" module against the already compiled and optionally
packaged "main" module via:

javac --module-source-path src/*/test/java --patch-module
foo=out/classes/main/foo --module foo
jar --create --file out/modules/test/foo.jar -C  out/classes/test/foo .


# THEN

Execute JUnit Platform Console Launcher selecting the "test" foo module
and scanning it for tests. Here, all 3rd-party modules are already put into
a project-local "lib/" directory:

java
  --module-path out/modules/test/foo.jar:out/modules/main:lib
  --patch-module foo=out/modules/main/foo.jar
  --module org.junit.platform.console
  --scan-module foo


You see, there's still a command line switch required to get the patching
going. This command will (a) load the patched foo module, that one that
reads all modules required for testing and (b) resolve the classes under
test from the main module.

Yes, if someone (or the build tool) jars the test and main classes into
a single JAR file, you may create a "patched by default" module which
is no in control of the original author -- but, no change to what you
may already do today.

Cheers,
Christian


On Wed, Feb 5, 2020 at 5:44 PM Robert Scholte <[hidden email]> wrote:

> Hi Simone,
>
> to me the commandline doesn't change, so in your case the library.jar
> won't patch jetty-client.jar.
>
> It will only be patch as you already would do right now:
> java --module-path jetty-client.jar:library.jar:app.jar
> --module app/com.app.Main --patch-module jetty.client=/path/to/library.jar
>
>
> For Maven plugins I don't expect a lot of changes. I should probably use
> the name in the patched module descriptor instead of automatically choosing
> the main module descriptor, but these are little adjustments.
>
> Having a patched module descriptor in a jar might be awkward, hence maybe
> the packager shouldn't allow to add it, nor other tools to use it.
> But these are details open for discussion.
>
> Robert
> On 5-2-2020 08:57:53, Simone Bordet <[hidden email]> wrote:
> Hi Robert,
>
> On Wed, Feb 5, 2020 at 8:38 AM Robert Scholte wrote:
> >
> > Hi Simone,
> >
> > I understand your concern, but the patched module descriptor doesn't
> have to (or should not) replace the --patch-module option. This proposal is
> about the additional options you now need to put on the commandline, but
> which already fit in the module descriptor.
>
> I understand it does not replace --patch-module.
> I understand it adds the additional "requires", "opens", etc.
>
> But how do you stop a library that uses Jetty to ship a jar containing
> a patched module file that exports and opens things in Jetty that the
> Jetty authors did not want to export or open, without users knowing
> it?
>
> jetty-client.jar -> contains legit module-info.class
> library.jar -> contains patched descriptor that patches jetty-client
> app.jar -> my application with a legit module-info.class
>
> java --module-path jetty-client.jar:library.jar:app.jar --module
> app/com.app.Main
>
> With this command line, does the Java runtime parse and enable the
> patched descriptor contained in library.jar, opening up jetty-client?
> If not, how would you enable it in Maven?
>
> Am I missing something?
>
> Thanks!
>
> --
> Simone Bordet
> ---
> Finally, no matter how good the architecture and design are,
> to deliver bug-free software with optimal performance and reliability,
> the implementation technique must be flawless. Victoria Livschitz
>
Reply | Threaded
Open this post in threaded view
|

Re: RFE simplify usage of patched module [by Robert Scholte, from jdk-dev]

Christian Stein
On Thu, Feb 6, 2020 at 8:35 AM Christian Stein <[hidden email]> wrote:

> I agree with you, that the user (the build tool in most of the cases)
> should
> stay in control which module descriptor is in effect for every module.
>
> As my domain is testing, and I guess the majority of uses-cases is within
> the realm of intra-module (white-box) testing, I'll expound on the topic
> with
> a complete example.
>
>
> # GIVEN
>
> - src/foo/main/java/module-info.java
>

Reading this post a day later, I assume there more questions raised than
answers given by this "foo" example. If that's the case, I apologize for the
confusion -- my tunnel vision of "living on the testing side of the modular
world" led to jumps in the story I wanted to tell.

This time, I created a project at [0] with a detailed description on its
front
page, i.e the README.md file. I read that new story thrice and hope to have
a more complete example prepared. It is based on the "Project Jigsaw:
Module System Quick-Start Guide" [1] and extends the example modules
with testing. Java-based build programs accompany the project to make
it easy to "replay" (read: build) it on every supported platform.

If any new questions appear, I'm happy to answer those on this ML.

If you want me to copy "the new story" as plain text to this ML as well,
just ping me. I refrained copying it directly, as it spans some pages
and contains ASCII-art images, quotes, log excerpts...

Cheers,
Christian

[0]: https://github.com/sormuras/java-module-patching
[1]: https://openjdk.java.net/projects/jigsaw/quick-start
Reply | Threaded
Open this post in threaded view
|

Re: RFE simplify usage of patched module [by Robert Scholte, from jdk-dev]

Alex Buckley
Hi Christian,

On 2/7/2020 4:41 AM, Christian Stein wrote:
> This time, I created a project at [0] with a detailed description on its
> front page, i.e the README.md file.
>
> [0]: https://github.com/sormuras/java-module-patching

To restate:

- You're saying that, today, it's brittle to copy directives from
src/org.astro/main/java/module-info.java to
src/org.astro/test/java/module-info.java. (And having copied them, you
still need to `open` the test module and add some `requires`.)

- You suggest that, in future, there will still be a
src/org.astro/test/java/module-info.java file which is of interest to
test frameworks.

What you're hoping is that new syntax will let you invert the patching:

- Today, you set the module-path so that out/modules/test/org.astro.jar
is the "primary" version of the module; then you redefine everything in
it except module-info by overlaying out/modules/main/org.astro.jar.

- In future, you want to have out/modules/main/org.astro.jar as the
"primary" version, and redefine only its module-info by specifying the
sidecar out/modules/test/org.astro.jar. The sidecar would have some
syntax to identify that its declaration of org.astro is strictly
additive to the "primary" version. You would set the module-path to
out/modules/main:out/modules/test:lib so that the module system (1)
finds the "primary" version in out/modules/main and (2) augments its
module-info with sidecars found in out/modules/test and lib. I assume
some new command line option would enable or enumerate the sidecars
explicitly, because obviously the dependences and exports of
out/modules/main/org.astro.jar shouldn't depend on which JAR files
happen to be lurking deep in the module-path.

Stepping back, the core issue is that once the true "primary" version of
a module is built -- out/modules/main/org.astro.jar -- you don't want to
change it. No build or test tool wants to physically rewrite its
module-info to require test-time dependencies at test time, and then to
not require such dependencies at release time. You want the module
system to virtually rewrite the module-info instead. And that's already
possible, as long as you put on your test-colored sunglasses and view
out/modules/test/org.astro.jar as the "primary" version, and
out/modules/main/org.astro.jar as the overlay ... once the tests have
run, go back to viewing out/modules/main/org.astro.jar as the "primary"
version. Introducing a new kind of module descriptor, with merging by
the module system according to new command line options, seems like a
lot of overhead that can already be worked around by tools at test-time.

Alex
Reply | Threaded
Open this post in threaded view
|

Re: RFE simplify usage of patched module [by Robert Scholte, from jdk-dev]

Robert Scholte
It is kind of unfortunate that this topic has been hijacked for something I don't see as an issue.
I've been able to build up the right set of commandline arguments for this case for both javac and java

With *just* the dependencies(respecting compile + test scope) from the pom, and the module descriptor I've been able to compile the main/test sources and run the tests.

However, recently I've informed by this case: if the test sources use one of the java.* modules (that are not used by the main sources) the only correct way to solve it now is by adding the required flags by hand (and only to the test-compile configuration!).
This is hard to explain and instead of diving into the specifications, understanding what's happening, you'll see that they choose for the easy workaround: add this "test scoped" module as a required module to the module descriptor. 
In the end this will do more harm than good. A bit off-topic, but with Maven we already see that adding dependencies is simple, however cleaning up unused dependencies is not. Hence the exclude-option for dependencies is used frequently. Unlike Maven dependencies, you cannot exclude required modules, so here it is even more important that the list of required modules is the minimum required list.

To prevent these workarounds and to provide an easier way to patch a module via a dedicated descriptor will help keeping the module system cleaner.
I'm very pleased I've been able to hide most logic behind the required flags. 
This is a valid use case and in my opinion deserves a solution without explicit knowledge of these flags by the enduser (a tool like Maven should solve this, but this can only be done if it has access to this information.)
Introducing an extra modifier looks like one solution, maybe there is a better one.

thanks,
Robert

On 10-2-2020 23:03:28, Alex Buckley <[hidden email]> wrote:
Hi Christian,

On 2/7/2020 4:41 AM, Christian Stein wrote:
> This time, I created a project at [0] with a detailed description on its
> front page, i.e the README.md file.
>
> [0]: https://github.com/sormuras/java-module-patching

To restate:

- You're saying that, today, it's brittle to copy directives from
src/org.astro/main/java/module-info.java to
src/org.astro/test/java/module-info.java. (And having copied them, you
still need to `open` the test module and add some `requires`.)

- You suggest that, in future, there will still be a
src/org.astro/test/java/module-info.java file which is of interest to
test frameworks.

What you're hoping is that new syntax will let you invert the patching:

- Today, you set the module-path so that out/modules/test/org.astro.jar
is the "primary" version of the module; then you redefine everything in
it except module-info by overlaying out/modules/main/org.astro.jar.

- In future, you want to have out/modules/main/org.astro.jar as the
"primary" version, and redefine only its module-info by specifying the
sidecar out/modules/test/org.astro.jar. The sidecar would have some
syntax to identify that its declaration of org.astro is strictly
additive to the "primary" version. You would set the module-path to
out/modules/main:out/modules/test:lib so that the module system (1)
finds the "primary" version in out/modules/main and (2) augments its
module-info with sidecars found in out/modules/test and lib. I assume
some new command line option would enable or enumerate the sidecars
explicitly, because obviously the dependences and exports of
out/modules/main/org.astro.jar shouldn't depend on which JAR files
happen to be lurking deep in the module-path.

Stepping back, the core issue is that once the true "primary" version of
a module is built -- out/modules/main/org.astro.jar -- you don't want to
change it. No build or test tool wants to physically rewrite its
module-info to require test-time dependencies at test time, and then to
not require such dependencies at release time. You want the module
system to virtually rewrite the module-info instead. And that's already
possible, as long as you put on your test-colored sunglasses and view
out/modules/test/org.astro.jar as the "primary" version, and
out/modules/main/org.astro.jar as the overlay ... once the tests have
run, go back to viewing out/modules/main/org.astro.jar as the "primary"
version. Introducing a new kind of module descriptor, with merging by
the module system according to new command line options, seems like a
lot of overhead that can already be worked around by tools at test-time.

Alex
Reply | Threaded
Open this post in threaded view
|

Re: RFE simplify usage of patched module [by Robert Scholte, from jdk-dev]

Alex Buckley
On 2/12/2020 1:08 PM, Robert Scholte wrote:
> To prevent these workarounds and to provide an easier way to patch a
> module via a dedicated descriptor will help keeping the module
> system cleaner.

It will lead to "split modules" on the modulepath, which will cause just
as many maintenance headaches as split packages on the classpath. Yes,
there is some maintenance benefit if a module explicitly declares that
it patches (i.e. becomes part of) another module (which might then have
to explicitly declare that it allows patching) ... but for a developer
to understand the resulting module graph requires looking at everything
on the modulepath, which is no better than having to look at everything
on the classpath. In Java, a declaration -- whether a module, a class,
or a method -- happens in one source file, and it's up to tools to
rewrite declarations if other interesting source files are known to
those tools.

> However, recently I've informed by this case: if the test sources
> use one of the java.* modules (that are not used by the main sources)
> the only correct way to solve it now is by adding the required flags
> by hand (and only to the test-compile configuration!). This is hard
> to explain and instead of diving into the specifications,
> understanding what's happening, you'll see that they choose for the
> easy workaround: add this "test scoped" module as a required module
> to the module descriptor.

Is there nothing that Maven can do to make the test-compile
configuration easier to create? Maven has all the source code at its
fingertips, including knowledge of module directories which seem to
declare the same module more than once because JUnit recommends it, yet
still Maven makes the user laboriously write out the command line flags
for patching?

Alex
Reply | Threaded
Open this post in threaded view
|

Re: RFE simplify usage of patched module [by Robert Scholte, from jdk-dev]

Robert Scholte
I am aware that the current patch concept is already complicating things.
Apart from the impact of the requested implementation, the information+options should all be available already: the patch module descriptor should be considered as an alias for some related commandline options.


With a tool like Maven it is already possible to compile and run patched modules, but in case of requiring java.* modules it is very awkward that it requires additional configuration in the pom instead of some dedicated module descriptor, which feels most natural.
You'll get an exception like: package java.sql is declared in module java.sql, but module some.module.name does not read it
And the first thing that comes to mind is: add java.sql to the (main) module descriptor. But this should not be done.

Other options to avoid this would be: 
- source code scanning to figure out all used modules. To me not an option: way too expensive/resource consuming.
- by default add all java.* modules, since they should be available anyway.
All other solutions will be a tool specific solution, even though they're struggling all with the same problem.

To me having the correct (main) module descriptor should be the ultimate goal. People should not be tempted to apply workarounds on this descriptor to make things work for tests (even though those module are for sure part of the java runtime)
Especially because of the impact wrong module descriptors would have and which cannot be adjusted.

thanks,
Robert
On 13-2-2020 00:12:02, Alex Buckley <[hidden email]> wrote:
On 2/12/2020 1:08 PM, Robert Scholte wrote:
> To prevent these workarounds and to provide an easier way to patch a
> module via a dedicated descriptor will help keeping the module
> system cleaner.

It will lead to "split modules" on the modulepath, which will cause just
as many maintenance headaches as split packages on the classpath. Yes,
there is some maintenance benefit if a module explicitly declares that
it patches (i.e. becomes part of) another module (which might then have
to explicitly declare that it allows patching) ... but for a developer
to understand the resulting module graph requires looking at everything
on the modulepath, which is no better than having to look at everything
on the classpath. In Java, a declaration -- whether a module, a class,
or a method -- happens in one source file, and it's up to tools to
rewrite declarations if other interesting source files are known to
those tools.

> However, recently I've informed by this case: if the test sources
> use one of the java.* modules (that are not used by the main sources)
> the only correct way to solve it now is by adding the required flags
> by hand (and only to the test-compile configuration!). This is hard
> to explain and instead of diving into the specifications,
> understanding what's happening, you'll see that they choose for the
> easy workaround: add this "test scoped" module as a required module
> to the module descriptor.

Is there nothing that Maven can do to make the test-compile
configuration easier to create? Maven has all the source code at its
fingertips, including knowledge of module directories which seem to
declare the same module more than once because JUnit recommends it, yet
still Maven makes the user laboriously write out the command line flags
for patching?

Alex
Reply | Threaded
Open this post in threaded view
|

Re: RFE simplify usage of patched module [by Robert Scholte, from jdk-dev]

Alex Buckley
On 2/14/2020 12:02 AM, Robert Scholte wrote:
> To me having the correct (main) module descriptor should be the ultimate
> goal. People should not be tempted to apply workarounds on this
> descriptor to make things work for tests (even though those module are
> for sure part of the java runtime)
> Especially because of the impact wrong module descriptors would have and
> which cannot be adjusted.

Nowhere else in Java is there the ability to "patch" one declaration by
making further declarations in source code, so I don't think there's a
way forward for a `patch module ...` declaration. I understand that it
looks good when viewed through the lens of testing, where there's
exactly one secondary module declaration (the test module) and it's
tightly scoped to support white-box tests. However, we have to consider
if it's a good idea in general for a module to be defined by parts,
where anyone could throw multiple `patch module foo {..}` declarations
on the module path and have each of them modify the "master" foo module.

The answer is that it's not a good idea: (i) different `requires`
clauses might clash (i.e. read modules which export the same packages),
(ii) one patch might `open` the module while another patch doesn't want
it open (and there's no way to say non-open), and (iii) the content of
packages in the unified module is now determined by the order in which
the "master" module and the "patch" modules are deployed on the module path.

These things are so undesirable that even `--patch-module` is unable to
override module declarations! That said, command line options can have
superpowers that would never be expressible in source code declarations,
so one option might be a `--patch-module-descriptor` option. This would
be orthogonal to `--patch-module`, which deliberately has leaky
classpath-style semantics for its specified content, and that's neither
desirable nor useful here. Within limits, `--patch module-descriptor`
would merge the declaration of the "patch" module into the "master"
module, and fail hard if the limits are broken. What are the limits? A
starting point is the rules for versioned module descriptors in a
modular multi-release JAR --
http://openjdk.java.net/jeps/238#Modular-multi-release-JAR-files -- and
perhaps it's reasonable to allow additional _qualified_ exports in the
"patch" module.

Now, I am not "proposing" `--patch-module-descriptor` for a future JDK.
I am recognizing that certain use cases involve changing a module's
dependences and encapsulation in a tightly scoped way. A different way
to address those use cases would be to ship the logic just described for
`--patch-module-descriptor` in a launcher offered by the test framework
itself. If JUnit creates its own module layer, then it can define module
descriptors at run time how ever it likes. It's not clear to me if
Christian & co. have looked into java.lang.ModuleLayer,
java.lang.module.Configuration, and java.lang.module.ModuleFinder. If
they have, we'd love to hear their experiences. If they haven't, they
should.

Alex
Reply | Threaded
Open this post in threaded view
|

Re: RFE simplify usage of patched module [by Robert Scholte, from jdk-dev]

Christian Stein
In reply to this post by Alex Buckley
Hi Alex,

thanks for your reply. Please find answers inlined below.

Note: I just realized that you already replied to Robert, and
from what I saw, the `--patch-module-descriptor` option idea would
indeed be a workable solution. Have to re-read your reply two or
three times to fully grasp that suggestion, though. I'll send this
answer anyway -- to round-up and effectively close this sub-thread.

On Mon, Feb 10, 2020 at 11:03 PM Alex Buckley <[hidden email]>
wrote:

> Hi Christian,
>
> On 2/7/2020 4:41 AM, Christian Stein wrote:
> > This time, I created a project at [0] with a detailed description on its
> > front page, i.e the README.md file.
> >
> > [0]: https://github.com/sormuras/java-module-patching
>
> To restate:
>
> - You're saying that, today, it's brittle to copy directives from
> src/org.astro/main/java/module-info.java to
> src/org.astro/test/java/module-info.java. (And having copied them, you
> still need to `open` the test module and add some `requires`.)
>
> - You suggest that, in future, there will still be a
> src/org.astro/test/java/module-info.java file which is of interest to
> test frameworks.
>
>
That is correct. It is and will be relevant to compile and run
test-related modules. That includes inter-module (black box) test
modules and in(tra)-module (white box) test modules. The former
may use the standard `module-info.java` syntax as of today. As they
use an unique name, no patching is required. The latter, re-use
the name of their associated module on purpose. Those in-module
tests are expected to use the same package name -- allowing access
to package private members.


> What you're hoping is that new syntax will let you invert the patching:
>

No, not at all.

My hope is to re-use the same well-known `module-info.java` syntax
to define the additional needs (modifiers and directives) of the
in-module test module. Similar to the standalone intra-module test
module.



> - Today, you set the module-path so that out/modules/test/org.astro.jar
> is the "primary" version of the module; then you redefine everything in
> it except module-info by overlaying out/modules/main/org.astro.jar.
>


Exactly.


> - In future, you want to have out/modules/main/org.astro.jar as the
> "primary" version, and redefine only its module-info by specifying the
> sidecar out/modules/test/org.astro.jar.

[...]


No. I still want out/modules/test/org.astro.jar to be and stay my
only version of the org.astro module that is relevant at test compile
and runtime time.

I want some help when declaring src/org.astro/test/java/module-info.java
It should "extend" the src/org.astro/main/java/module-info.java
description. At compile and/or runtime.

I don't want to manually merge the orginial modifiers already defined
in src/org.astro/main/java/module-info.java



> Stepping back, the core issue is that once the true "primary" version of
> a module is built -- out/modules/main/org.astro.jar -- you don't want to
> change it.

[...]


True. That's why I prefer to have standalone "test" versions of all modules.
All in the sense of inter-module (black box) and in(-tra)-module (white box)
testing.

I'll cut it here and continue on the  `--patch-module-descriptor` thread
later.

Cheers,
Christian
Reply | Threaded
Open this post in threaded view
|

Re: RFE simplify usage of patched module [by Robert Scholte, from jdk-dev]

Christian Stein
In reply to this post by Alex Buckley
Hi Alex,

On Fri, Feb 14, 2020 at 7:06 PM Alex Buckley <[hidden email]>
wrote:

> [...]

Now, I am not "proposing" `--patch-module-descriptor` for a future JDK.
> I am recognizing that certain use cases involve changing a module's
> dependences and encapsulation in a tightly scoped way.


I can clearly envision the superpowers of the not (yet) proprosed
`--patch-module-descriptor` option unleashed upon the
Modular Testing World solving the long-standing issue of
resorting to either the class-path or the command-line when
it comes to in-(tra)-module (white box) testing. [0]

Let's explore the feature set of such an option and its limits together.
Looking forward to help with and test-drive that solution.

Three or two years ago, I contributed the "tester plugin" to pro [1].
Pro is a Java build tool that works seamlessly with modules and was
created be Rémi Forax.
That pro tool already performs such a merge action when for in-(tra)-
module (white box) test modules (can you please specify simple
token for such a type of module?!).
Here is the relevant implementation [2] which may guide us
when exploring the feature set. I guess, Rémi will comment on
this topic, too. As he is, if I recall correctly, "the father of the idea".

A different way
> to address those use cases would be to ship the logic just described for
> `--patch-module-descriptor` in a launcher offered by the test framework
> itself.


That's too late in the build game. You already need this merged
descriptor at compile time. And you need the user to define the
additional elements (modifiers and directives), as no build tool
may infer them always correctly by inspecting the test sources...


> If JUnit creates its own module layer, [...]


No, we don't. We "just" make use of the ModuleReader when
scanning a module for potential test classes. That logic [3] is
shipped in a MR-JAR since two years ago and works flawlessly.


> [...] then it can define module descriptors at run time

how ever it likes [...]


Like said before, I think, that's too late in the build game. And
still we had to require the user to pass the extra information
to our framework ... in non-standard manner. Using the manually
merged in-(tra)-module (white box) test modules works for us
since day one of Java 9.


> . It's not clear to me if
> Christian & co. have looked into java.lang.ModuleLayer,
> java.lang.module.Configuration, and java.lang.module.ModuleFinder.

If they have, we'd love to hear their experiences.


Yes, I did: [4]

Works like a charm a test runtime when you want to open a module
to the modules of the build tool. Or here, as you might have guessed,
read the extra information required for testing provided by the user
in a `module-info.test` [5] formatted file. The major drawback is the
simple text format that this `module-info.test` uses: brittle as best.

Summarizing, I like the _not_ proposed `--patch-module-descriptor` option.

Cheers,
Christian

[0]:
https://sormuras.github.io/blog/2018-09-11-testing-in-the-modular-world.html#white-box-modular-testing-with-module-infojava
[1]: https://github.com/forax/pro/tree/master/plugins/tester
[2]:
https://github.com/forax/pro/blob/2ddd9425cb95617b6dfd6c7d077ed387a5f6809c/src/main/java/com.github.forax.pro.helper/com/github/forax/pro/helper/ModuleHelper.java#L338-L378
[3]:
https://github.com/junit-team/junit5/blob/master/junit-platform-commons/src/main/java9/org/junit/platform/commons/util/ModuleUtils.java
[4]:
https://github.com/sormuras/junit-platform-isolator/tree/master/junit-platform-isolator-java-11/src/main/java/de/sormuras/junit/platform/isolator
[5]:
https://sormuras.github.io/blog/2018-09-11-testing-in-the-modular-world.html#white-box-modular-testing-with-extra-java-command-line-options
Reply | Threaded
Open this post in threaded view
|

Re: RFE simplify usage of patched module [by Robert Scholte, from jdk-dev]

Alex Buckley
On 2/14/2020 11:29 AM, Christian Stein wrote:
>> A different way to address those use cases would be to ship the
>> logic just described for `--patch-module-descriptor` in a launcher
>> offered by the test framework itself.
>  
> That's too late in the build game. You already need this merged
> descriptor at compile time. And you need the user to define the
> additional elements (modifiers and directives), as no build tool
> may infer them always correctly by inspecting the test sources...

Fair point, and thanks for reminding me about your "Testing In The
Modular World" document.

The thing I'm having trouble with is that javac already lets you specify
the --add-reads options on org.junit.jupiter.api & friends that are
needed to compile test code. If Maven expects the user to painfully
configure the POM to pass --patch-module-descriptor to javac, why can't
the POM offer an easy way to make Maven itself (in the `test-compile`
phase?) pass some defined-behind-the-scenes --add-reads options?

Similarly, at run time, you propose to always open the main module to
the same org.junit.platform.commons module -- `module-info.test` is more
or less the same for every JUnit project -- so why doesn't the POM have
an easy way to let Maven itself (in the `test` phase?) pass --add-opens
to java?

There are no new modularity "primitives" here (by which I mean features
of the module system itself, such as open modules) ... there is just
detailed configuration which, in the testing context, should be done by
the build tool rather than the user.

Alex
Reply | Threaded
Open this post in threaded view
|

Re: RFE simplify usage of patched module [by Robert Scholte, from jdk-dev]

Christian Stein
On Fri, Feb 14, 2020 at 10:31 PM Alex Buckley <[hidden email]>
wrote:

> On 2/14/2020 11:29 AM, Christian Stein wrote:
> >> A different way to address those use cases would be to ship the
> >> logic just described for `--patch-module-descriptor` in a launcher
> >> offered by the test framework itself.
> >
> > That's too late in the build game. You already need this merged
> > descriptor at compile time. And you need the user to define the
> > additional elements (modifiers and directives), as no build tool
> > may infer them always correctly by inspecting the test sources...
>
> Fair point, and thanks for reminding me about your "Testing In The
> Modular World" document.
>
> The thing I'm having trouble with is that javac already lets you specify
> the --add-reads options on org.junit.jupiter.api & friends that are
> needed to compile test code. If Maven expects the user to painfully
> configure the POM to pass --patch-module-descriptor to javac, why can't
> the POM offer an easy way to make Maven itself (in the `test-compile`
> phase?) pass some defined-behind-the-scenes --add-reads options?
>
>
Maven and other build tools would add the `--patch-module-descriptor`
automacially and point to the user-defined "patch module compilation unit"
that only contains the extra directives: like `requires java.sql;`, or
`requires org.testng.core;` or ... `requires org.junit.jupiter;`. Or all of
them.

Those extra directives needed by test modules to compile and run
are a deliberate decision made by the user -- on a per test module basis.
No build tool may infer those decisions correctly.

Similarly, at run time, you propose to always open the main module to
> the same org.junit.platform.commons module -- `module-info.test` is more
>

Not the main module. Only the current test module. Assuming, that you
run tests from a single test module at a time.


> or less the same for every JUnit project -- so why doesn't the POM have
> an easy way to let Maven itself (in the `test` phase?) pass --add-opens
> to java?
>

Opening a module for deep-reflection is just a single aspect of the
"Testing In The Modular World" story. Btw. `open module` or `--add-opens`
is only required iff the test framework of choice reflects into your test
modules. I already have a ServiceProvider-based variation running locally,
that uses this standard technique to pass the execution control to the
main-like entry-point of each test module.


> There are no new modularity "primitives" here (by which I mean features
> of the module system itself, such as open modules) ... there is just
> detailed configuration which, in the testing context, should be done by
> the build tool rather than the user.
>

Except for configuration parts that cannot be inferred. Here the user
has to resort to javac and java command line options, like `--add-reads`,
and must negotiate with the build tool how to pass those options.

And all of this tedious and errorprone textual configuration **after**
having declared a concise, tool-agnostic, well-definend and beautiful
inter-module test module using the official `module-info.java` syntax:

- test.base/test/module-info.java:

open /*test*/ module test.base {
  requires com.greetings; // module under test
  requires org.astro; // module under test
  requires java.sql; // system-provided module needed by test code
  requires org.junit.jupiter; // framework reflecting entry-points
  requires org.assertj.core; // ...and more test-related modules
}

Leaving this compile-able DSL behind **only** for in(tra)-module
(white-box) testing is ... inconvenient and hard to explain. Like
Robert wrote in his initial request to simplify the usage of patch
module.

Assuming `--patch-module-descriptor` was already available, the
org.astro test story would read:

- org.astro/main/module-info.java:

module org.astro {
  exports org.astro;
  requires java.logging;
}

- org.astro/test/module-info.java:

open /*test*/ module org.astro /*extends main module org.astro*/ {
  requires java.sql; // system-provided module needed by test code
  requires org.junit.jupiter; // framework reflecting entry-points
  requires org.assertj.core; // ...and more test-related modules
}

A build tool now easily may detect that `--patch-module-descriptor`
is required at test compile and run time. It could infer it from the
same module name or simply by seeing a main and a test
`module-info.java` compilation unit within the sources of the
org.astro Maven-/Gradle-/pro-/Bach-/any-build-tool-"module".

Build tools could pass the following argument to javac/java
while being in their test-scope tasks. Of course, implying that
org.astro/test/module-info.java is the current "primary" module
descriptor:

--patch-module-descriptor org.astro=org.astro/main/module-info.java

That's it.

The primary test-relevant modular directives are enriched by
those of the main module as declared in org.astro/main/module-info.java
resulting effectively in a synthetic in-module test module descriptor that
resembles the inter-module test descriptor presented above:

- SYNTHETIC test module for org.astro

open /*test*/ module org.astro /*extends main module org.astro*/ {
  requires java.sql; // system-provided module needed by test code
  requires org.junit.jupiter; // framework reflecting entry-points
  requires org.assertj.core; // ...and more test-related modules
  //
  exports org.astro; // patched from org.astro/main/module-info.java
  requires java.logging; // patched from org.astro/main/module-info.java
}

Cheers,
Christian
Reply | Threaded
Open this post in threaded view
|

Re: RFE simplify usage of patched module [by Robert Scholte, from jdk-dev]

Alan Bateman
In reply to this post by Robert Scholte
On 14/02/2020 08:02, Robert Scholte wrote:
> :
>
> With a tool like Maven it is already possible to compile and run patched modules, but in case of requiring java.* modules it is very awkward that it requires additional configuration in the pom instead of some dedicated module descriptor, which feels most natural.
> You'll get an exception like: package java.sql is declared in module java.sql, but module some.module.name does not read it
> And the first thing that comes to mind is: add java.sql to the (main) module descriptor. But this should not be done.
>
Can you summarize how this works when compiling tests that reference
types in modules on the application module path? Is the Maven compiler
plugin uses `--add-modules ALL-MODULE-PATH` to include they are all
resolved? If so, I'm just wondering about readability, do you also
generate a list of --add-reads options too? I guess I'm just wondering
why the issue is specific to modules that in the run-time image that
aren't transitively required by the (unpatched) module.

-Alan
Reply | Threaded
Open this post in threaded view
|

Re: RFE simplify usage of patched module [by Robert Scholte, from jdk-dev]

Christian Stein
In reply to this post by Christian Stein
I extended the example project with some potential use-cases
for the `--patch-module-descriptor` option at [0]. Those use-cases
show which command line option usages can be replaced and
therefore simplified (as this RFE thread was coined) in the form
of the following example:

Today:

  --add-modules java.sql
  --add-reads org.astro=java.sql

With  `--patch-module-descriptor src/org.astro/test/module-info.java`:

  module org.astro { // name specifies the "primary" module to be patched
      requires java.sql; // adds and reads the specified module
  }

Cheers,
Christian

[0]: https://github.com/sormuras/java-module-patching#examples

>
12