Bugzilla – Full Text Bug Listing
| Summary: | Reimplement org.eclipse.linuxtools.profiling.launch.remote.RemoteConnection in terms of the IRemote* interfaces | ||
|---|---|---|---|
| Product: | [Tools] Linux Tools | Reporter: | Corey Ashford <cjashfor> |
| Component: | Project | Assignee: | Linux Distros Inbox <linux.distros-inbox> |
| Status: | RESOLVED FIXED | QA Contact: | |
| Severity: | enhancement | ||
| Priority: | P3 | CC: | jjohnstn, obusatto, rafaelmt |
| Version: | 0.9.1 | ||
| Target Milestone: | --- | ||
| Hardware: | All | ||
| OS: | Linux | ||
| Whiteboard: | |||
| Attachments: | |||
|
Description
Corey Ashford
Created attachment 210156 [details]
Reimplement RemoteConnection in terms of IRemote* and move it to the parent package.
(In reply to comment #1) > Created attachment 210156 [details] > Reimplement RemoteConnection in terms of IRemote* and move it to the parent > package. RemoteConnection looks good. I like that it can be moved into the common launch plug-in. 1. You don't need to use ConsoleOutputStream in RemoteConnection. Use ByteArrayOutputStream instead. These are valid input to the remote command methods. 2. Can you explain the CMainTab replacement? This seems outside the description of the bug. (In reply to comment #2) > (In reply to comment #1) > > Created attachment 210156 [details] > > Reimplement RemoteConnection in terms of IRemote* and move it to the parent > > package. > > RemoteConnection looks good. I like that it can be moved into the common > launch plug-in. > > 1. You don't need to use ConsoleOutputStream in RemoteConnection. > Use ByteArrayOutputStream instead. These are valid input > to the remote command methods. > > 2. Can you explain the CMainTab replacement? This seems outside > the description of the bug. The motivation to put it in this patch is to show how RemoteConnection is used. One of the little development tips I picked up from doing kernel work was to add infrastructure and usage of that infrastructure in the same patch so that you don't have unused code at any commit in the tree. I can put it in a separate patch if you like, though. (In reply to comment #3) > 1. You don't need to use ConsoleOutputStream in RemoteConnection. > Use ByteArrayOutputStream instead. These are valid input > to the remote command methods. I'll fix this. (In reply to comment #3) > (In reply to comment #2) > > (In reply to comment #1) > > > Created attachment 210156 [details] > > > Reimplement RemoteConnection in terms of IRemote* and move it to the parent > > > package. > > > > RemoteConnection looks good. I like that it can be moved into the common > > launch plug-in. > > > > 1. You don't need to use ConsoleOutputStream in RemoteConnection. > > Use ByteArrayOutputStream instead. These are valid input > > to the remote command methods. > > > > 2. Can you explain the CMainTab replacement? This seems outside > > the description of the bug. > > The motivation to put it in this patch is to show how RemoteConnection is used. > One of the little development tips I picked up from doing kernel work was to > add infrastructure and usage of that infrastructure in the same patch so that > you don't have unused code at any commit in the tree. > I understand, but easier to do when not talking about UI which brings in opinion. > I can put it in a separate patch if you like, though. I would suggest a separate bug that relies on this one and the resource selector bug, both of which look good to be committed. There you can state what the tab looks like, how it is different from the normal CMainTab (if it differs other than the resource selectors). It will also break up the patches in size. For all of these bugs, please put in a statement that you are the author of the changes and that you have permission from your employer to submit them so we can expedite the iplog process. (In reply to comment #5) > (In reply to comment #2) > > The motivation to put it in this patch is to show how RemoteConnection is used. > > One of the little development tips I picked up from doing kernel work was to > > add infrastructure and usage of that infrastructure in the same patch so that > > you don't have unused code at any commit in the tree. > > > > I understand, but easier to do when not talking about UI which brings in > opinion. Heh, OK, good point. > > > I can put it in a separate patch if you like, though. > > I would suggest a separate bug that relies on this one and the resource > selector bug, both of which look good to be committed. There you can state > what the tab looks like, how it is different from the normal CMainTab (if it > differs other than the resource selectors). It will also break up the patches > in size. Will do. > > For all of these bugs, please put in a statement that you are the author of the > changes and that you have permission from your employer to submit them so we > can expedite the iplog process. Will do. Created attachment 210420 [details] Patch to reimplement RemoteConnection in terms of the IRemote* classes and move it it to its parent class, version 2 This patch addresses the concerns raised in comment #5. I wrote this patch myself and have the permission of my employer (IBM Corp.) to submit it. Created attachment 210421 [details]
Adds new example CMainTab implementation based on the RemoteResourceSelectorWidget
This is an example usage of three instances of the RemoteResourceSelector widget to create a launch tab with remote capability.
I wrote this patch and have the permission of my employer (IBM Corp.) to submit it.
(In reply to comment #8) > Created attachment 210421 [details] > Adds new example CMainTab implementation based on the > RemoteResourceSelectorWidget > > This is an example usage of three instances of the RemoteResourceSelector > widget to create a launch tab with remote capability. > > I wrote this patch and have the permission of my employer (IBM Corp.) to submit > it. Please remove this 2nd patch if you can or obsolete it. It is not needed here and doesn't apply to the bug. I am fine with approving your other patch once that is done. I would have to iplog+ this 2nd patch noting that it isn't checked in and you will have to submit that patch as part of a separate bug anyway. Eventually, I assume you will be creating a bug to change the remote Valgrind implementation to use the new building blocks. Your CMainTab appears to be one of those building blocks and you will have to change the remote Valgrind UI to not specify the working directory in its own tab as well as change the non-UI code to get it from the new tab. You can put this patch into a new bug that has a dependency on your other widget bug and this bug as well. Comment on attachment 210421 [details]
Adds new example CMainTab implementation based on the RemoteResourceSelectorWidget
I'm marking the new CMainTab implementation as obsolete, so that it's clear that it's not required as a solution to the request stated in the bugzilla. It was only an example of use of the other patch.
Once this patch is accepted, I will add a new bugzilla to request a CMainTab that is based on the ResourceSelectorWidgit, and another one for the Valgrind launch delegate which is, in turn, based on that tab.
(In reply to comment #10) > Comment on attachment 210421 [details] > Adds new example CMainTab implementation based on the > RemoteResourceSelectorWidget > > I'm marking the new CMainTab implementation as obsolete, so that it's clear > that it's not required as a solution to the request stated in the bugzilla. > It was only an example of use of the other patch. > > Once this patch is accepted, I will add a new bugzilla to request a CMainTab > that is based on the ResourceSelectorWidgit, and another one for the Valgrind > launch delegate which is, in turn, based on that tab. Great. I approve. I'll work on getting this committed. Any news on this one Jeff? Can we apply this patch? Corey, can you state that this patch is epl, that you did it by yourself, and that your company allows you to do it. Thanks This patch is EPL. I wrote 100% of it myself. I had permission from IBM to do so. committed: 8ed47c6da14a8ba3f6f996b11b7137e7c66c1b1a |