[8u] PING? RFR(s): 8221342: [TESTBUG] Generate Dockerfile for docker testing

[Severin Gehwolf]

On Mon, 2020-10-12 at 21:24 +0000, Hohensee, Paul wrote:
> Haven't run the test, but comparison with the JDK 13 patch looks fine
> to me.

Thanks for the review, Paul!

Cheers,
Severin

> On 10/9/20, 1:38 PM, "jdk8u-dev on behalf of Severin Gehwolf" <jdk8u-dev-retn at openjdk.java.net on behalf of sgehwolf at redhat.com> wrote:
> 
>     Hi Andrew,
> 
>     On Thu, 2020-09-24 at 11:39 +0200, Severin Gehwolf wrote:
>     > On Mon, 2020-08-31 at 16:35 +0200, Severin Gehwolf wrote:
>     > > Hi Andrew,
>     > >
>     > > On Wed, 2020-08-26 at 04:38 +0100, Andrew Hughes wrote:
>     > > > On 15:05 Tue 25 Aug     , Severin Gehwolf wrote:
>     > > > > Hi!
>     > > > >
>     > > > > Please review this 8u backport of this test bug which enhances
>     > > > > container testing to automatically generate docker files. This is
>     > > > > useful if the development platform is not Oracle Linux 7.X (which is
>     > > > > the case for me).
>     > > > >
>     > > > > The JDK 11 patch applies cleanly (after JDK-8220313 which is in the
>     > > > > approval queue)
>     > > >
>     > > > I don't see how this can be said to apply cleanly. Attempting to
>     > > > shuffle the patch fails to find a mapping for
>     > > > test/lib/jdk/test/lib/containers/docker/DockerTestUtils.java and
>     > > > test/lib/jdk/test/lib/containers/docker/DockerfileConfig.java.
>     > > >
>     > > > It is also beyond the scope of a script to know to duplicate these
>     > > > changes between two repositories. So this alone justifies a review.
>     > >
>     > > OK.
>     > >
>     > > > This is beyond the scope of this patch, but the proposed patch
>     > > > for the JDK tests seems wrong. We seem to have duplication in the
>     > > > JDK test tree, caused by the JFR backport.
>     > >
>     > > Yes, I've noticed it. Question is whether this is worth fixing. Some
>     > > tests rely on test/lib/testlibrary/jdk/testlibrary some others
>     > > on test/lib/jdk/test/lib.
>     > >
>     > > > The jdk/test/lib/testlibrary/jdk/testlibrary/containers directory
>     > > > would be the correct location for these tests. The JFR backport seems
>     > > > to have started an alternate tree at jdk/test/lib/jdk/test/lib with
>     > > > duplication of some files in the former directory. This should be
>     > > > resolved in the 8u282 lifecycle.
>     > >
>     > > I'm not sure I agree but wouldn't object a patch which does this. It
>     > > seems a significant piece of work and would require some re-testing of
>     > > commonly run tests. Either way, this seems beyond the point of this
>     > > patch (as you said).
>     > >
>     > > > > but doesn't compile since Files.writeString() API is
>     > > > > not available in JDK 8. Replaced it with:
>     > > > >
>     > > > > byte[] bytes = dockerFileStr.getBytes(StandardCharsets.UTF_8);
>     > > > > Files.write(dockerfile, bytes);
>     > > > >
>     > > >
>     > > > Could this not be simplified to:
>     > > >
>     > > > Files.write(dockerfile, dockerFileStr.getBytes(StandardCharsets.UTF_8));
>     > > >
>     > > > I don't see the need for the intermediate variable.
>     > >
>     > > Sure. Fixed:
>     > > https://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8221342/jdk8/02/
>     > >
>     > > > Other changes look fine.
>     > >
>     > > Thanks for the review!
>     >
>     > Ping?
> 
>     Ping v2? This is blocking some other docker test fixes which will
>     enable better testing of to-come 8u container backports. Thanks!
> 
>     Cheers,
>     Severin
> 
>