Hi Romain, Ok, I'll create a scenario for it.
Thank you. Em ter, 20 de nov de 2018 às 17:29, Romain Manni-Bucau < rmannibu...@gmail.com> escreveu: > Hi Daniel > > Do you think you can assert the classloader in the execution thread didnt > change after the call? Was one concern and i didnt find the matching assert > > Le mar. 20 nov. 2018 20:00, Daniel Cunha <daniels...@apache.org> a écrit : > > > Hi folks, > > > > I've added a new test with JAXRS, calling a command in my endpoint from > > Bootstrap CLI. > > Please review the changes on my PR[1] and let me know what do you think > > about it. > > > > Thank you. :) > > > > [1] https://github.com/apache/tomee/pull/176 > > > > Em seg, 19 de nov de 2018 às 13:57, Daniel Cunha <daniels...@gmail.com> > > escreveu: > > > > > Hi Jon, > > > > > > ok. I got it! :) > > > > > > Em seg, 19 de nov de 2018 às 13:49, Jonathan Gallimore < > > > jonathan.gallim...@gmail.com> escreveu: > > > > > >> On Mon, Nov 19, 2018 at 4:30 PM Daniel Cunha <daniels...@gmail.com> > > >> wrote: > > >> > > >> > Hi guys, > > >> > > > >> > I updated the PR with some chages: > > >> > 1 - Move BasicURLClassPath.CustomizableURLClassLoader to be public. > > >> > 2 - I'm using BasicURLClassPath.CustomizableURLClassLoader instead > of > > >> > DynamicClassLoader (class that I created) > > >> > 3- DynamicClassLoader is not needed anymore. Was removed from the > PR. > > >> > 3 - Added more tests: > > >> > - 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 > > >> > > > >> > Question: > > >> > > > >> > @Jon, > > >> > what do you want me mean with: A webapp calling commands in the CLI? > > >> > > > >> > > >> Hopefully I can answer that one in a straightforward way :). Try > > creating > > >> a > > >> servlet that loads the Bootstrap CLI class, and invokes some methods. > > >> Effectively you'd be calling the TomEE command line commands from a > > >> servlet. That use case hadn't occurred to me, but it sounds like one > > worth > > >> trying. > > >> > > >> That would probably need to sit > > >> under > > arquillian/arquillian-tomee-tests/arquillian-tomee-webprofile-tests. > > >> > > >> Jon > > >> > > >> > > >> > > > >> > Command is a class with main method, if I've it deployed in my > library > > >> > folder, it should works fine, I just need to create a instance and > > call > > >> the > > >> > main. > > >> > Same will be if I try execute it with Process (that is exactly what > we > > >> are > > >> > testing). > > >> > > > >> > Em sex, 2 de nov de 2018 às 07:53, Jonathan Gallimore < > > >> > jonathan.gallim...@gmail.com> escreveu: > > >> > > > >> > > 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_ > > >> > > > > > > >> > > > > > >> > > > > >> > > > >> > > > >> > -- > > >> > Daniel "soro" Cunha > > >> > https://twitter.com/dvlc_ > > >> > > > >> > > > > > > > > > -- > > > Daniel "soro" Cunha > > > https://twitter.com/dvlc_ > > > > > > > > > -- > > Daniel "soro" Cunha > > https://twitter.com/dvlc_ > > > -- Daniel "soro" Cunha https://twitter.com/dvlc_
