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) 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 ;). 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_ >
