I'm following this thread with some interest. What would be some good tests to make sure these cases are covered off? Some suggestions in addition to the existing test:
* Standalone client that has its own main and delegates to the CLI main * As above, but perhaps with the standalone client creating a classloader with the CLI jars and then launching it * A webapp calling commands in the CLI Any others? These are interesting use cases that I hadn't considered - this is interesting. Jon On Wed, Oct 31, 2018 at 2:30 PM Romain Manni-Bucau <rmannibu...@gmail.com> wrote: > 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_ > > >
