8205054: Could not find "lsof" on test machine
Leo Korinth
leo.korinth at oracle.com
Tue Jun 19 08:13:47 UTC 2018
Thanks David,
Leo
On 19/06/18 01:58, David Holmes wrote:
> Hi Mikael,
>
> On 19/06/2018 9:43 AM, Mikael Vidstedt wrote:
>>
>> Two minor things which can be done as follow-ups if we want to get the
>> fix pushed quickly:
>>
>>
>> + if (isWindows() == false && lsofCommand().isPresent() ==
>> false) {
>>
>> Booleans are not normally compared with explicit values, so for
>> consistency with the other checks in the code I’d suggest changing
>> that to:
>>
>> if (!isWindows() && !lsofCommand().isPresent()) {
>
> Oops - good catch.
>> Also, there is already an isWindows() method in
>> test/lib/jdk/test/lib/Platform.java which would probably be better to
>> (re-)use.
>
> Also good point.
>
>> Apart from that it looks good!
>
> Thanks. We had been awaiting further word from Goetz before I pushed
> this for Leo, but I understand he is, or was, away for a couple of days.
> In the interests of getting the CI testing cleaned up I'll push this
> with your suggested cosmetic changes applied. (Leo I hope you don't mind
> :) )
>
> Thanks,
> David
>
>> Cheers,
>> Mikael
>>
>>
>>> On Jun 15, 2018, at 4:53 AM, Leo Korinth <leo.korinth at oracle.com> wrote:
>>>
>>> Hi David and Goetz.
>>>
>>> I have added the last caching thing David asked for. Are you also
>>> okay with the changes Goetz?
>>>
>>> Webrev:
>>> http://cr.openjdk.java.net/~lkorinth/8205054/02_03/ (incremental)
>>> http://cr.openjdk.java.net/~lkorinth/8205054/03/ (full)
>>>
>>> I also noticed that my test case unfortunately runs in many tiers.
>>> But that is nothing that is special to my test case but is shared
>>> between most test cases that are selected by being under "runtime/".
>>> I do not feel qualified in doing changes there, and maybe there is a
>>> reason for it.
>>>
>>> Thanks,
>>> Leo
>>>
>>> On 15/06/18 11:56, David Holmes wrote:
>>>> Hi Leo,
>>>> On 15/06/2018 7:37 PM, Leo Korinth wrote:
>>>>> Hi David,
>>>>>
>>>>> Thanks for the input, it makes the test case _much_ better.
>>>>> Unfortunately it is easy to get blind when you are in a hurry and
>>>>> responsible for a failing test case. It is of course much better to
>>>>> look for the command at start.
>>>>>
>>>>> New output:
>>>>>
>>>>> STDOUT:
>>>>> Could not find lsof like command
>>>>> Exit test case as successful though it could not verify anything
>>>>> STDERR:
>>>>>
>>>>> JavaTest Message: Test complete.
>>>> Much clearer - thanks!
>>>>> Webrev:
>>>>> http://cr.openjdk.java.net/~lkorinth/8205054/01_02/ (incremental)
>>>>> http://cr.openjdk.java.net/~lkorinth/8205054/02/ (full)
>>>> Generally looks good. Only nit is that the lsofCommand() result
>>>> should be cached in a field so you don't need to recalculate it for
>>>> the actual execution.
>>>> Thanks,
>>>> David
>>>>> Testing:
>>>>> Tested on my GNU/Linux machine and currently running a mach5 test
>>>>> (mostly for verifying the windows part)
>>>>>
>>>>> Thanks,
>>>>> Leo
>>>>>
>>>>>
>>>>> On 15/06/18 10:40, David Holmes wrote:
>>>>>> Hi Leo,
>>>>>>
>>>>>> Inine ...
>>>>>>
>>>>>> On 15/06/2018 6:06 PM, Leo Korinth wrote:
>>>>>>> Hi David.
>>>>>>>
>>>>>>> On 15/06/18 04:08, David Holmes wrote:
>>>>>>>> Sorry after doing more testing and looking at the output I think
>>>>>>>> we need to do better. For a machine with no lsof we see:
>>>>>>>>
>>>>>>>> using command: <not found>
>>>>>>
>>>>>> That looks like an error ...
>>>>>>
>>>>>>>> (Second VM) Open file descriptors:
>>>>>>>>
>>>>>>>> using command: <not found>
>>>>>>
>>>>>> That looks like an error too - but why did we keep going after the
>>>>>> first error?
>>>>>>>> (Third VM) Open file descriptors:
>>>>>>>>
>>>>>>>> VM RESULT => RETAINS FD
>>>>>>>> VM RESULT => VM EXIT
>>>>>>>>
>>>>>>>> Log file was not inherited by third VM
>>>>>>
>>>>>> But that's not true - we couldn't determine anything! So this
>>>>>> looks like an accidental pass rather than an deliverate skip.
>>>>>>
>>>>>>>> ---
>>>>>>>>
>>>>>>>> That looks like an error is occurring to me and that we pass by
>>>>>>>> accident. I think we need to terminate the test, cleanly of
>>>>>>>> course, as soon as we can't find the tool and report that the
>>>>>>>> test can't run because of that.
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>> David
>>>>>>>
>>>>>>> I could change the output to "using command: <not found,
>>>>>>> terminating test as successful as we can not finish it in a
>>>>>>> meaningful way>" or maybe something else that you prefer that is
>>>>>>> better.
>>>>>>>
>>>>>>> I do believe this is the easy way to terminate the test cleanly
>>>>>>> without leaving running JVMs. We need to communicate back to the
>>>>>>> first JVM that the test was successful. Is it the string "<not
>>>>>>> found>" that looks like an error and that we pass by accident? Or
>>>>>>> is it something in the test case code that looks fishy?
>>>>>>
>>>>>> See above.
>>>>>>
>>>>>> I'd suggest locating the command as one of the first things in the
>>>>>> test in the first VM and then bail out if not found.
>>>>>>
>>>>>> Thanks,
>>>>>> David
>>>>>> ------
>>>>>>
>>>>>>> Or maybe the comment should be improved?
>>>>>>> // if command can not be found return list without log file
>>>>>>> (some machines does not have "lsof" in the expected place). (add
>>>>>>> this->)The empty list will make the test exit successfully.(<-add
>>>>>>> this)
>>>>>>>
>>>>>>>> On 15/06/2018 11:21 AM, David Holmes wrote:
>>>>>>>>> Hi Leo,
>>>>>>>>>
>>>>>>>>> This is what I was concerned about - and yes the CI testing did
>>>>>>>>> shake it out in a few cycles.
>>>>>>>>>
>>>>>>>>> I agree with the fix and will sponsor it for you.
>>>>>>>
>>>>>>> Thanks David.
>>>>>>> /Leo
>>>>>>>
>>>>>>>>>
>>>>>>>>> Thanks,
>>>>>>>>> David
>>>>>>>>>
>>>>>>>>> On 15/06/2018 3:31 AM, Leo Korinth wrote:
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> On 14/06/18 19:10, Lindenmaier, Goetz wrote:
>>>>>>>>>>> Hi Leo,
>>>>>>>>>>>
>>>>>>>>>>> the fix looks good. It makes sense to skip a test if the
>>>>>>>>>>> infrastructure is
>>>>>>>>>>> insufficient to run it.
>>>>>>>>>>>
>>>>>>>>>>> Could you please put the long comment at the end of line 179
>>>>>>>>>>> into a line of it's own?
>>>>>>>>>>
>>>>>>>>>> Fixed
>>>>>>>>>>
>>>>>>>>>>> Also, please take the chance and move the Copyright message
>>>>>>>>>>> to the beginning of the file.
>>>>>>>>>>
>>>>>>>>>> Fixed
>>>>>>>>>>
>>>>>>>>>>> Best regards,
>>>>>>>>>>> Goetz.
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Thanks for the fast review; I need a second reviewer and
>>>>>>>>>> someone to help me push (I am not a commiter).
>>>>>>>>>>
>>>>>>>>>> New webrevs:
>>>>>>>>>> http://cr.openjdk.java.net/~lkorinth/8205054/00_01/ (incremental)
>>>>>>>>>> http://cr.openjdk.java.net/~lkorinth/8205054/01/ (full)
>>>>>>>>>>
>>>>>>>>>> Thanks,
>>>>>>>>>> Leo
>>>>>>>>>>
>>>>>>>>>>>> -----Original Message-----
>>>>>>>>>>>> From: hotspot-runtime-dev [mailto:hotspot-runtime-dev-
>>>>>>>>>>>> bounces at openjdk.java.net] On Behalf Of Leo Korinth
>>>>>>>>>>>> Sent: Donnerstag, 14. Juni 2018 18:55
>>>>>>>>>>>> To: Hotspot dev runtime <hotspot-runtime-dev at openjdk.java.net>
>>>>>>>>>>>> Subject: RFR: 8205054: Could not find "lsof" on test machine
>>>>>>>>>>>>
>>>>>>>>>>>> Hi,
>>>>>>>>>>>>
>>>>>>>>>>>> Unfortunately some test machines does not have "lsof"
>>>>>>>>>>>> installed, or it
>>>>>>>>>>>> can not be found.
>>>>>>>>>>>>
>>>>>>>>>>>> I do not know how to find "lsof" on the test machine where
>>>>>>>>>>>> the test
>>>>>>>>>>>> fails; the command might not be installed. Here is a fix to
>>>>>>>>>>>> make the
>>>>>>>>>>>> test case succeed if "lsof" can not be found. I believe it
>>>>>>>>>>>> is the best
>>>>>>>>>>>> option (at least for now). I am sorry for all the noise I am
>>>>>>>>>>>> generating.
>>>>>>>>>>>>
>>>>>>>>>>>> Bug:
>>>>>>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8205054
>>>>>>>>>>>>
>>>>>>>>>>>> Webrev:
>>>>>>>>>>>> http://cr.openjdk.java.net/~lkorinth/8205054/00/
>>>>>>>>>>>>
>>>>>>>>>>>> Testing:
>>>>>>>>>>>> I have tested this on my machine, and I have a mach5 test
>>>>>>>>>>>> job running.
>>>>>>>>>>>>
>>>>>>>>>>>> Thanks,
>>>>>>>>>>>> Leo
>>
More information about the hotspot-runtime-dev
mailing list