Allow defined and automatic modules to co-exist #164
Conversation
|
how both |
hgschmie
force-pushed
the
jpms-with-tests
branch
from
748cf02 to
f69e83d
Compare
|
Assuming one contains the unittest classes of the other jar, this will lead to package name collisions. I'm pretty sure this is not going to work this way. If tests are reusable by other projects, it should be a separate project/module. |
|
I can foresee situations where a Maven project has production code, covered by tests. The test sources could also contain some factories or fakers for domain classes from the production code. As long as these are in a separate package, those wouldn't lead to package name collisions - but it would require the test-jar to only contain that package, not the actual unit tests. Exporting them generally doesn't make sense, I agree. |
|
In our project (https://github.com/jdbi/jdbi), I am planning to generate actual JPMS modules (with a module-info) for the main artifacts. This is what others will consume and we want to support JPMS there. Our test code is packaged in ...-tests.jar artifacts, which will not be JPMS modules but just plain old java jars. I will attach a number of traces from the compile run of the "jdbi3-caffeine-cache" maven module, which depends on "jdbi3-core". These are taken from our regular build / my experimental JPMS build. Currently, we generate all jars with What happens now is that the compiler plugin will see both artifacts (jdbi3-core-3.41.1-SNAPSHOT.jar) and (jdbi3-core-3.41.1-SNAPSHOT-tests.jar) and put them on the class path (because we don't use the module system in our current build). I have attached a trace of the build without JPMS (logfile-nojpms), that shows So the jars show up on the test path, get added to the class path. All good. We've done this forever and it works just fine. Now, adding a module-info to the main artifact of jdbi3-core and the jdbi-caffeine-cache maven modules results in this: The test path passed into the maven test compile is still exactly the same as in the non-JPMS build. But the compiler plugin now splits the artifacts into class path (anything that has automatic module naming) and module path (anything that actually has a module-info file OR a There actually is a simple workaround: Changing the module id of the main artifact from Main artifact goes onto the module path. Test artifact goes onto the class path. Compiler is happy. Tests run fine (because they are executed from the class path and any artifact on the class path is allowed to access the module path). While this works for me, I consider this a source of "gotcha" unexpected behavior. Migration to JPMS invites using the "automatic module id" as the actual id in a "module-info" file and suddenly compilation fails. Or complicated rework of a large build is necessary. With the proposed patch applied, the compiler plugin behaves exactly as it does with the workaround above (see logfile-jpms-fix) by moving an "automatically named" module onto the class path and keep an explicitly named (module-info or manifest) on the module path. I don't have a maven compiler plugin integration test yet, happy to whip one up if this proposed change would be considered for inclusion. logfile-nojpms.txt |
|
(not sure why the unit tests fail on windows, I run them locally on macos and they are fine). |
|
I think Windows is failing because jdbi3-core-3.41.1-SNAPSHOT-tests and jdbi3-core-3.41.1-SNAPSHOT are considered different versions of the same module (i.e. jdbi3.core) and that given the way the Java code resolves the automatic module name, it makes sense. |
|
hi @rfscholte , thanks for looking at the PR. Why would the windows build behave differently on VM level than the build on MacOS and Linux. Could you explain the "given the way the java code resolves the automatic module name, it makes sense" comment? It seems to imply that Java on Windows resolves automatic modules names differently than on Linux or MacOS. I am disagreeing with your assessment; there is an implication that the VM can not deal with multiple artifacts with the same module id spread out across module and classpath. This is not true, as the patch applied to the plexus-java component makes the compiler work correctly. The current behavior is observed because of a choice that the plexus-java component made when distributing artifacts across module and class path. Ironically, if the components had different module ids it would spread them out exactly as it would spread them with this patch and the same module id. It would still fail if there were two "real" JPMS modules (not automatically named). Thank you for implying that I don't understand the module system and educating me on the origins of package names. Here is what is happening in our project: We have published automatic module names ( At the very least, the component should fail and not silently drop one dependency from the class path. The resulting visible behavior to users is confusing because unless you know where to look, it is opaque where the errors come from. |
add an automatic module id to the test jars, so test jar and main jar can coexist on the module path.
|
There should be no differences between OSes, but the names of the failing tests are related to the kind of changes you are making. I haven't checked your patch, because there's still a disagreement on how plexus-java should work, |
|
I think "what we consider" is the important phrase here. As you have freely admitted that you haven't even looked at the change and the condescending language that you use, I don't think there is any point for me to argue with you. |
The included jars are simply "jar cf" commands on directories that contain the required files. Happy to add those; there are a number of other jars included with the source code where it is not clear how those were created e.g. |
|
I'd love to understand the Windows failures. Unfortunately I don't have access to any windows box myself and all the MacOS / Linux things work ok. The fact that it fails consistently worries me as this implies that there is something systemic. I can't re-run the github actions; could you rerun them with debug logging enabled? |
The problem was that Filenames on Windows and *ix hash differently, so the test, which relies on the exact order of the files passed into the location manager retained not the first but the second file. The PR used a HashMap where it should use a LinkedHashMap. After fixing this, the tests pass on windows (at least locally for me). |
|
Let me try to express my concerns again: just like javac there's no such things as scoped artifacts, i.e. the difference between compile and test: it is just a bunch of jars and that will be divided over the modulepath and classpath. In case of a required jar you cannot assume that it is pointing to the explicit module instead of the automatich module. Hence the way to solve this is to make sure they have different names, not by adding this logic! |
Aren't you assuming that both jars have module names ? In this case, only one has a module name, not both. |
|
The differentiator is "has a module descriptor" and "does not have a module descriptor". jars that have a module descriptor must go on the module path. It becomes much more murky with jars that have either an automatic module name in the manifest or have the vm generate a module name based on the filename. Fundamentally, those need to be treated the same (because the VM treats them the same). What can happen is that you have a jar that has a module descriptor and a jar that has not that clash. In that case, the former must go on the module path. We can now argue what should happen with the second:
The current behavior is 1 + 2 (which is what I am observing). I propose that "3" is a better world to live in (the patch explicitly prefers modules with a module descriptor) and matches a large number of real world scenarios where projects transition from "no JPMS support" over "automatic module names" to "module descriptors". The fourth option is also a viable alternative IMHO, except that it is user unfriendly where a better alternative exists. |
Every jar has a module name, either explicit (so it has a module descriptor, hence you also know its requirements) or automatic (either via the manifest of by filename). Suppose my module descriptor contains a requirement on module q, then there's zero guarantee it is pointing to the explicit one. It could very well the filenamebased one, while the explicit one is pulled in via a different module. |
|
So far this code can be explained in a single line: it takes the ordered list of jars, scans the used module descriptors and put all required jars on the modulepath, the rest on the classpath. I still don't support the usecase, especially because module name collisions can be avoided by defining a proper and unique module name and as far as I understand it is not a third party library, so you do control the module names. While writing this code I considered it as a utility, so I wanted to avoid adding a logging framework (even JUL). The ResolvePathsResult should contain all the information and the consumer (e.g. maven-compiler-plugin or maven-surefire-plugin) can decide what to do. So what might be missing is the reason why this module is not on either modulepath or classpath. It is possible to figure out its modulename and discover it is a duplicate, but maybe it makes sense to raise a pathexception for duplicatemodulename for better understanding. |
In a maven project, a maven module creates two artifacts:
- a main artifact with a module-info.class file that defines a JPMS
module "foo.bar". The jar is called foo-bar-1.0.jar
- a test artifact which is not a JPMS module. It is called
foo-bar-1.0-tests.jar
Another module declares dependencies on both modules:
<dependencies>
<dependency>
<groupId>xxx</groupId>
<artifactId>foo-bar</artifactId>
<version>1.0</version>
</dependency>
<dependency>
<groupId>xxx</groupId>
<artifactId>foo-bar</artifactId>
<version>1.0</version>
<classifier>tests</classifier>
<scope>test</scope>
</dependency>
</dependencies>
This is a common use case in large projects. The LocationManager now
creates two JavaModuleDescriptors for the "foo.bar" JPMS module. One
from the main artifact and its module-info.class (automatic == false)
and one from the test artifact (automatic == true).
The current code considers these duplicates and drops one. As a result,
the test code no longer compiles because a dependency is missing.
The patch separates out modules with a module descriptor and automatic
modules.
Then it adds the modules with module descriptors to the module path.
Any duplicate is dropped for modules with module descriptors.
Finally, it adds the automatic modules; if a module with the same module
id already exists on the module path, it adds it to the class path.
In the case above, this will allow the compile to successfully compile
test code (the tests dependency drops to the class path, while the main
artifact is on the module path).
hgschmie
force-pushed
the
jpms-with-tests
branch
from
b850d6e to
e1bef18
Compare
|
I keep coming back to this thread and I am still not sure how to proceed. @rfscholte you keep talking about logging frameworks. No one but you has suggested any logging framework. The way I see it, the current behavior is simply wrong. Dropping a path element quietly leads to problems. Looking at the options above, we are currently at "1". I can live with "4" which will show the problems to the user and they can fix their build (and don't have to guess what went wrong). I would be more comfortable with implementing 2 and 3 (which is what this PR does) but at the very minimum this should end up as a path exception. We are discussing this for two weeks now without any clear resolution in sight. Even though it has been approved, it is not applied. What is the next step? |
|
There's not a "not approve" option, otherwise I would have picked that one, which would make it still undecided. |
|
Ok, then let's do that. I will close this PR and submit a new one that errors out if it sees multiple modules with the same module name. My interest is around making this predictable and actionable for users. The current behavior is not. |
That is good to know, thank you for pointing this out. I admittedly only glanced at the JLS and JEP 261 and the only reference that I could find was in JEP 261: "[...]When searching a module path for a module of a particular name, the module system takes the first definition of a module of that name. Version strings, if present, are ignored; if an element of a module path contains definitions of multiple modules with the same name then resolution fails and the compiler, linker, or virtual machine will report an error and exit.[...]" If this is relevant to the module path resolution, it seems that erroring out might be an acceptable outcome. |
In a maven project, a maven module creates two artifacts:
module "foo.bar". The jar is called foo-bar-1.0.jar
foo-bar-1.0-tests.jar
Another module declares dependencies on both modules:
This is a common use case in large projects. The LocationManager now
creates two JavaModuleDescriptors for the "foo.bar" JPMS module. One
from the main artifact and its module-info.class (automatic == false)
and one from the test artifact (automatic == true).
The current code considers these duplicates and drops one. As a result,
the test code no longer compiles because a dependency is missing.
The patch separates out modules with a module descriptor and automatic
modules.
Then it adds the modules with module descriptors to the module path.
Any duplicate is dropped for modules with module descriptors.
Finally, it adds the automatic modules; if a module with the same module
id already exists on the module path, it adds it to the class path.
In the case above, this will allow the compile to successfully compile
test code (the tests dependency drops to the class path, while the main
artifact is on the module path).