[8u] PING? RFR(s): 8221342: [TESTBUG] Generate Dockerfile for docker testing
Hohensee, Paul
hohensee at amazon.com
Mon Oct 12 21:24:17 UTC 2020
Haven't run the test, but comparison with the JDK 13 patch looks fine to me.
Paul
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
More information about the jdk8u-dev
mailing list