On Thu, Oct 27, 2016 at 10:09 AM, Gunnar Morling <gun...@hibernate.org> wrote: > 2016-10-27 15:55 GMT+02:00 Scott Marlow <smar...@redhat.com>: >> >> I remember now why I didn't remove the JPADependencyProcessor >> injection of Javassist in >> https://github.com/wildfly/wildfly/pull/8474, it would cause a failure >> in applications that embeds a copy of the ORM jars but the application >> doesn't have the Javassisst jars on its classpath. Currently, >> JPADependencyProcessor ensures that Javassist is available to such an >> application. > > > Why would one support such use case? If a user is adding their own ORM in > the app, I'd expect them to do it completely, with all the dependencies of > *that version*. I mean, how would you even know whether the Javassist > version you are adding is the right one?
I think that exporting the Javassist classes from ORM was considered a bad idea back then in 2011. Now, we don't want to lose application compatibility. > > And what happens if the user *is* providing Javassist with the app? Tbh. > allowing the user to add their own ORM in the app seems even more a > justification to not inject Javassist. Sure, lets get to the point where we can allow the user to have their own Javassist. > >> I could easily work around the above in the >> >> wildfly/testsuite/compat/src/test/java/org/jboss/as/test/compat/jpa/hibernate/HibernateJarsInDeploymentTestCase.java >> test, however, this would break application compatibility, which is >> more important. This goes back to around 2011 when we first started >> auto injecting the Javassist classes to applications that are using >> Hibernate as a JPA persistence provider. The next time where we would >> be allowed to break application compatibility, would be on an EAP >> .zero release. >> >> If https://github.com/wildfly/wildfly/pull/8474 ever gets merged into >> WildFly, perhaps we could enhance JPADependencyProcessor with a >> persistence unit check for a "don't inject javassist" hint. Or maybe >> that could help the ORM testsuite now, in which case we could create >> an independent pull request for the "don't inject javassist" hint >> handling. > > > Would that hint be another "knob" to be set by the user? If so, I wouldn't > like it too much, it's just such a technical detail we shouldn't bother the > user with. Yes, it would be another knob, however, it is likely to only be used by the Hibernate ORM testsuite, as I haven't heard of any user request for this. > > Another idea: can you examine the ORM module you are about to add and check > wether it exports Javassist and if so, not auto-inject Javassist? Not sure > whether it's doable in terms of capabilities of the module API. I'm not 100%, we would likely need to explore what is available from the application deployment org.jboss.as.server.deployment.module.ModuleSpecification at the early deployment phase that JPADependencyProcessor is called in. ModuleSpecification seems to have getSystemDependencies() + getUserDependencies() + getLocalDependencies() calls, but I'm not sure how (deployment) expensive it would be to sequentially walk through each module dependency and look for javassist classes. > >> >> >> On Thu, Oct 27, 2016 at 9:40 AM, Gunnar Morling <gun...@hibernate.org> >> wrote: >> > >> > >> > 2016-10-27 14:59 GMT+02:00 Scott Marlow <smar...@redhat.com>: >> >> >> >> On Thu, Oct 27, 2016 at 4:49 AM, Gunnar Morling <gun...@hibernate.org> >> >> wrote: >> >> > Hi, >> >> >> >> Hi, >> >> >> >> > >> >> > 2016-10-27 4:27 GMT+02:00 Scott Marlow <smar...@redhat.com>: >> >> >> >> >> >> >> >> >> > Unless I am mistaken, Gunnar is suggesting that the Hibernate ORM >> >> >> > module >> >> >> > (the WF module) export Javassist. Not the application. >> >> > >> >> > >> >> > Right, Hibernate ORM's module should be the one exposing it, not the >> >> > application nor JipiJapa. >> >> >> >> JipiJapa has zero to do with this, we will create a pr later today to >> >> remove the unneeded dependencies, which has nothing to do with this >> >> conversation. >> > >> > >> > Yes, there is this superfluous dependency, thanks for removing it. >> > >> > But the other issue is hat in JPADependencyProcessor (that's what I >> > meant >> > when referring to "JipiJapa", sorry if that's not correct), a dependency >> > to >> > Javassist is injected. This shouldn't be the case for the reasons I've >> > pointed out. Also with the PR >> > https://github.com/wildfly/wildfly/pull/8474 >> > you mentioned this seems to be the case. >> >> >> >> >> >> > >> >> > I've done the following changes locally: >> >> > >> >> > 1) Altered JPADependencyProcessor to not add the Javassist dependency >> >> > to >> >> > the >> >> > deployment >> >> > 2) Altered module.xml of jipijapa-hibernate5 to not depend on >> >> > Javassist >> >> >> >> I did the same locally, which is an unused dependency. I removed some >> >> other unused dependencies other as well. Gail and I will talk later >> >> about removing these unused dependencies. >> >> >> >> > 3) Added a new module for the latest Javassist >> >> > 4) Altered module.xml of ORM itself to depend on that new module >> >> > *and* >> >> > re-export it >> >> > >> >> > With all this in place, the tests in Hibernate Search pass >> >> > successfully. >> >> > >> >> > Scott, do you think we can try another PR with that? Again, it >> >> > doesn't >> >> >> >> https://github.com/wildfly/wildfly/pull/8474 is still open, no need >> >> for a new PR when we already have an open one, with one exception, as >> >> I didn't actually remove the export of Javassist to the application >> >> classpath. I forget why. I'll try that locally and run the WildFly >> >> testsuite to see if there is a failure. >> > >> > > > _______________________________________________ hibernate-dev mailing list hibernate-dev@lists.jboss.org https://lists.jboss.org/mailman/listinfo/hibernate-dev
