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

[Severin Gehwolf]

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?

Thanks,
Severin