date:20190717


Sounds good,

Thank you,

Misha

On 7/17/19 6:45 PM, Igor Ignatev wrote:
We definitely should do it as a separate RFE, I meant to write it in 
my email, but was interrupted by a fire drill, and forgot about it 
when returned.


— Igor

On Jul 17, 2019, at 6:38 PM, mikhailo.seledt...@oracle.com 
 wrote:


However, I would recommend to do this work as part of a new RFE. If 
we agree, I will create an RFE, and we can continue discussion in the 
context of that RFE.

Re: RFR: 8227642: [TESTBUG] Make docker tests podman compatible

2019-07-17 Thread Igor Ignatev

We definitely should do it as a separate RFE, I meant to write it in my email, 
but was interrupted by a fire drill, and forgot about it when returned. 

— Igor

> On Jul 17, 2019, at 6:38 PM, mikhailo.seledt...@oracle.com wrote:
> 
> However, I would recommend to do this work as part of a new RFE. If we agree, 
> I will create an RFE, and we can continue discussion in the context of that 
> RFE.
>

Re: RFR: 8227642: [TESTBUG] Make docker tests podman compatible




On 7/17/19 11:37 AM, Igor Ignatyev wrote:


Hi Severin,

the updated webrev looks good to me, please see a couple comments below.

Cheers,
-- Igor

On Jul 17, 2019, at 10:34 AM, Severin Gehwolf > 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?


Now that more container technologies are coming online we could consider 
more generic names for these properties/variables.


Here are some thoughts:

  - container.support (CONTAINER_COMMAND) - may be too generic

  - linux.container.support (LINUX_CONTAINER_COMMAND) - more narrow

  - even more narrow/specific: oci.container.support 
(OCI_CONTAINER_COMMAND)


 OCI in this case is " Open Container Initiative", ( Linux 
Foundation project to design open standards for Linux Container technology)


 I believe both Docker and Podman are OCI compliant.

However, I would recommend to do this work as part of a new RFE. If we 
agree, I will create an RFE, and we can continue discussion in the 
context of that RFE.



Thank you,

Misha




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.





Testing: Container tests with docker daemon running on Linux x86_64,
container tests without docker daemon running (podman is daemon-less)
via the podman binary on Linux x86_64 (with -e:PATH). All pass.


Sounds good.


Overall looks good to me.


Thanks for the review!


One minor nit: DockerTestUtils.java does not need "import
java.util.Map;" (no need to post updated webrev for this change)


OK, good catch. Fixed locally.

Thanks,
Severin



Thank you,

Misha


More thoughts?

Thanks,
Severin


Thanks,
-- Igor

On Jul 16, 2019, at 5:36 AM, Severin Gehwolf > wrote:


Hi,

I believe I still need a *R*eviewer for this. Any takers?

Thanks,
Severin

On Fri, 2019-07-12 at 15:19 -0700, mikhailo.seledt...@oracle.com 
 wrote:

Hi Severin,

  The change looks good to me. Thank you for adding support for 
Podman

container technology.

Testing: I ran both HotSpot and JDK container tests with your patch;
tests executed on Oracle Linux 7.6 using default container 
engine (Docker):


test/hotspot/jtreg/containers/   AND
test/jdk/jdk/internal/platform/docker/

All PASS


Thanks,

Misha


On 7/12/19 11:08 AM, Severin Gehwolf wrote:

Hi,

There is an alternative container engine which is being used by 
Fedora
and RHEL 8, called podman[1]. It's mostly compatible with 
docker. It
looks like OpenJDK docker tests can be made podman compatible 
with a
few little tweaks. One "interesting" one is to not assert 
"Successfully
built" in the build output but only rely on the exit code, 
which seems

to be OK for my testing. Interestingly the test would be skipped in
that case.

Bug: https://bugs.openjdk.java.net/browse/JDK-8227642
webrev: 
http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8227642/01/webrev/


Adjustments I've done:
 * Don't assert "Successfully built" in image build output[2].
 * Add

Re: 8228338: tools/pack200/TimeStamp.java fails with NullPointerException

A test job I launched before the CI tests just finished now and it looks like 
this catches all the failures but let’s wait until the CI is done to be certain.

Thanks,

Brian

> On Jul 17, 2019, at 5:09 PM, Lance Andersen  wrote:
> 
> All good Brian, I agree and the latest update also looks OK
> 
> 
>> On Jul 17, 2019, at 8:04 PM, Brian Burkhalter > > wrote:
>> 
>> Sorry Lance but there was another failure which I’ve lumped into the same 
>> issue and updated the webrev in place. I think it would be better to wait 
>> until tiers 1-3 are done before looking at it to avoid more spinning.

Re: 8228338: tools/pack200/TimeStamp.java fails with NullPointerException

All good Brian, I agree and the latest update also looks OK


> On Jul 17, 2019, at 8:04 PM, Brian Burkhalter  
> wrote:
> 
> Sorry Lance but there was another failure which I’ve lumped into the same 
> issue and updated the webrev in place. I think it would be better to wait 
> until tiers 1-3 are done before looking at it to avoid more spinning.
> 
> Thanks,
> 
> Brian
> 
>> On Jul 17, 2019, at 4:17 PM, Lance Andersen > > wrote:
>> 
>> Looks OK Brian
>>> On Jul 17, 2019, at 7:14 PM, Brian Burkhalter >> > wrote:
>>> 
>>> https://bugs.openjdk.java.net/browse/JDK-8228338 
>>> 
>>> http://cr.openjdk.java.net/~bpb/8228338/webrev.00/ 
>>> 
>>> 
>>> Another slip up due to not having tested the JDK-8067801 patch sufficiently.
>>> 
>>> Thanks,
>>> 
>>> Brian
>> 
>>  
>> 
>>   
>> 
>>  Lance Andersen| 
>> Principal Member of Technical Staff | +1.781.442.2037
>> Oracle Java Engineering 
>> 1 Network Drive 
>> Burlington, MA 01803
>> lance.ander...@oracle.com 
>> 
>> 
>> 
> 

 
  

 Lance Andersen| 
Principal Member of Technical Staff | +1.781.442.2037
Oracle Java Engineering 
1 Network Drive 
Burlington, MA 01803
lance.ander...@oracle.com

Re: 8228338: tools/pack200/TimeStamp.java fails with NullPointerException

Sorry Lance but there was another failure which I’ve lumped into the same issue 
and updated the webrev in place. I think it would be better to wait until tiers 
1-3 are done before looking at it to avoid more spinning.

Thanks,

Brian

> On Jul 17, 2019, at 4:17 PM, Lance Andersen  wrote:
> 
> Looks OK Brian
>> On Jul 17, 2019, at 7:14 PM, Brian Burkhalter > > wrote:
>> 
>> https://bugs.openjdk.java.net/browse/JDK-8228338 
>> 
>> http://cr.openjdk.java.net/~bpb/8228338/webrev.00/
>> 
>> Another slip up due to not having tested the JDK-8067801 patch sufficiently.
>> 
>> Thanks,
>> 
>> Brian
> 
>  
> 
>   
> 
>  Lance Andersen| 
> Principal Member of Technical Staff | +1.781.442.2037
> Oracle Java Engineering 
> 1 Network Drive 
> Burlington, MA 01803
> lance.ander...@oracle.com 
> 
> 
>

Re: 8228338: tools/pack200/TimeStamp.java fails with NullPointerException

Looks OK Brian
> On Jul 17, 2019, at 7:14 PM, Brian Burkhalter  
> wrote:
> 
> https://bugs.openjdk.java.net/browse/JDK-8228338
> http://cr.openjdk.java.net/~bpb/8228338/webrev.00/
> 
> Another slip up due to not having tested the JDK-8067801 patch sufficiently.
> 
> Thanks,
> 
> Brian

 
  

 Lance Andersen| 
Principal Member of Technical Staff | +1.781.442.2037
Oracle Java Engineering 
1 Network Drive 
Burlington, MA 01803
lance.ander...@oracle.com

8228338: tools/pack200/TimeStamp.java fails with NullPointerException

https://bugs.openjdk.java.net/browse/JDK-8228338
http://cr.openjdk.java.net/~bpb/8228338/webrev.00/

Another slip up due to not having tested the JDK-8067801 patch sufficiently.

Thanks,

Brian

Re: 8228204: Fix for JDK-8067801 breaks java/io/NegativeInitSize.java

Hi Lance,

> On Jul 17, 2019, at 3:10 PM, Lance Andersen  wrote:
> 
> Looks OK,  please add noreg-self label or equivalent to the issue

Done and removed bug number from @bug tag. Pushed.

Sorry for the noise.

Thanks,

Brian

Re: 8228204: Fix for JDK-8067801 breaks java/io/NegativeInitSize.java

Hi Brian,

Looks OK,  please add noreg-self label or equivalent to the issue


> On Jul 17, 2019, at 5:43 PM, Brian Burkhalter  
> wrote:
> 
> https://bugs.openjdk.java.net/browse/JDK-8228204
> 
> I lamely did not catch this before the push of the patch for [1]. The diff is 
> below.
> 
> Thanks,
> 
> Brian
> 
> [1] https://bugs.openjdk.java.net/browse/JDK-8067801
> [2] diff
> 
> @@ -22,7 +22,7 @@
>  */
> 
> /* @test
> -   @bug 4015701 4127654
> +   @bug 4015701 4127654 8067801
>@summary Test if the constructor would detect
> illegal arguments.
> */
> @@ -49,8 +49,10 @@
> ("PushbackReader failed to detect negative init size");
> }
> 
> +byte[] ba = { 123 };
> +ByteArrayInputStream goodbis = new ByteArrayInputStream(ba);
> try {
> -PushbackInputStream pbis = new PushbackInputStream(null, -1);
> +PushbackInputStream pbis = new PushbackInputStream(goodbis, -1);
> } catch (IllegalArgumentException e) {
> } catch (Exception e) {
> throw new Exception
> @@ -66,8 +68,6 @@
> ("BufferedOutputStream failed to detect negative init size");
> }
> 
> -byte[] ba = { 123 };
> -ByteArrayInputStream goodbis = new ByteArrayInputStream(ba);
> try {
> BufferedInputStream bis = new BufferedInputStream(goodbis, -1);
> } catch (IllegalArgumentException e) {
> 

 
  

 Lance Andersen| 
Principal Member of Technical Staff | +1.781.442.2037
Oracle Java Engineering 
1 Network Drive 
Burlington, MA 01803
lance.ander...@oracle.com

Re: RFR: 8227587: Add internal privileged System.loadLibrary

2019-07-17 Thread Mandy Chung





On 7/17/19 3:25 AM, Claes Redestad wrote:

Hi Peter,

On 2019-07-17 10:29, Peter Levart wrote:

Hi Claes,

Am I reading correct that package-private 
ClassLoader.loadLibrary(Class fromClass, String name, boolean 
isAbsolute) wouldn't need to consult SecurityManager wasn't it for 
accessing system properties in private 
ClassLoader.initializePath(String propName). So perhaps if the two 
calls to initializePath() in the if block of 
ClassLoader.loadLibrary() are wrapped with a single doPrivileged(), 
you wouldn't have to wrap the call to JavaLangAccess.loadLibrary in 
BootLoader. And since initializePath() would only be called once and 
then the results cached, you end up with less doPrivileged() 
wrappings when SecurityManager is present as soon as 
BootLoader.loadLibrary is called the 2nd time.


hmm, fromClass.getClassLoader() also does a security check, though.



Class::getClassLoader is caller-sensitive and the call to 
fromClass.getClassLoader() does not trigger any security permission 
check as it's called from NativeLibrary who is loaded by the boot loader.



That said, there's a case for wrapping the initializePath calls in a
doPriv of their own, since we now have an implicit requirement that some
system library is loaded first (so that sys_paths is initialized) before
any user code attempts to load a library of their own...



I created https://bugs.openjdk.java.net/browse/JDK-8228336 to track the 
idea to refactor the system native library loading to BootLoader.   I 
also have JDK-8228336 capturing Peter's observation.




BTW, there is a data race when accessing usr_paths and sys_paths 
static fields in multi-threaded scenario. Their type is String[] 
which means that the contents of the arrays could be observed 
uninitialized (elements being null) in some thread. Both fields 
should be made volatile (not only sys_paths) as assignment to them 
can be performed more than once in idempotent racy initialization.


.. and we typically seem to be loading at least one library before
initializing any user code, which is probably why this has never been an
issue. It seems like a good idea to remove this subtle fragility,
though. I wonder if we shouldn't "simply" initialize the two paths
during bootstrap and put them in final fields.



JDK-8228336 tracks this.

Mandy

8228204: Fix for JDK-8067801 breaks java/io/NegativeInitSize.java

https://bugs.openjdk.java.net/browse/JDK-8228204

I lamely did not catch this before the push of the patch for [1]. The diff is 
below.

Thanks,

Brian

[1] https://bugs.openjdk.java.net/browse/JDK-8067801
[2] diff

@@ -22,7 +22,7 @@
  */
 
 /* @test
-   @bug 4015701 4127654
+   @bug 4015701 4127654 8067801
@summary Test if the constructor would detect
 illegal arguments.
 */
@@ -49,8 +49,10 @@
 ("PushbackReader failed to detect negative init size");
 }
 
+byte[] ba = { 123 };
+ByteArrayInputStream goodbis = new ByteArrayInputStream(ba);
 try {
-PushbackInputStream pbis = new PushbackInputStream(null, -1);
+PushbackInputStream pbis = new PushbackInputStream(goodbis, -1);
 } catch (IllegalArgumentException e) {
 } catch (Exception e) {
 throw new Exception
@@ -66,8 +68,6 @@
 ("BufferedOutputStream failed to detect negative init size");
 }
 
-byte[] ba = { 123 };
-ByteArrayInputStream goodbis = new ByteArrayInputStream(ba);
 try {
 BufferedInputStream bis = new BufferedInputStream(goodbis, -1);
 } catch (IllegalArgumentException e) {

Re: 8067801: Enforce null check for underlying I/O streams

Thanks, Lance!

> On Jul 17, 2019, at 2:19 PM, Lance Andersen  wrote:
> 
> Looks good.  Maybe consider expanding to some of the other classes when you 
> have cycles.  But in the meantime, ship it!

Re: RFR (S): JDK-8222778: Packaging Tool (JEP 343) on Linux/AArch64

2019-07-17 Thread Dmitry Chuyko


Hi Andrew,

On 7/17/19 11:52 AM, Andrew Haley wrote:

On 7/16/19 8:55 PM, Dmitry Chuyko wrote:

Hello,

Please review a small patch that mostly fixes jpackage test for Linux
aarch64 and also arm,x86,power. It is prepared for 'JDK-8200758-branch'
branch of open 'sandbox' repo.

There are few parts:

1. LinuxPlatform.cpp and IniFile.cpp got small fixes for compiler warnings.

This is weird:

--- old/src/jdk.jpackage/linux/native/libapplauncher/LinuxPlatform.cpp  
2019-07-16 22:11:30.200258978 +0300
+++ new/src/jdk.jpackage/linux/native/libapplauncher/LinuxPlatform.cpp  
2019-07-16 22:11:29.867374851 +0300
@@ -127,7 +127,7 @@
  }

  void LinuxPlatform::SetCurrentDirectory(TString Value) {
-chdir(PlatformString(Value).toPlatformString());
+int ignored = chdir(PlatformString(Value).toPlatformString());
  }

Firstly it does not fix the problem: you've gone from an unused value to an
unused variable. Secondly, we ignore the return value of printf all the time;
what's different about this one?


chdir() is __wur, which is /usr/include/aarch64-linux-gnu/sys/cdefs.h:#  
define __wur __attribute_warn_unused_result__


In some places this warning is disabled (CoreLibraries.gmk, 
Awt2dLibraries.gmk).


It would be more correct to handle the result here as chdir might not 
succeeded in fact.




This:

@@ -212,9 +212,11 @@
  } else if (count == 0) {
  // break;
  } else {
+/*
  if (buffer[count - 1] == EOF) {
  buffer[count - 1] = '\0';
  }
+*/

I'm wondering if this bogus code might have been inherited from Windows.

Yes, something not-posix in ...-posix file. Removed in webrev.01.



2. LinuxDebBundler.getArch() now maps only x86_64 to amd64, x86 is still
mapped to i386, and other archs map to themselves.

3. In tests, new method getRpmArch() was added to linux/base/Base.java,
it maps JVMs os.arch to default rpmbuild arch. Multiple tests were
modified to use that method instead of "x86_64" in rpm file name. Some
timeouts were increased.

+if ("arm".equals(arch))
+return "armv7hl";

Is this actually correct? How do you know it's not, say, armv7hf? I would
have thought there must be some better way to get the RPM arch. Isn't
running /bin/arch right on your systems?


Using $(arch) is an interesting question. For now implementation decides 
what kind of bundle to create looking at JVM binary arch. This should 
guarantee that the bundle will fit the same arch of host and some 
compatible architectures (app will be be installable and runnable). And 
it also mean current tests may fail if host and VM archs are different 
(because packages won't be found while may be created and may be working).


'arvmv7hl' is a common build environment/target for arm port and looks 
like a compatible option. We may look into VM binary metadata though to 
get exact build arch.


-Dmitry

Re: 8067801: Enforce null check for underlying I/O streams

Hi Brian
> On Jul 17, 2019, at 5:08 PM, Brian Burkhalter  
> wrote:
> 
> Hi Lance,
> 
>> On Jul 17, 2019, at 1:55 PM, Lance Andersen > > wrote:
>> 
>> I guess the next question(here he goes again ;-)  is do you want to test all 
>> of the constructors such as BufferedInputStream(InputStream, int) or just 
>> one per class?  I realize the BufferedInputStream(InputStream) just calls 
>> into BufferedInputStream(InputStream, int).
> 
> Might as well add them in: http://cr.openjdk.java.net/~bpb/8067801/webrev.02/ 
> 
>> Sorry for  the continual questions and extra work 
> 
> No worries: it’s good to have things pointed out!

Looks good.  Maybe consider expanding to some of the other classes when you 
have cycles.  But in the meantime, ship it!
> 
> Thanks,
> 
> Brian

 
  

 Lance Andersen| 
Principal Member of Technical Staff | +1.781.442.2037
Oracle Java Engineering 
1 Network Drive 
Burlington, MA 01803
lance.ander...@oracle.com

Re: 8067801: Enforce null check for underlying I/O streams

Hi Lance,

> On Jul 17, 2019, at 1:55 PM, Lance Andersen  wrote:
> 
> I guess the next question(here he goes again ;-)  is do you want to test all 
> of the constructors such as BufferedInputStream(InputStream, int) or just one 
> per class?  I realize the BufferedInputStream(InputStream) just calls into 
> BufferedInputStream(InputStream, int).

Might as well add them in: http://cr.openjdk.java.net/~bpb/8067801/webrev.02/

> Sorry for  the continual questions and extra work 

No worries: it’s good to have things pointed out!

Thanks,

Brian

Re: RFR (S): JDK-8222778: Packaging Tool (JEP 343) on Linux/AArch64

2019-07-17 Thread Alexander Matveev


Hi Dmitry,

Updated webrev looks fine.

Thanks,
Alexander

On 7/17/2019 1:44 PM, Dmitry Chuyko wrote:
Ok, here is the updated webrev: 
http://cr.openjdk.java.net/~dchuyko/8222778/webrev.01/


-Dmitry

On 7/17/19 11:03 PM, Alexander Matveev wrote:

Hi Dmitry,

I also do not see point of keeping this block, so lets remove it 
instead of keeping it commented.


Thanks,
Alexander

On 7/16/2019 3:50 PM, Dmitry Chuyko wrote:

Alexander, thanks for having a look,

On 7/17/19 12:30 AM, Alexander Matveev wrote:

Hi Dmitry,

http://cr.openjdk.java.net/~dchuyko/8222778/webrev.00/src/jdk.jpackage/unix/native/libapplauncher/PosixPlatform.cpp.frames.html 

Why code between lines 215 and 219 was disabled? Not sure what it 
tries to do, if it tries to guarantee NULL termination we should 
probably keep it or allocate buffer with extra null or read 
(sizeof(buffer)-1). I think EOF defined as -1.


gcc 5.4.0 on Linux reports an error:

sandbox/src/jdk.jpackage/unix/native/libapplauncher/PosixPlatform.cpp: 
In member function ‘bool PosixProcess::ReadOutput()’:


sandbox/src/jdk.jpackage/unix/native/libapplauncher/PosixPlatform.cpp:215:35: 
error: comparison is always false due to limited range of data type 
[-Werror=type-limits]

 if (buffer[count - 1] == EOF) {

Here buffer is char[] and read(int, void *, size_t) is used.

It won't be right to keep it commented, to me it looks like this 
block can be removed. If not, there should be some other fix like 
adding ifdefs for some platforms or using call different from read().


-Dmitry


Otherwise looks fine.

Thanks,
Alexander

On 7/16/2019 12:55 PM, Dmitry Chuyko wrote:

Hello,

Please review a small patch that mostly fixes jpackage test for 
Linux aarch64 and also arm,x86,power. It is prepared for 
'JDK-8200758-branch' branch of open 'sandbox' repo.


There are few parts:

1. LinuxPlatform.cpp and IniFile.cpp got small fixes for compiler 
warnings.


2. LinuxDebBundler.getArch() now maps only x86_64 to amd64, x86 is 
still mapped to i386, and other archs map to themselves.


3. In tests, new method getRpmArch() was added to 
linux/base/Base.java, it maps JVMs os.arch to default rpmbuild 
arch. Multiple tests were modified to use that method instead of 
"x86_64" in rpm file name. Some timeouts were increased.


bug: https://bugs.openjdk.java.net/browse/JDK-8222778

webrev: http://cr.openjdk.java.net/~dchuyko/8222778/webrev.00/

testing: test/jdk/tools/jpackage jtreg tests pass on Ubuntu 16.04 
with rpmbuild on x86_64, aarch64, arm, x86 and power, except 
deb/MaintainerTest (fails everywhere similarly to x86_64 because 
of extra "Unknown" name in email).


I didin't cover s390 as we in BellSoft currently don't build on 
that arch. On typical armv7 hw increased or default timeouts are 
still too low, while they are fine for some relatively weak 
aarch64 machines. Deb tests run especially slow because of 
dpkg-deb itself. I used "force-unsafe-io" option in 
/etc/dpkg/dpkg.cfg, it does reduce packaging time but still not 
enough to have really fast tests.


-Dmitry

Re: 8067801: Enforce null check for underlying I/O streams

Hi Brian
> On Jul 17, 2019, at 4:39 PM, Brian Burkhalter  
> wrote:
> 
> Hi Lance,
> 
>> On Jul 17, 2019, at 1:02 PM, Lance Andersen > > wrote:
>> 
>> I think what you have is fine, a unique test in each subdirectory as it is 
>> organized by class.  What you might consider is having a generic NPE Test 
>> class, which validates the error for Constructors and methods as described 
>> in the package javadoc.  Maybe this is a TBD, but if you decide that is a 
>> good idea, consider renaming the test now to maybe NPETests.java
> 
> I merged the two tests into one and converted to TestNG [1]. There is 
> precedent for having a test which deals with multiple classes in 
> test/jdk/java/io [2].

Thanks for the pointer. 

Looks fine and You are good to go…..

 I guess the next question(here he goes again ;-)  is do you want to test all 
of the constructors such as BufferedInputStream(InputStream, int) or just one 
per class?  I realize the BufferedInputStream(InputStream) just calls into 
BufferedInputStream(InputStream, int).

Sorry for  the continual questions and extra work 


Best
Lance


> 
> Thanks,
> 
> Brian
> 
> [1] http://cr.openjdk.java.net/~bpb/8067801/webrev.01/ 
> 
> [2] test/jdk/java/io/NegativeInitSize.java

 
  

 Lance Andersen| 
Principal Member of Technical Staff | +1.781.442.2037
Oracle Java Engineering 
1 Network Drive 
Burlington, MA 01803
lance.ander...@oracle.com

Re: RFR (S): JDK-8222778: Packaging Tool (JEP 343) on Linux/AArch64

2019-07-17 Thread Dmitry Chuyko

Ok, here is the updated webrev:
http://cr.openjdk.java.net/~dchuyko/8222778/webrev.01/

-Dmitry

On 7/17/19 11:03 PM, Alexander Matveev wrote:

Hi Dmitry,

I also do not see point of keeping this block, so lets remove it
instead of keeping it commented.

Thanks,
Alexander

On 7/16/2019 3:50 PM, Dmitry Chuyko wrote:

Alexander, thanks for having a look,

On 7/17/19 12:30 AM, Alexander Matveev wrote:

Hi Dmitry,

http://cr.openjdk.java.net/~dchuyko/8222778/webrev.00/src/jdk.jpackage/unix/native/libapplauncher/PosixPlatform.cpp.frames.html

Why code between lines 215 and 219 was disabled? Not sure what it
tries to do, if it tries to guarantee NULL termination we should
probably keep it or allocate buffer with extra null or read
(sizeof(buffer)-1). I think EOF defined as -1.

gcc 5.4.0 on Linux reports an error:

sandbox/src/jdk.jpackage/unix/native/libapplauncher/PosixPlatform.cpp:
In member function ‘bool PosixProcess::ReadOutput()’:

sandbox/src/jdk.jpackage/unix/native/libapplauncher/PosixPlatform.cpp:215:35:
error: comparison is always false due to limited range of data type
[-Werror=type-limits]

if (buffer[count - 1] == EOF) {

Here buffer is char[] and read(int, void *, size_t) is used.

It won't be right to keep it commented, to me it looks like this
block can be removed. If not, there should be some other fix like
adding ifdefs for some platforms or using call different from read().

-Dmitry

Otherwise looks fine.

Thanks,
Alexander

On 7/16/2019 12:55 PM, Dmitry Chuyko wrote:

Hello,

Please review a small patch that mostly fixes jpackage test for
Linux aarch64 and also arm,x86,power. It is prepared for
'JDK-8200758-branch' branch of open 'sandbox' repo.

There are few parts:

1. LinuxPlatform.cpp and IniFile.cpp got small fixes for compiler
warnings.

2. LinuxDebBundler.getArch() now maps only x86_64 to amd64, x86 is
still mapped to i386, and other archs map to themselves.

3. In tests, new method getRpmArch() was added to
linux/base/Base.java, it maps JVMs os.arch to default rpmbuild
arch. Multiple tests were modified to use that method instead of
"x86_64" in rpm file name. Some timeouts were increased.

bug: https://bugs.openjdk.java.net/browse/JDK-8222778

webrev: http://cr.openjdk.java.net/~dchuyko/8222778/webrev.00/

testing: test/jdk/tools/jpackage jtreg tests pass on Ubuntu 16.04
with rpmbuild on x86_64, aarch64, arm, x86 and power, except
deb/MaintainerTest (fails everywhere similarly to x86_64 because of
extra "Unknown" name in email).

I didin't cover s390 as we in BellSoft currently don't build on
that arch. On typical armv7 hw increased or default timeouts are
still too low, while they are fine for some relatively weak aarch64
machines. Deb tests run especially slow because of dpkg-deb itself.
I used "force-unsafe-io" option in /etc/dpkg/dpkg.cfg, it does
reduce packaging time but still not enough to have really fast tests.

-Dmitry

Re: 8067801: Enforce null check for underlying I/O streams

Hi Lance,

> On Jul 17, 2019, at 1:02 PM, Lance Andersen  wrote:
> 
> I think what you have is fine, a unique test in each subdirectory as it is 
> organized by class.  What you might consider is having a generic NPE Test 
> class, which validates the error for Constructors and methods as described in 
> the package javadoc.  Maybe this is a TBD, but if you decide that is a good 
> idea, consider renaming the test now to maybe NPETests.java

I merged the two tests into one and converted to TestNG [1]. There is precedent 
for having a test which deals with multiple classes in test/jdk/java/io [2].

Thanks,

Brian

[1] http://cr.openjdk.java.net/~bpb/8067801/webrev.01/
[2] test/jdk/java/io/NegativeInitSize.java

Re: RFR (S): JDK-8222778: Packaging Tool (JEP 343) on Linux/AArch64

2019-07-17 Thread Alexander Matveev

Hi Dmitry,

I also do not see point of keeping this block, so lets remove it instead
of keeping it commented.

Thanks,
Alexander

On 7/16/2019 3:50 PM, Dmitry Chuyko wrote:

Alexander, thanks for having a look,

On 7/17/19 12:30 AM, Alexander Matveev wrote:

Hi Dmitry,

http://cr.openjdk.java.net/~dchuyko/8222778/webrev.00/src/jdk.jpackage/unix/native/libapplauncher/PosixPlatform.cpp.frames.html

gcc 5.4.0 on Linux reports an error:

sandbox/src/jdk.jpackage/unix/native/libapplauncher/PosixPlatform.cpp:
In member function ‘bool PosixProcess::ReadOutput()’:

sandbox/src/jdk.jpackage/unix/native/libapplauncher/PosixPlatform.cpp:215:35:
error: comparison is always false due to limited range of data type
[-Werror=type-limits]

if (buffer[count - 1] == EOF) {

Here buffer is char[] and read(int, void *, size_t) is used.

It won't be right to keep it commented, to me it looks like this block
can be removed. If not, there should be some other fix like adding
ifdefs for some platforms or using call different from read().

-Dmitry

Otherwise looks fine.

Thanks,
Alexander

On 7/16/2019 12:55 PM, Dmitry Chuyko wrote:

Hello,

Please review a small patch that mostly fixes jpackage test for
Linux aarch64 and also arm,x86,power. It is prepared for
'JDK-8200758-branch' branch of open 'sandbox' repo.

There are few parts:

1. LinuxPlatform.cpp and IniFile.cpp got small fixes for compiler
warnings.

2. LinuxDebBundler.getArch() now maps only x86_64 to amd64, x86 is
still mapped to i386, and other archs map to themselves.

3. In tests, new method getRpmArch() was added to
linux/base/Base.java, it maps JVMs os.arch to default rpmbuild arch.
Multiple tests were modified to use that method instead of "x86_64"
in rpm file name. Some timeouts were increased.

bug: https://bugs.openjdk.java.net/browse/JDK-8222778

webrev: http://cr.openjdk.java.net/~dchuyko/8222778/webrev.00/

I didin't cover s390 as we in BellSoft currently don't build on that
arch. On typical armv7 hw increased or default timeouts are still
too low, while they are fine for some relatively weak aarch64
machines. Deb tests run especially slow because of dpkg-deb itself.
I used "force-unsafe-io" option in /etc/dpkg/dpkg.cfg, it does
reduce packaging time but still not enough to have really fast tests.

-Dmitry

Re: 8067801: Enforce null check for underlying I/O streams

Hi Brian,
> On Jul 17, 2019, at 3:46 PM, Brian Burkhalter  
> wrote:
> 
> Hi Lance,
> 
>> On Jul 17, 2019, at 12:35 PM, Brian Burkhalter > > wrote:
>> 
>>> I might consider adding the NPE message to the individual classes as it is 
>>> easy to miss in the java.io  >> > package but that is just a suggestion.  Especially with 
>>> the new search feature of javadoc, I usually go straight to the method so 
>>> would never see the info in the package comments.
>> 
>> It might be better but I don’t know where else in java.io  
>> an NPE is thrown without being documented on the ctor/method itself. If 
>> there are other places then a general inspection might be in order.
> 
> I looked through some of the other classes and there are 
> not-locally-documented NPEs in a number of constructors including all the 
> Readers and Writers. I think for now I’d rather leave it as is.

Understand, go with what makes sense,  BTW, I was not suggesting adding it to 
the individual constructor javadoc, but to the Class overview javadoc at the 
top of the class.

Totally get leaving it as is. :-)
> 
>> Note that the practice of using the package-level doc occurs also in 
>> java.nio:
>> 
>> "Unless otherwise noted, passing a null argument to a constructor or method 
>> in any class or interface in this package will cause a NullPointerException 
>> to be thrown."
>> 
>>> Did you give any thought of using TestNG/assertThrows?  Not a big deal, I 
>>> just a personal preference :-)
>> 
>> I don’t see how that would work in this case as we do *not* expect an 
>> exception.
> 
> Sorry: I had it backwards - d’oh!

:-)  

Blame me as I could have been clearer in my comment, apologizes for that :-)

> You are correct, that would work. I’ll look into revising it.
> 
> What I was actually wondering about with respect to the tests is whether one 
> can have the same bug ID in two different tests.

Yes that should be OK as @bug is just an informational tag: 
https://openjdk.java.net/jtreg/tag-spec.html

> Also I was not sure whether there might be a location where the two merged 
> into a single test would be appropriate.

I think what you have is fine, a unique test in each subdirectory as it is 
organized by class.  What you might consider is having a generic NPE Test 
class, which validates the error for Constructors and methods as described in 
the package javadoc.  Maybe this is a TBD, but if you decide that is a good 
idea, consider renaming the test now to maybe NPETests.java

Either way you are good to go.

Best
Lance
> 
> Thanks,
> 
> Brian

 
  

 Lance Andersen| 
Principal Member of Technical Staff | +1.781.442.2037
Oracle Java Engineering 
1 Network Drive 
Burlington, MA 01803
lance.ander...@oracle.com

Re: 8067801: Enforce null check for underlying I/O streams

Hi Lance,

> On Jul 17, 2019, at 12:35 PM, Brian Burkhalter  
> wrote:
> 
>> I might consider adding the NPE message to the individual classes as it is 
>> easy to miss in the java.io  > > package but that is just a suggestion.  Especially with 
>> the new search feature of javadoc, I usually go straight to the method so 
>> would never see the info in the package comments.
> 
> It might be better but I don’t know where else in java.io  
> an NPE is thrown without being documented on the ctor/method itself. If there 
> are other places then a general inspection might be in order.

I looked through some of the other classes and there are not-locally-documented 
NPEs in a number of constructors including all the Readers and Writers. I think 
for now I’d rather leave it as is.

> Note that the practice of using the package-level doc occurs also in java.nio:
> 
> "Unless otherwise noted, passing a null argument to a constructor or method 
> in any class or interface in this package will cause a NullPointerException 
> to be thrown."
> 
>> Did you give any thought of using TestNG/assertThrows?  Not a big deal, I 
>> just a personal preference :-)
> 
> I don’t see how that would work in this case as we do *not* expect an 
> exception.

Sorry: I had it backwards - d’oh! You are correct, that would work. I’ll look 
into revising it.

What I was actually wondering about with respect to the tests is whether one 
can have the same bug ID in two different tests. Also I was not sure whether 
there might be a location where the two merged into a single test would be 
appropriate.

Thanks,

Brian

Re: 8067801: Enforce null check for underlying I/O streams

Hi Lance,

> On Jul 17, 2019, at 12:28 PM, Lance Andersen  
> wrote:
> 
> I think this looks good. 

Thanks.

> I might consider adding the NPE message to the individual classes as it is 
> easy to miss in the java.io  package but that is just a 
> suggestion.  Especially with the new search feature of javadoc, I usually go 
> straight to the method so would never see the info in the package comments.

It might be better but I don’t know where else in java.io an NPE is thrown 
without being documented on the ctor/method itself. If there are other places 
then a general inspection might be in order.

Note that the practice of using the package-level doc occurs also in java.nio:

"Unless otherwise noted, passing a null argument to a constructor or method in 
any class or interface in this package will cause a NullPointerException to be 
thrown."

> Did you give any thought of using TestNG/assertThrows?  Not a big deal, I 
> just a personal preference :-)

I don’t see how that would work in this case as we do *not* expect an exception.

Thanks,

Brian

Re: 8067801: Enforce null check for underlying I/O streams

Hi Brian,

I think this looks good. 

 I might consider adding the NPE message to the individual classes as it is 
easy to miss in the java.io package but that is just a suggestion.  Especially 
with the new search feature of javadoc, I usually go straight to the method so 
would never see the info in the package comments.

Did you give any thought of using TestNG/assertThrows?  Not a big deal, I just 
a personal preference :-)

HTH

Best
Lance

> On Jul 17, 2019, at 12:12 PM, Brian Burkhalter  
> wrote:
> 
> https://bugs.openjdk.java.net/browse/JDK-8067801
> http://cr.openjdk.java.net/~bpb/8067801/webrev.00/
> 
> Add null check to Filter{In,Out}putStream constructors. This covers all the 
> cases listed in the issue as those are all subclasses of these two classes. 
> No javadoc update is need due to this statement in the java.io package doc:
> 
> "Unless otherwise noted, passing a null argument to a constructor or method 
> in any class or interface in this package will cause a NullPointerException 
> to be thrown."
> 
> Thanks,
> 
> Brian

 Lance Andersen| 
Principal Member of Technical Staff | +1.781.442.2037
Oracle Java Engineering 
1 Network Drive 
Burlington, MA 01803
lance.ander...@oracle.com

Re: RFR: 8227642: [TESTBUG] Make docker tests podman compatible

2019-07-17 Thread Igor Ignatyev



Hi Severin,

the updated webrev looks good to me, please see a couple comments below.

Cheers,
-- Igor

> On Jul 17, 2019, at 10:34 AM, Severin Gehwolf  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?

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

> 
>>> Testing: Container tests with docker daemon running on Linux x86_64,
>>> container tests without docker daemon running (podman is daemon-less)
>>> via the podman binary on Linux x86_64 (with -e:PATH). All pass.
>> 
>> Sounds good.
>> 
>> 
>> Overall looks good to me.
> 
> Thanks for the review!
> 
>> One minor nit: DockerTestUtils.java does not need "import 
>> java.util.Map;" (no need to post updated webrev for this change)
> 
> OK, good catch. Fixed locally.
> 
> Thanks,
> Severin
> 
>> 
>> Thank you,
>> 
>> Misha
>> 
>>> More thoughts?
>>> 
>>> Thanks,
>>> Severin
>>> 
 Thanks,
 -- Igor
 
> On Jul 16, 2019, at 5:36 AM, Severin Gehwolf  wrote:
> 
> Hi,
> 
> I believe I still need a *R*eviewer for this. Any takers?
> 
> Thanks,
> Severin
> 
> On Fri, 2019-07-12 at 15:19 -0700, mikhailo.seledt...@oracle.com wrote:
>> Hi Severin,
>> 
>>   The change looks good to me. Thank you for adding support for Podman
>> container technology.
>> 
>> Testing: I ran both HotSpot and JDK container tests with your patch;
>> tests executed on Oracle Linux 7.6 using default container engine 
>> (Docker):
>> 
>> test/hotspot/jtreg/containers/   AND
>> test/jdk/jdk/internal/platform/docker/
>> 
>> All PASS
>> 
>> 
>> Thanks,
>> 
>> Misha
>> 
>> 
>> On 7/12/19 11:08 AM, Severin Gehwolf wrote:
>>> Hi,
>>> 
>>> There is an alternative container engine which is being used by Fedora
>>> and RHEL 8, called podman[1]. It's mostly compatible with docker. It
>>> looks like OpenJDK docker tests can be made podman compatible with a
>>> few little tweaks. One "interesting" one is to not assert "Successfully
>>> built" in the build output but only rely on the exit code, which seems
>>> to be OK for my testing. Interestingly the test would be skipped in
>>> that case.
>>> 
>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8227642
>>> webrev: 
>>> http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8227642/01/webrev/
>>> 
>>> Adjustments I've done:
>>>  * Don't assert "Successfully built" in image build output[2].
>>>  * Add /usr/sbin to PATH as the podman binary relies on iptables for it
>>>to work which is in /usr/sbin on Fedora
>>>  * Allow for Metrics.getCpuSystemUsage() and Metrics.getCpuUserUsage()
>>>to be equal to the previous value. I've found those counters to be
>>>slowly increasing, which made the tests unreliable.

Re: RFR: 8227642: [TESTBUG] Make docker tests podman compatible




On 7/17/19 10:34 AM, Severin Gehwolf 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.

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.


Thank you for the details. That's OK by me.


Thank you,

Misha




Testing: Container tests with docker daemon running on Linux x86_64,
container tests without docker daemon running (podman is daemon-less)
via the podman binary on Linux x86_64 (with -e:PATH). All pass.

Sounds good.


Overall looks good to me.

Thanks for the review!


One minor nit: DockerTestUtils.java does not need "import
java.util.Map;" (no need to post updated webrev for this change)

OK, good catch. Fixed locally.

Thanks,
Severin


Thank you,

Misha


More thoughts?

Thanks,
Severin


Thanks,
-- Igor


On Jul 16, 2019, at 5:36 AM, Severin Gehwolf  wrote:

Hi,

I believe I still need a *R*eviewer for this. Any takers?

Thanks,
Severin

On Fri, 2019-07-12 at 15:19 -0700, mikhailo.seledt...@oracle.com wrote:

Hi Severin,

The change looks good to me. Thank you for adding support for Podman
container technology.

Testing: I ran both HotSpot and JDK container tests with your patch;
tests executed on Oracle Linux 7.6 using default container engine (Docker):

  test/hotspot/jtreg/containers/   AND
test/jdk/jdk/internal/platform/docker/

All PASS


Thanks,

Misha


On 7/12/19 11:08 AM, Severin Gehwolf wrote:

Hi,

There is an alternative container engine which is being used by Fedora
and RHEL 8, called podman[1]. It's mostly compatible with docker. It
looks like OpenJDK docker tests can be made podman compatible with a
few little tweaks. One "interesting" one is to not assert "Successfully
built" in the build output but only rely on the exit code, which seems
to be OK for my testing. Interestingly the test would be skipped in
that case.

Bug: https://bugs.openjdk.java.net/browse/JDK-8227642
webrev: http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8227642/01/webrev/

Adjustments I've done:
   * Don't assert "Successfully built" in image build output[2].
   * Add /usr/sbin to PATH as the podman binary relies on iptables for it
 to work which is in /usr/sbin on Fedora
   * Allow for Metrics.getCpuSystemUsage() and Metrics.getCpuUserUsage()
 to be equal to the previous value. I've found those counters to be
 slowly increasing, which made the tests unreliable.

Testing:

Running docker tests with docker as engine. Did the same with podman as
engine via -Djdk.test.docker.command=podman on Linux x86_64. Both
passed (non-trivially).

Thoughts?

Thanks,
Severin

[1] https://podman.io/
[2] Image builds with podman look
  like ("COMMIT" over "Successfully built"):
STEP 1: FROM fedora:29
STEP 2: RUN dnf install -y java-11-openjdk-devel && dnf clean all
--> Using cache 96f8b1a0dfe7dba581a64fc67a27002ddf52e032af55f9ddc765182a690afd9d
STEP 3: COPY TestMetrics.class  TestMetrics.java /opt/
269042160f7a4e6a06789cd19640ea658a8f941bc53de0fd40a574dc3bdb49a8
STEP 4: CMD /usr/lib/jvm/java-11-openjdk/bin/java -cp /opt --add-modules 
java.base --add-exports java.base/jdk.internal.platform=ALL-UNNAMED TestMetrics
STEP 5: COMMIT fedora-metrics-11
d749088d6ce4510f212820ad4eca55a9b05e5c5c245f2372b6cfe91926e8cd7e

Re: RFR: 8227642: [TESTBUG] Make docker tests podman compatible

2019-07-17 Thread Severin Gehwolf

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

> > Testing: Container tests with docker daemon running on Linux x86_64,
> > container tests without docker daemon running (podman is daemon-less)
> > via the podman binary on Linux x86_64 (with -e:PATH). All pass.
> 
> Sounds good.
> 
> 
> Overall looks good to me.

Thanks for the review!

> One minor nit: DockerTestUtils.java does not need "import 
> java.util.Map;" (no need to post updated webrev for this change)

OK, good catch. Fixed locally.

Thanks,
Severin

> 
> Thank you,
> 
> Misha
> 
> > More thoughts?
> > 
> > Thanks,
> > Severin
> > 
> > > Thanks,
> > > -- Igor
> > > 
> > > > On Jul 16, 2019, at 5:36 AM, Severin Gehwolf  
> > > > wrote:
> > > > 
> > > > Hi,
> > > > 
> > > > I believe I still need a *R*eviewer for this. Any takers?
> > > > 
> > > > Thanks,
> > > > Severin
> > > > 
> > > > On Fri, 2019-07-12 at 15:19 -0700, mikhailo.seledt...@oracle.com wrote:
> > > > > Hi Severin,
> > > > > 
> > > > >The change looks good to me. Thank you for adding support for 
> > > > > Podman
> > > > > container technology.
> > > > > 
> > > > > Testing: I ran both HotSpot and JDK container tests with your patch;
> > > > > tests executed on Oracle Linux 7.6 using default container engine 
> > > > > (Docker):
> > > > > 
> > > > >  test/hotspot/jtreg/containers/   AND
> > > > > test/jdk/jdk/internal/platform/docker/
> > > > > 
> > > > > All PASS
> > > > > 
> > > > > 
> > > > > Thanks,
> > > > > 
> > > > > Misha
> > > > > 
> > > > > 
> > > > > On 7/12/19 11:08 AM, Severin Gehwolf wrote:
> > > > > > Hi,
> > > > > > 
> > > > > > There is an alternative container engine which is being used by 
> > > > > > Fedora
> > > > > > and RHEL 8, called podman[1]. It's mostly compatible with docker. It
> > > > > > looks like OpenJDK docker tests can be made podman compatible with a
> > > > > > few little tweaks. One "interesting" one is to not assert 
> > > > > > "Successfully
> > > > > > built" in the build output but only rely on the exit code, which 
> > > > > > seems
> > > > > > to be OK for my testing. Interestingly the test would be skipped in
> > > > > > that case.
> > > > > > 
> > > > > > Bug: https://bugs.openjdk.java.net/browse/JDK-8227642
> > > > > > webrev: 
> > > > > > http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8227642/01/webrev/
> > > > > > 
> > > > > > Adjustments I've done:
> > > > > >   * Don't assert "Successfully built" in image build output[2].
> > > > > >   * Add /usr/sbin to PATH as the podman binary relies on iptables 
> > > > > > for it
> > > > > > to work which is in /usr/sbin on Fedora
> > > > > >   * Allow for Metrics.getCpuSystemUsage() and 
> > > > > > Metrics.getCpuUserUsage()
> > > > > > to be equal to the previous value. I've found those counters to 
> > > > > > be
> > > > > > slowly increasing, which made the tests unreliable.
> > > > > > 
> > > > > > Testing:
> > > > > > 
> > > > > > Running docker tests with docker as engine. Did the same with 
> > > > > > podman as
> > > > > > engine via -Djdk.test.docker.command=podman on Linux x86_64. Both
> > > > > > passed (non-trivially).
> > > > > > 
> > > > > > Thoughts?
> > > > > > 
> > > > > > Thanks,
> > > > > > Severin
> > > > > > 
> > > > > > [1] https://podman.io/
> > > > > > [2] Image builds with podman look
> > > > > >  like ("COMMIT" over "Successfully built"):
> > > > > > STEP 1: FROM fedora:29
> > > > > > STEP 2: RUN dnf install -y java-11-openjdk-devel &&

Re: RFR: 8227642: [TESTBUG] Make docker tests podman compatible


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.

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)



Testing: Container tests with docker daemon running on Linux x86_64,
container tests without docker daemon running (podman is daemon-less)
via the podman binary on Linux x86_64 (with -e:PATH). All pass.


Sounds good.


Overall looks good to me.

One minor nit: DockerTestUtils.java does not need "import 
java.util.Map;" (no need to post updated webrev for this change)



Thank you,

Misha



More thoughts?

Thanks,
Severin


Thanks,
-- Igor


On Jul 16, 2019, at 5:36 AM, Severin Gehwolf  wrote:

Hi,

I believe I still need a *R*eviewer for this. Any takers?

Thanks,
Severin

On Fri, 2019-07-12 at 15:19 -0700, mikhailo.seledt...@oracle.com wrote:

Hi Severin,

   The change looks good to me. Thank you for adding support for Podman
container technology.

Testing: I ran both HotSpot and JDK container tests with your patch;
tests executed on Oracle Linux 7.6 using default container engine (Docker):

 test/hotspot/jtreg/containers/   AND
test/jdk/jdk/internal/platform/docker/

All PASS


Thanks,

Misha


On 7/12/19 11:08 AM, Severin Gehwolf wrote:

Hi,

There is an alternative container engine which is being used by Fedora
and RHEL 8, called podman[1]. It's mostly compatible with docker. It
looks like OpenJDK docker tests can be made podman compatible with a
few little tweaks. One "interesting" one is to not assert "Successfully
built" in the build output but only rely on the exit code, which seems
to be OK for my testing. Interestingly the test would be skipped in
that case.

Bug: https://bugs.openjdk.java.net/browse/JDK-8227642
webrev: http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8227642/01/webrev/

Adjustments I've done:
  * Don't assert "Successfully built" in image build output[2].
  * Add /usr/sbin to PATH as the podman binary relies on iptables for it
to work which is in /usr/sbin on Fedora
  * Allow for Metrics.getCpuSystemUsage() and Metrics.getCpuUserUsage()
to be equal to the previous value. I've found those counters to be
slowly increasing, which made the tests unreliable.

Testing:

Running docker tests with docker as engine. Did the same with podman as
engine via -Djdk.test.docker.command=podman on Linux x86_64. Both
passed (non-trivially).

Thoughts?

Thanks,
Severin

[1] https://podman.io/
[2] Image builds with podman look
 like ("COMMIT" over "Successfully built"):
STEP 1: FROM fedora:29
STEP 2: RUN dnf install -y java-11-openjdk-devel && dnf clean all
--> Using cache 96f8b1a0dfe7dba581a64fc67a27002ddf52e032af55f9ddc765182a690afd9d
STEP 3: COPY TestMetrics.class  TestMetrics.java /opt/
269042160f7a4e6a06789cd19640ea658a8f941bc53de0fd40a574dc3bdb49a8
STEP 4: CMD /usr/lib/jvm/java-11-openjdk/bin/java -cp /opt --add-modules 
java.base --add-exports java.base/jdk.internal.platform=ALL-UNNAMED TestMetrics
STEP 5: COMMIT fedora-metrics-11
d749088d6ce4510f212820ad4eca55a9b05e5c5c245f2372b6cfe91926e8cd7e

8067801: Enforce null check for underlying I/O streams