4:09 p.m.
2016-10-27 15:55 GMT+02:00 Scott Marlow <smarlow(a)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?
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.
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.
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.
On Thu, Oct 27, 2016 at 9:40 AM, Gunnar Morling <gunnar(a)hibernate.org>
wrote:
>
>
> 2016-10-27 14:59 GMT+02:00 Scott Marlow <smarlow(a)redhat.com>:
>>
>> On Thu, Oct 27, 2016 at 4:49 AM, Gunnar Morling <gunnar(a)hibernate.org>
>> wrote:
>> > Hi,
>>
>> Hi,
>>
>> >
>> > 2016-10-27 4:27 GMT+02:00 Scott Marlow <smarlow(a)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.
>
>