RFC: JMC-6173: jmc -open requires full path

Tue Jul 23 09:53:03 UTC 2019

Hi Carmine, 

> On 12 Jul 2019, at 13:42, Carmine Vincenzo Russo <carusso at redhat.com> wrote:
> 
> Hi Erik,
> 
> I want to point out  a couple of things before going forward. 
> 
> In the issue description it says to use $ jmc -open recording.jfr instead of $ jmc -open /Demo/recording.jfr
> That's why I assumed the subdirectories where needed. While I get from your words that we are working in the directory /Demo and from the command line it needs the full path even if the file is in the current directory. Please correct me if I am wrong.

Correct.

> 
> Another thing is that I tried with the sequence of command you suggested as use case, when I run 
> $ jcmd 4711 JFR.dump filename=dump.jfr
> It saves the recording in the application directory and not in my current working directory,

It will save in a directory relative to where the java process was started from, unless you give a full path. 

(It’s hard to do anything about jcmd, because JVM has no knowledge from where jcmd was launched.)

> therefore everything should be executed in the application directory for your use case,
> or in alternative we should use a full path, but since it is in a different directory the issue shouldn't apply to it.
> 
> To summarize,
> if the issue is about opening the recording in the current directory, it already works for me without any patch (tested on Fedora).

I could not get it to work on Mac OS X, I needed to use the full path.

Erik

> If is about opening the recording from somewhere else, than can you give me more details about the issue? It's not really clear.
> 
> Thanks,
> 
> Carmine 
> 
> 
> 
> On Mon, Jul 8, 2019 at 10:47 PM Erik Gahlin <erik.gahlin at oracle.com <mailto:erik.gahlin at oracle.com>> wrote:
> Hi Carmine, 
> 
> See comments inline.
>  
>> Hi Erik, 
>> 
>> Thank you for your response. 
>> 
>> On Thu, Jun 27, 2019 at 6:05 PM Erik Gahlin <erik.gahlin at oracle.com <mailto:erik.gahlin at oracle.com>> wrote:
>> Hi Carmine ,
>> 
>> Searching the home directory could take a very long time. It could also 
>> open the wrong file if several files have the same name. I don't think 
>> it is the correct approach.
>> 
>> I know, starting from the user home is a little bit extreme and can take a while, therefore instead I'm starting from the current directory
> It's the same problem. 
> 
> On my computer, it could take an hour if I am in the wrong directory or a network share. I don't know any other applications that searches for a file in subdirectories.
> 
>> in the updated patch and searching the subdirectories from there.
>> Since JMC doesn't have a specific folder to save the recordings it all depends on the users. 
> This is primarily meant for command-line users. For example, 
> 
> $ java -XX:StartFlightRecording -jar my.jar 
> ...
> $ jcmd 4711 JFR.dump filename=dump.jfr
> $ jmc -open recording.jfr
> 
> The cumbersome part here is to switch to a GUI and do various operations with the mouse. 
> 
>> As for files with the same name, there is the same problem, JMC gives the possibility to choose the name for every recordings, therefore to have two files with the same name the user should do it on purpose. 
>>  
>> 
>> Have you tried Paths.get(".", filename) ?
>> 
>> It will look in the current working directory. To see if the full path 
>> is correct do
>> 
>>    path.normalize().toAbsolutePath()
>> 
>> if this doesn't work, it could be because of some magic happening in jmc 
>> launcher, in which case the current directory have to be extracted in 
>> the launcher code. 
>> 
>> Updated the patch with you suggestions regarding Paths, everything seems to work fine.
> Good, have you tried on the most common OSes (Mac, Linux and Windows)?
> 
> Thanks
> Erik
> 
>> At the moment a full path, a path with ~/ and a filename both in the current directory or in a subdirectories of the current directory open a recording. 
>> Also I made some tests for the function findFile, there are 3 tests right now: 
>> - EmptyPathTest: tests what happens with an empty string as filename.
>> - ValidFileNameTest: tests with a temp file whether findFile can find the correct file. 
>> - InvalidFileNameTest: tests what happens when the given filename doesn't exists.
>>  
>> Here the updated patch: http://cr.openjdk.java.net/~aptmac/JMC-6173/webrev.01/ <http://cr.openjdk.java.net/%7Eaptmac/JMC-6173/webrev.01/>
>> 
>> Thanks,
>> Carmine
>> 
>> 
>> Erik
>> > Hi,
>> >
>> > On Fri, Jun 21, 2019 at 10:39 AM Jie Kang <jkang at redhat.com <mailto:jkang at redhat.com>> wrote:
>> >
>> >> On Fri, Jun 21, 2019 at 8:31 AM Carmine Vincenzo Russo
>> >> <carusso at redhat.com <mailto:carusso at redhat.com>> wrote:
>> >>> Hello,
>> >>>
>> >>> The patch addresses the issue JMC-6173[0].
>> >>> I have added a check to see if the path is full or not. If it falls in
>> >> the
>> >>> second case, starting from the user home it recursively search for the
>> >>> given file name in all the not hidden folder.
>> >>> There was also a problem with relative path like ~/record/test.jfr, now
>> >>> fixed.
>> >>> I tested the patch with full path, relative path, filename only and null
>> >>> value.
>> >>>
>> >>> I'm planning to add few tests, at least for a valid filename, an invalid
>> >>> filename and a null value.
>> >>>
>> >>> See the attached patch bellow and let me know what you think.
>> >> I don't see an attachment unfortunately; was it missed or maybe stripped?
>> >>
>> > Carmine sent me a note saying he attached the patch to the e-mail but it
>> > doesn't appear to show up here. Instead, please find his patch at the
>> > following webrev:
>> >
>> > http://cr.openjdk.java.net/~aptmac/JMC-6173/webrev.00/ <http://cr.openjdk.java.net/%7Eaptmac/JMC-6173/webrev.00/>
>> >
>> > I believe he's looking into unit tests at the moment, but wanted feedback
>> > on his implementation before going forward.
>> >
>> >
>> >> Regards,
>> >>
>> >>> Thank you!
>> >>> Carmine
>> >>>
>> >>>
>> >>> [0] https://bugs.openjdk.java.net/browse/JMC-6173 <https://bugs.openjdk.java.net/browse/JMC-6173>
>> >>>
>> >>> --
>> >>> Carmine Vincenzo Russo
>> > Cheers,
>> >
>> > Alex
>> 
>> 
>> 
>> -- 
>> Carmine Vincenzo Russo 
> 
> 
> 
> -- 
> Carmine Vincenzo Russo 