Hi Igor,
On Wed, 2019-07-17 at 11:37 -0700, Igor Ignatyev wrote:
>
> Hi Severin,
>
> the updated webrev looks good to me, please see a couple comments below.
Thanks. More below.
> Cheers,
> -- Igor
>
> > On Jul 17, 2019, at 10:34 AM, Severin Gehwolf <sgehw...@redhat.com> wrote:
> >
> > Hi Misha,
> >
> > On Wed, 2019-07-17 at 10:22 -0700, mikhailo.seledt...@oracle.com wrote:
> > > Hi Severin,
> > >
> > > On 7/17/19 5:44 AM, Severin Gehwolf wrote:
> > > > Hi Igor, Misha,
> > > >
> > > > On Tue, 2019-07-16 at 11:49 -0700, Igor Ignatyev wrote:
> > > > > Hi Severin,
> > > > >
> > > > > I don't think that tests (or test libraries for that matter) should
> > > > > be responsible for setting correct PATH value, it should be a part of
> > > > > host configuration procedure (tests can/should check that all
> > > > > required bins are available though). in other words, I'd prefer if
> > > > > you remove 'env.put("PATH", ...)' lines from both DockerTestUtils and
> > > > > TestJFREvents. the rest looks good to me.
> > > > Updated webrev:
> > > > http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8227642/02/webrev/
> > > >
> > > > No more additions to PATH are being done.
> > > >
> > > > I've discovered that VMProps.java which defines "docker.required", used
> > > > the "docker" binary even for podman test runs. This ended up not
> > > > running most of the tests even with -Djdk.test.docker.command=podman
> > > > specified.
> > > Good catch.
> should we rename docker.support and DOCKER_COMMAND to something more abstract?
container.support and CONTAINER_ENGINE_COMMAND perhaps?
> > > > I've fixed that by moving DOCKER_COMMAND to Platform.java so
> > > > that it can be used in both places.
> > >
> > > Sounds good to me.
> > >
> > > (of course, the alternative would be to import
> > > jdk.test.lib.containers.docker.DockerTestUtils into VMProps.java -- not
> > > sure if there are any potential problems doing it this way)
> >
> > I've tried that but for some reason this was a problem and VMProps
> > failed to compile. I don't know exactly how those jtreg extensions work
> > and went with the Platform approach. Hope that's OK.
>
> all files needed for VMProps (or other @requires expression class)
> have to be listed in requires.extraPropDefns or
> requires.extraPropDefns.bootlibs property in TEST.ROOT file in all
> the test suites which use these extensions. we are trying to be very
> cautious in what is used by VMProps (directly and indirectly) so
> these lists won't grow and we won't require any modules other than
> java.base, given DockerTestUtils has dependencies on a number of
> other library classes, the Platform approach is much better from that
> point of view.
I had a feeling it was something like that. Thanks for the explanation!
Cheers,
Severin
