Le mer. 31 oct. 2018 à 14:52, Daniel Cunha <daniels...@gmail.com> a écrit :
> Em seg, 29 de out de 2018 às 14:01, Jonathan Gallimore < > jonathan.gallim...@gmail.com> escreveu: > > > We should write a test case for both of these, and agree on them. I'd > like > > to be able to merge this in with confidence, and without the tests I > don't > > see how we can do that. > > > > Jon > > > > On Mon, Oct 29, 2018 at 3:35 PM Romain Manni-Bucau < > rmannibu...@gmail.com> > > wrote: > > > > > Hi Daniel, > > > > > > the point is that this code is reused in several places so we must be > as > > > clean as we can, here the cases I had in mind: > > > > > > 1. reused main in another main (so an app calls the main): here the > > pitfall > > > is that the property openejb.classloader.first.disallow-system-load is > > not > > > resetted and the TCLL neither so after the main you use a closed > > > classloader (so will likely easily fail) > > > > I really didn't get your point. Side note we are create a classloader only > for tomee cli execution, which keep alive only for a short time during > command execution. > > That part of code is not reused in another place, if yes, just point me > where, because I can't find it. > > It will close only after the program finish, since it is inside a try-catch > block, no? > Assuming the main is launch but this main is sometimes launched programmatically (like in a webapp) When not you are right > > > > > 2. if a user relies in java 8 on the classloader being the system one > > then > > > your new classloader level breaks it so we shouldnt create this new > > > classloader when not needed and the old ClassPath impl is working IMO. > We > > > can enrich it with a warn log saying "it will fail in next version" > (and > > we > > > drop that code after 1 or 2 releases). Idea is to not break directly > > users > > > who can not even know some of the libs they added in the classpath for > > > logging purposes rely on that classloader setup - random example indeed > > ;). > > > > > > > Yeah, I probably could use the SystemClassPath, but it is doing much more > than I need to CLI. I don't think that I need to change the java.class.path > property to make it works or other property like > openejb.classloader.first.disallow-system-load. > We should likely be cleaner here but not your PR, fair enough > > And to be honest, I didn't understand what is > openejb.classloader.first.disallow-system-load and why do we have it. :) > It is used in the "webapp" loader to check if we should load some well known classes from the system classloader or not. It is likely the kind of property you rely on when you want to be embeddable but also standalone. > > > > > Romain Manni-Bucau > > > @rmannibucau <https://twitter.com/rmannibucau> | Blog > > > <https://rmannibucau.metawerx.net/> | Old Blog > > > <http://rmannibucau.wordpress.com> | Github < > > > https://github.com/rmannibucau> | > > > LinkedIn <https://www.linkedin.com/in/rmannibucau> | Book > > > < > > > > > > https://www.packtpub.com/application-development/java-ee-8-high-performance > > > > > > > > > > > > > Le lun. 29 oct. 2018 à 16:14, Daniel Cunha <daniels...@apache.org> a > > > écrit : > > > > > > > Hi, > > > > > > > > I updated the PR with tests! > > > > > > > > Em seg, 29 de out de 2018 às 08:27, Jonathan Gallimore < > > > > jonathan.gallim...@gmail.com> escreveu: > > > > > > > > > I saw your note on the PR regarding Romain's feedback. > > > > > > > > > > For visibility here, Romain is querying whether this patch will > cause > > > an > > > > > issue with folks calling the CLI with a custom classpath using > `java > > > -cp > > > > > ...` under Java 8. I'd suggest we add a test for it, which looks to > > be > > > > what > > > > > Daniel is doing. > > > > > > > > > > Thanks guys. > > > > > > > > > > Jon > > > > > > > > > > On Thu, Oct 25, 2018 at 12:35 PM Daniel Cunha < > daniels...@apache.org > > > > > > > > wrote: > > > > > > > > > > > Hi Folks, > > > > > > > > > > > > I sent a PR on TomEE to fix the TOMEE-2253. I got a lot of > feedback > > > > from > > > > > > Romain (thank you Romain for all comments and help) > > > > > > > > > > > > I believe which we are in good shape to merge. > > > > > > The changes cover from Java 8 till Java 11 which not provide any > > > impact > > > > > for > > > > > > TomEE or application deployed on it. > > > > > > > > > > > > Please, look at the PR and let me know what you think. > > > > > > > > > > > > PR: https://github.com/apache/tomee/pull/176 > > > > > > > > > > > > -- > > > > > > Daniel "soro" Cunha > > > > > > https://twitter.com/dvlc_ > > > > > > > > > > > > > > > > > > > > > > > -- > > > > Daniel "soro" Cunha > > > > https://twitter.com/dvlc_ > > > > > > > > > > > Thank you! :) > -- > Daniel "soro" Cunha > https://twitter.com/dvlc_ >
