TOMEE-2253 - tomee.sh -version not working properly with Java 11

classic Classic list List threaded Threaded
8 messages Options
Reply | Threaded
Open this post in threaded view
|

TOMEE-2253 - tomee.sh -version not working properly with Java 11

Daniel Cunha-2
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_
Reply | Threaded
Open this post in threaded view
|

Re: TOMEE-2253 - tomee.sh -version not working properly with Java 11

jgallimore
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 <[hidden email]> 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_
>
Reply | Threaded
Open this post in threaded view
|

Re: TOMEE-2253 - tomee.sh -version not working properly with Java 11

Daniel Cunha-2
Hi,

I updated the PR with tests!

Em seg, 29 de out de 2018 às 08:27, Jonathan Gallimore <
[hidden email]> 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 <[hidden email]>
> 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_
Reply | Threaded
Open this post in threaded view
|

Re: TOMEE-2253 - tomee.sh -version not working properly with Java 11

Romain Manni-Bucau
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 <[hidden email]> a écrit :

> Hi,
>
> I updated the PR with tests!
>
> Em seg, 29 de out de 2018 às 08:27, Jonathan Gallimore <
> [hidden email]> 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 <[hidden email]>
> > 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_
>
Reply | Threaded
Open this post in threaded view
|

Re: TOMEE-2253 - tomee.sh -version not working properly with Java 11

jgallimore
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 <[hidden email]>
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)
> 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 <[hidden email]> a
> écrit :
>
> > Hi,
> >
> > I updated the PR with tests!
> >
> > Em seg, 29 de out de 2018 às 08:27, Jonathan Gallimore <
> > [hidden email]> 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 <[hidden email]>
> > > 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_
> >
>
Reply | Threaded
Open this post in threaded view
|

Re: TOMEE-2253 - tomee.sh -version not working properly with Java 11

Daniel Cunha
Em seg, 29 de out de 2018 às 14:01, Jonathan Gallimore <
[hidden email]> 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 <[hidden email]>
> 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?


> > 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.

And to be honest, I didn't understand what is
openejb.classloader.first.disallow-system-load and why do we have it. :)


> > 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 <[hidden email]> a
> > écrit :
> >
> > > Hi,
> > >
> > > I updated the PR with tests!
> > >
> > > Em seg, 29 de out de 2018 às 08:27, Jonathan Gallimore <
> > > [hidden email]> 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 <[hidden email]
> >
> > > > 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_
Reply | Threaded
Open this post in threaded view
|

Re: TOMEE-2253 - tomee.sh -version not working properly with Java 11

Romain Manni-Bucau
Le mer. 31 oct. 2018 à 14:52, Daniel Cunha <[hidden email]> a écrit :

> Em seg, 29 de out de 2018 às 14:01, Jonathan Gallimore <
> [hidden email]> 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 <
> [hidden email]>
> > 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 <[hidden email]> a
> > > écrit :
> > >
> > > > Hi,
> > > >
> > > > I updated the PR with tests!
> > > >
> > > > Em seg, 29 de out de 2018 às 08:27, Jonathan Gallimore <
> > > > [hidden email]> 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 <
> [hidden email]
> > >
> > > > > 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_
>
Reply | Threaded
Open this post in threaded view
|

Re: TOMEE-2253 - tomee.sh -version not working properly with Java 11

jgallimore
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 <[hidden email]>
wrote:

> Le mer. 31 oct. 2018 à 14:52, Daniel Cunha <[hidden email]> a écrit
> :
>
> > Em seg, 29 de out de 2018 às 14:01, Jonathan Gallimore <
> > [hidden email]> 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 <
> > [hidden email]>
> > > 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 <[hidden email]> a
> > > > écrit :
> > > >
> > > > > Hi,
> > > > >
> > > > > I updated the PR with tests!
> > > > >
> > > > > Em seg, 29 de out de 2018 às 08:27, Jonathan Gallimore <
> > > > > [hidden email]> 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 <
> > [hidden email]
> > > >
> > > > > > 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_
> >
>