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

Support defining a list of module overrides for Maven/Java modules #81

Open
FroMage opened this issue Oct 21, 2013 · 30 comments
Open

Support defining a list of module overrides for Maven/Java modules #81

FroMage opened this issue Oct 21, 2013 · 30 comments

Comments

@FroMage
Copy link
Member

FroMage commented Oct 21, 2013

We should try to fix all the Maven issues by allowing the user to specify a file which lists a number of Maven overrides which allow us for each module name / version pair to:

  • add dependencies
  • replace dependencies
  • delete dependencies
@akberc
Copy link
Member

akberc commented Oct 23, 2013

+1 Thank you.
In a very similar ceylon.build task, I am going to be using Ceylon declarative style to define Dependency { .... {Exclusion {

Please reference your approach so that we can align the IDE 'export Java module', this issue and the ceylon.build task.

I understand that they maybe totally unrelated, but they are tied to the user experience of being able to use Java modules without understanding too much of the deeper problems.

@FroMage
Copy link
Member Author

FroMage commented Oct 28, 2013

@alesj I merged your branch and added support for --maven-overrides parameters. I can confirm it lets me compile Maven-using modules more easily, but I have a much harder time at runtime.

Using the latest distribution (yes you have to build one), you can test with:

../ceylon-dist/dist/bin/ceylon --rep cmr-test --rep aether --maven-overrides cmr-test/overrides.xml com.redhat.ceylon.compiler.java.test.cmr.modules.bug1100/1

Using the cmr-test repo at http://stephane.epardaud.fr/cmr-test-81.zip

It will fail with:

Caused by: java.lang.LinkageError: loader constraint violation: when resolving method "org.eclipse.jetty.servlet.ServletHolder.setServlet(Ljavax/servlet/Servlet;)V" the class loader (instance of org/jboss/modules/ModuleClassLoader) of the current class, org/apache/camel/component/jetty/JettyHttpComponent, and the class loader (instance of org/jboss/modules/ModuleClassLoader) for resolved class, org/eclipse/jetty/servlet/ServletHolder, have different Class objects for the type t/Servlet;)V used in the signature

That's as far as I got. I added lots of overrides to make certain imports shared, thanks to the hard-to-decypher info I get with the --verbose flag, but then that one I just don't understand.

It seems that a lot of issues have to do with not only missing dependencies but also that they should be shared, even more so for the runtime than for the compiler.

Can you help me with these @alesj ?

The source code is:

module com.redhat.ceylon.compiler.java.test.cmr.modules.bug1100 "1" {
    import java.base "7";
    import "commons-codec:commons-codec" "1.4";
    import "org.apache.camel:camel-core" "2.9.4";
    import "org.apache.camel:camel-jetty" "2.9.4";
}
import org.apache.camel.impl { DefaultCamelContext }
import org.apache.camel.component.jetty { JettyHttpComponent }
import org.apache.camel.builder { RouteBuilder }
import java.lang { Thread { currentThread } }

"Run the module `simple`."
shared void run() {
  print("Start Camel");
  value context = DefaultCamelContext();
  context.addComponent("jetty", JettyHttpComponent());
  object routeBuilder extends RouteBuilder() {
    shared actual void configure() {
      from("jetty:http://localhost:8080").log("got request");
    }
  }
  context.addRoutes(routeBuilder);
  context.start();
  currentThread().sleep(10000);
  print("Stop Camel");
}

@FroMage
Copy link
Member Author

FroMage commented Oct 28, 2013

Also, I had lots of issues when importing maven modules with a . rather than a : separator, because the CMR imports those with a :, which then leads to duplicate modules foo:bar and foo.bar which causes lots of issues. So I vote we force Maven imports to use a : in the module name and:

  • automatically resolve from Aether if there is a : in the module name (no more need for --rep aether unless to override the settings.xml)
  • never resolve from Aether if there is no : in the module name.

We wanted that behaviour anyways but thought of adding an annotation to the import for that, but since not using the : separator leads to issues and ambiguity, I think this would solve all those issues at once.

@dmlloyd
Copy link

dmlloyd commented Oct 28, 2013

Guys using version numbers in your run time imports is a very very bad idea, unless you want to see lots and lots of LinkageErrors like above and/or enter OSGi hell. I know it seems clever but it's going to break down on you really fast as the number of installed modules increases. It would make much more sense to keep a single (or perhaps two or at most three) parallel version streams which you keep up to date via whatever mechanism, and perhaps add in version constraints to make sure things are properly enforced. Depending on specific versions is going to give you nothing but trouble.

@dmlloyd
Copy link

dmlloyd commented Oct 28, 2013

The LinkageError itself is due to inheriting from more than one version of the same base class or interface at the same time, which is a direct expression of this problem.

@FroMage
Copy link
Member Author

FroMage commented Oct 28, 2013

I don't know what you're telling us. I'm pretty sure I only import a single version of camel. I don't know which module is having multiple versions loaded at the same time and which versions these are, and who imported them and why they are even visible to each other.

Or how to solve this. This is using nothing but Maven modules, and they do specify versions. Should we attempt to merge them all to one?

Is there a way in JBoss Modules to dump a list of loaded modules and their dependencies?

@quintesse
Copy link
Member

@dmlloyd Not sure what you refer to when mentioning "version streams"? Or the problem with depending on specific versions. Isn't this what JBoss Modules does? Isn't this what slots are for?

@alesj
Copy link
Member

alesj commented Oct 28, 2013

Is there a way in JBoss Modules to dump a list of loaded modules and their dependencies?

I know there is some JMX support in it.
But I doubt it knows to dump dependencies. @dmlloyd ?

@dmlloyd
Copy link

dmlloyd commented Oct 28, 2013

No, slots are definitely not for supporting N versions of one JAR. The JVM simply is not capable of doing this. Even Maven resolves down to a single cohesive version set (or tries to anyway). And, I'm not the one telling you; the JVM is telling you specifically that you are linking to two versions of one class name from a subclass which is disallowed by the JVM linking rules. It even tells you where those classes came from.

Yes the latest versions of JBoss Modules have a dependency printer but iirc it only supports the built-in static module loader. It wouldn't be too hard to write one for your own module loader though.

@quintesse
Copy link
Member

No, slots are definitely not for supporting N versions of one JAR

No, I meant when you say "using version numbers in your run time imports is a very very bad idea".

But isn't that what those versions numbers are for, to be able to decide what goes with what and maybe say "sorry, you're trying to use N different versions of this module, no can do"

@dmlloyd
Copy link

dmlloyd commented Oct 28, 2013

Yes, if that's all you're using them for (a.k.a. "constraints"). It's when multiple versions of things get pulled in that it's a problem.

Note that this happens not only with versions but also with equivalent JARs with different GAV coordinates. This is why we do not directly use Maven information within the Wildfly appserver.

@gavinking
Copy link
Member

Caused by: java.lang.LinkageError: loader constraint violation: when resolving method "org.eclipse.jetty.servlet.ServletHolder.setServlet(Ljavax/servlet/Servlet;)V" the class loader (instance of org/jboss/modules/ModuleClassLoader) of the current class, org/apache/camel/component/jetty/JettyHttpComponent, and the class loader (instance of org/jboss/modules/ModuleClassLoader) for resolved class, org/eclipse/jetty/servlet/ServletHolder, have different Class objects for the type t/Servlet;)V used in the signature

the JVM is telling you specifically that you are linking to two versions of one class name from a subclass which is disallowed by the JVM linking rules. It even tells you where those classes came from.

I have an issue report in my web browser I've been meaning to submit for a couple of days, but let me say it here too: that error message is, in 2013, totally inadequate. JBoss Modules should be capable of a much better error message. This kind of thing was barely acceptable back in the Clinton administration, but not any more. ;-)

@dmlloyd
Copy link

dmlloyd commented Oct 28, 2013

JBoss Modules can only provide what the JVM gives us access to, which is precious little.

@gavinking
Copy link
Member

@dmlloyd Well, let's debate that somewhere else.

@FroMage
Copy link
Member Author

FroMage commented Oct 28, 2013

So, @alesj WDYT? It seems to me that if we want to merge every imported Maven module so that only one can live at the same time with a single version, we have to stop doing all this lazy loading of modules because we need the full graph resolved at startup and all dependencies figured out for Maven.

So now we have overrides and I'm still not convinced this got us closer to supporting Maven modules.

I'm going to guess this isn't an issue for Ceylon modules because the shared imports are actually enforced so two a single module can't ever see two versions of the same module loaded at once, even if it happens privately.

@alesj
Copy link
Member

alesj commented Oct 28, 2013

@FroMage I think overrides should be fine, you just need to do it properly aka more exact.
Where I doubt flat / full graph will solve the problem either.

@FroMage
Copy link
Member Author

FroMage commented Oct 29, 2013

Well, did you find the proper override file to make this work? I doubt most humans will be able to. Especially when it "just works" using Maven and a flat classpath.

@alesj
Copy link
Member

alesj commented Oct 29, 2013

@FroMage how did you create the overrides.xml in the first place?
Afair, there was some missing class in the first place.

@FroMage
Copy link
Member Author

FroMage commented Oct 30, 2013

Tic-toc tic-toc…

@FroMage
Copy link
Member Author

FroMage commented Oct 30, 2013

I created it by trial and error looking at the compiler logs first and then the jboss modules logs for every time it failed and why.

@alesj
Copy link
Member

alesj commented Oct 30, 2013

Well, atm it looks you only got the javax.servlet thing wrong -- from 10.000ft. :-)
Dunno how to fix it exactly, but - since you have it all set - perhaps give it another try.

And yes, I do agree it's a non-human task, hence we'll probably need to add some tooling to it asap.

@FroMage
Copy link
Member Author

FroMage commented Oct 31, 2013

So why don't you try it and see if you can find the proper override?

@FroMage
Copy link
Member Author

FroMage commented Nov 6, 2013

This is going to slip to 1.1

@sgalles
Copy link

sgalles commented Jun 7, 2014

@FroMage you asked for feedback about this feature, now I think that I can give my humble opinion about this.

Well, I'm a bit pessimistic actually.
As is, I think that most developers will just give up before being able to just see their module run (or maybe compile).

For instance this is the Dropwizard dependency tree generated by gradle

compile
\--- com.yammer.dropwizard:dropwizard-core:0.6.2
     +--- com.sun.jersey:jersey-core:1.17.1
     +--- com.sun.jersey:jersey-server:1.17.1
     |    \--- asm:asm:3.1
     +--- com.sun.jersey:jersey-servlet:1.17.1
     +--- com.yammer.metrics:metrics-core:2.2.0
     |    \--- org.slf4j:slf4j-api:1.7.2 -> 1.7.4
     +--- com.yammer.metrics:metrics-servlet:2.2.0
     |    +--- com.yammer.metrics:metrics-core:2.2.0 (*)
     |    \--- com.fasterxml.jackson.core:jackson-databind:2.1.1 -> 2.1.4
     |         +--- com.fasterxml.jackson.core:jackson-annotations:2.1.4
     |         \--- com.fasterxml.jackson.core:jackson-core:2.1.4
     +--- com.yammer.metrics:metrics-jetty:2.2.0
     |    +--- com.yammer.metrics:metrics-core:2.2.0 (*)
     |    \--- org.eclipse.jetty:jetty-server:8.1.8.v20121106 -> 8.1.10.v20130312
     |         +--- org.eclipse.jetty.orbit:javax.servlet:3.0.0.v201112011016
     |         +--- org.eclipse.jetty:jetty-continuation:8.1.10.v20130312
     |         \--- org.eclipse.jetty:jetty-http:8.1.10.v20130312
     |              \--- org.eclipse.jetty:jetty-io:8.1.10.v20130312
     |                   \--- org.eclipse.jetty:jetty-util:8.1.10.v20130312
     +--- com.yammer.metrics:metrics-logback:2.2.0
     |    +--- com.yammer.metrics:metrics-core:2.2.0 (*)
     |    +--- ch.qos.logback:logback-core:1.0.7 -> 1.0.10
     |    \--- ch.qos.logback:logback-classic:1.0.7 -> 1.0.10
     |         +--- ch.qos.logback:logback-core:1.0.10
     |         \--- org.slf4j:slf4j-api:1.7.2 -> 1.7.4
     +--- com.yammer.metrics:metrics-jersey:2.2.0
     |    +--- com.yammer.metrics:metrics-core:2.2.0 (*)
     |    +--- com.yammer.metrics:metrics-annotation:2.2.0
     |    \--- com.sun.jersey:jersey-server:1.15 -> 1.17.1 (*)
     +--- com.fasterxml.jackson.core:jackson-databind:2.1.4 (*)
     +--- com.fasterxml.jackson.jaxrs:jackson-jaxrs-json-provider:2.1.4
     |    +--- com.fasterxml.jackson.core:jackson-core:2.1.4
     |    +--- com.fasterxml.jackson.core:jackson-databind:2.1.4 (*)
     |    \--- com.fasterxml.jackson.module:jackson-module-jaxb-annotations:2.1.4
     |         +--- com.fasterxml.jackson.core:jackson-core:2.1.4
     |         \--- com.fasterxml.jackson.core:jackson-databind:2.1.4 (*)
     +--- com.fasterxml.jackson.dataformat:jackson-dataformat-yaml:2.1.4
     |    \--- com.fasterxml.jackson.core:jackson-core:2.1.4
     +--- com.fasterxml.jackson.datatype:jackson-datatype-guava:2.1.2
     |    +--- com.fasterxml.jackson.core:jackson-databind:2.1.2 -> 2.1.4 (*)
     |    +--- com.fasterxml.jackson.core:jackson-core:2.1.2 -> 2.1.4
     |    \--- com.google.guava:guava:11.0.2 -> 14.0.1
     +--- net.sourceforge.argparse4j:argparse4j:0.4.0
     +--- org.slf4j:slf4j-api:1.7.4
     +--- org.slf4j:jul-to-slf4j:1.7.4
     |    \--- org.slf4j:slf4j-api:1.7.4
     +--- ch.qos.logback:logback-core:1.0.10
     +--- ch.qos.logback:logback-classic:1.0.10 (*)
     +--- org.slf4j:log4j-over-slf4j:1.7.4
     |    \--- org.slf4j:slf4j-api:1.7.4
     +--- org.eclipse.jetty:jetty-server:8.1.10.v20130312 (*)
     +--- org.eclipse.jetty:jetty-servlet:8.1.10.v20130312
     |    \--- org.eclipse.jetty:jetty-security:8.1.10.v20130312
     |         \--- org.eclipse.jetty:jetty-server:8.1.10.v20130312 (*)
     +--- org.eclipse.jetty:jetty-http:8.1.10.v20130312 (*)
     +--- com.google.guava:guava:14.0.1
     +--- com.google.code.findbugs:jsr305:2.0.1
     +--- org.hibernate:hibernate-validator:4.3.1.Final
     |    +--- javax.validation:validation-api:1.0.0.GA
     |    \--- org.jboss.logging:jboss-logging:3.1.0.CR2
     +--- joda-time:joda-time:2.2
     \--- com.fasterxml.jackson.datatype:jackson-datatype-joda:2.1.2
          +--- com.fasterxml.jackson.core:jackson-core:2.1.2 -> 2.1.4
          +--- com.fasterxml.jackson.core:jackson-databind:2.1.2 -> 2.1.4 (*)
          \--- joda-time:joda-time:2.1 -> 2.2

It does not matter if the dependencies here are conceptually correct or not. It just works in the java world.

First, you see that gradle (like Maven) use a strategy to remove the version conflicts (2.1.2 -> 2.1.4 means 2.1.2 requested, but 2.1.4 retained).
Now, I think it is clear that this depency tree will generates java.lang.LinkageError all over the place the way the Maven interop works.

OK, so I guess that a tool could help to solve the java.lang.LinkageError problems. This tool could generate a overrides.xml that simulate the version conflict resolution used by Gradle or Maven.

Problem is, there are classes of problem that, I think, can only be resolved manually.

We have a very good example here.

At some point trying to make my Dropwizard work, I got :
java.lang.ClassCastException: org.slf4j.helpers.NOPLogger cannot be cast to ch.qos.logback.classic.Logger
Here no fancy classloading error. Simply, the slf4j-api could not find the ch.qos.logback.classic.Logger implementation of the logger.

Actually to make this work the tree must be transformed
from

 +--- ch.qos.logback:logback-core
     +--- ch.qos.logback:logback-classic
     +--- org.slf4j:log4j-over-slf4j
     |    \--- org.slf4j:slf4j-api

to

 ++--- ch.qos.logback:logback-core
     +--- ch.qos.logback:logback-classic
     +--- org.slf4j:log4j-over-slf4j
     |    \--- org.slf4j:slf4j-api
                \--- ch.qos.logback:logback-classic <-- added here

This can't be automatized.

BTW, I have a better understanding now of the reason why I was able to make my exemple work by importing the Maven modules into Ceylon modules via gradles scripts (in my local Ceylon repos).
By using the Gradle version conflict resolution, at least, the version conflict was solved before runtime (no java.lang.LinkageError ).
Of course it was not enough, I've also done something plain hugly : my script created the modules with a dependency on all other modules in the dependency tree. I've vaguely emulated the 'flat java classpath'. I guess that's why it worked.

Bottom line, given that the existing Maven librairies were not created to be used in a non-flat classpath, I'm naively wodering if the classloading scheme of Ceylon should not aggregate the whole Maven dependency tree into a huge virtual 'Maven' Ceylon module (of course such module would share every single class :/ )

This is super ugly but this might be the price to pay to have a real easy interop with Maven. And anyway, it does not prevent people from creating clean Ceylon modules for these jars, with clean dependencies manually created.

My two cents.

@akberc
Copy link
Member

akberc commented Jun 7, 2014

IMHO Maven overrides are too simplistic for real-life scenarios, and there may not be any further options under module resolver. We have had the conversation a few times and it appears that it would be better handled by 3rd-party tools and the Ceylon ecosystem and let the core Dev team tackle the bigger issues..

Please take a look here:
https://github.com/dgwave/ceylon-maven-plugin
https://github.com/dgwave/lahore-tasks

After 1.1, release ceylon maven plugin and ceylon.build tasks may provide more useful interaction with Maven: I don't have a solution yet, but I am trying to tackle the problem from both ends: Ceylon and Maven.

@sgalles
Copy link

sgalles commented Jun 8, 2014

IMHO Maven overrides are too simplistic for real-life scenarios

I agree.

there may not be any further options under module resolver

I don't know. The Gradle team has a nice programatic solution for this. For the Ceylon compiler/runtime it might work with a Java/Ceylon class plugin or Ceylon code in the module descriptor.
http://www.gradle.org/docs/current/dsl/org.gradle.api.artifacts.ResolutionStrategy.html

it appears that it would be better handled by 3rd-party tools and the Ceylon ecosystem and let the core Dev team tackle the bigger issues..

Yes. I agree too. But it is a pity that the existing Maven interop infrastructure embedded in Ceylon can not be leveraged at least for 80% of the use cases (and the difficult 20% could indeed be handled by the 3rd-party tools)

Please take a look here:
https://github.com/dgwave/ceylon-maven-plugin
https://github.com/dgwave/lahore-tasks

Yes, I really like the approach you choose. And indeed it will definitly help to solve the 20% hard cases related to Maven interop per se.
Problem is, now I'm really more worried about the altered behavior of libraries like org.slf4j:slf4j-api in the non-flat classpath of Ceylon. Even if the version conflicts are solved, at the end of the day, I will have to make my Maven libraries work in a non flat classpath, and there is a risk that I don't succed.

And this risk is worrying, because for every new Maven library this risk exists. And best case, I may succed, but it does not work out of the box and I can plan several hours (days ?) of trial and error to integrate this new Maven library.

That's why I was interested with the modules with multiple resource-root
https://groups.google.com/forum/#!topic/ceylon-users/nHQajoGSfQ0
I wanted to check that at least I could fall back on this to create the 'huge maven module' I was talking about (statically in my local repos, or my private Herd repos for instance)

After 1.1, release ceylon maven plugin and ceylon.build tasks may provide more useful interaction with Maven: I don't have a solution yet, but I am trying to tackle the problem from both ends: Ceylon and Maven.

Great, I'm looking forward to it.

@gavinking
Copy link
Member

Isn't these problems something that assemblies would solve? Indeed, isn't this maven-overrides XML file just a rubbish hacked-in assembly descriptor?

@sgalles
Copy link

sgalles commented Jun 8, 2014

Isn't these problems something that assemblies would solve?

👍 +1

@FroMage
Copy link
Member Author

FroMage commented Jun 10, 2014

Isn't these problems something that assemblies would solve? Indeed, isn't this maven-overrides XML file just a rubbish hacked-in assembly descriptor?

Well, you can't say at the same time that it's an assembly descriptor and that it's rubbish. It is indeed a way to declare overrides for bogus Maven dependencies, which, I gather from guessing is part of your secret project for assemblies. It's XML because that was the easiest thing to try, and insofar as it's part of the assembly functionality, it doesn't seem to be working as expected, because figuring out how to fix Maven dependencies is not trivial, as we've seen.

It is, however a good way to test our ideas, and where assemblies will go and where they will face issues.

It has been suggested several times that we should put all of Maven dependencies inside a flat classpath, but I've rejected this approach so far because the notion of flat classpath is not well-defined when it comes to modularity. Consider two Ceylon modules which include two distinct Maven modules, which in turn import common Maven modules with different versions. Do we have one "flat classpath group" for each Ceylon module? Or a single one which unifies the two imports?

ATM we do dependency resolution on a per-import basis, so we can't unify the two groups after the fact. We could change this, but then when we support dynamic loading of modules, how are we going to unify that third Maven module when the two first are already loaded and resolved?

The problem gets worse if the Maven modules import Ceylon modules, because suddenly that flat classpath part can view classes from different versions of the same Ceylon module, which only works in a non-flat classpath.

I suppose that we can make the Maven imports much closer to how Maven does it by unifying all the imports and merging the constraints to generate the correct override list. Either with a new command that would produce an override.xml or do it automatically at runtime. But until now we've always made overrides explicit. Note that this would not solve the issue of missing module imports, which you noticed.

It's clear we have issues and we can improve. I suggest we improve one bit at a time to find the right solution. It's not clear at all that a flat classpath is a better solution. It may be, and yes it would solve the part about missing imports, but it would leave the issue about version override resolution, which we have to solve in both cases, and it would open questions about composability. If anyone has other ideas, we're taking.

@FroMage
Copy link
Member Author

FroMage commented Sep 25, 2014

Moving again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants