Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.
Bug 353056 - Add support for multiple Valgrind launch configs
Summary: Add support for multiple Valgrind launch configs
Status: RESOLVED FIXED
Alias: None
Product: Linux Tools
Classification: Tools
Component: Valgrind (show other bugs)
Version: 0.8.0   Edit
Hardware: All Linux
: P3 enhancement (vote)
Target Milestone: 0.9.1   Edit
Assignee: Jeff Johnston CLA
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-07-25 20:20 EDT by Corey Ashford CLA
Modified: 2011-11-09 14:19 EST (History)
6 users (show)

See Also:


Attachments
Adds capability of creating multiple Valgrind remote configs via new abstract classes (35.90 KB, patch)
2011-07-25 20:31 EDT, Corey Ashford CLA
no flags Details | Diff
Create an extension point to change the valgrind executable path (30.77 KB, patch)
2011-08-31 13:06 EDT, Otavio Pontes CLA
no flags Details | Diff
Screenshot of the Linuxtools Path Project Property Page (39.55 KB, image/png)
2011-09-30 16:36 EDT, Otavio Pontes CLA
no flags Details
Screenshot of the Linuxtools Path Project Property Page - Option added By an extension point (38.62 KB, image/png)
2011-09-30 16:36 EDT, Otavio Pontes CLA
no flags Details
Another ss to show that Linuxtools Path is not inside C/C++ (50.71 KB, image/png)
2011-09-30 18:57 EDT, Otavio Pontes CLA
no flags Details
Screenshot of the Linuxtools Path Project Property Page -- new version (36.74 KB, image/png)
2011-10-04 13:02 EDT, Otavio Pontes CLA
no flags Details
Screenshot of the Linuxtools Path Project Property Page2 -- new version (37.01 KB, image/png)
2011-10-04 13:02 EDT, Otavio Pontes CLA
no flags Details
Screenshot of the Linuxtools Path Project Property Page3 -- new version (37.24 KB, image/png)
2011-10-04 13:03 EDT, Otavio Pontes CLA
no flags Details
Screenshot of the Linuxtools Path Project Property Page4 -- new version (48.07 KB, image/png)
2011-10-04 13:03 EDT, Otavio Pontes CLA
no flags Details
Screenshot of the Linuxtools Path Project Property Page -- new version (52.63 KB, image/png)
2011-10-17 08:55 EDT, Otavio Pontes CLA
no flags Details
Screenshot of the Linuxtools Path Project Property Page2 -- new version (37.00 KB, image/png)
2011-10-17 08:55 EDT, Otavio Pontes CLA
no flags Details
Screenshot of the Linuxtools Path Project Property Page3 -- new version (48.11 KB, image/png)
2011-10-17 08:56 EDT, Otavio Pontes CLA
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Corey Ashford CLA 2011-07-25 20:20:18 EDT
Build Identifier: I20110613-1736

As a bundler of linuxtools, and the Valgrind plug-in specifically, we would like to be able add multiple launch configs for the Valgrind plug-in so that we can make it easier for our customers to use specific Valgrind executables that we provide.

For example, we may have two Valgrind executables in addition to /usr/bin/valgrind.  In our case, we can have /opt/at4.0/bin/valgrind and /opt/at5.0/bin/valgrind, and perhaps others in the future.

We would like to make it simple for users of our SDK to choose a profile config that already has the path to the valgrind executable pre-configured correctly.

Reproducible: Always

Steps to Reproduce:
Notice that the current upstream remote Valgrind code doesn't provide a simple way to accomplish the above.
Comment 1 Corey Ashford CLA 2011-07-25 20:31:45 EDT
Created attachment 200320 [details]
Adds capability of creating multiple Valgrind remote configs via new abstract classes

In order to make it possible for a another plug-in to add new Valgrind
profile launch configurations, I've made two of the internal classes
Abstract and exported: ValgrindRemoteLaunchDelegate and ValgrindRemoteTab,
now both given the "Abstract" prefix.  These two abstract classes are
now extended and instantiated by internal classes of the same previous
names.  The net change is unnoticeable by users of Valgrind remote.
    
The only parameterization added to the configs, at this point, is the
location of the Valgrind executable, and the attribute used to acquire
the current (i.e. persistent) setting of this location. Other
parameterizations are possible, given further modifications of
the abstract classes.

Since I'm pretty new to Eclipse plug-in programming, it's not clear to me that
what I've done is the best way to accomplish the goal, but it certainly does work.
Comment 2 Otavio Pontes CLA 2011-08-31 13:06:26 EDT
Created attachment 202543 [details]
Create an extension point to change the valgrind executable path

I have another approach to work with several valgrind executables installed in the system at the same time.
I coded an extension point in valgrind plugin to make it possible for a third-party plugins to change the valgrind location using a ILaunchConfiguration object. So it is possible to implement a plugin that adds a project property to change the path of the valgrind executable.
T patch is attached and also available at: https://github.com/obusatto/linuxtools
Comment 3 Jeff Johnston CLA 2011-09-07 19:22:55 EDT
(In reply to comment #0)
> Build Identifier: I20110613-1736
> 
> As a bundler of linuxtools, and the Valgrind plug-in specifically, we would
> like to be able add multiple launch configs for the Valgrind plug-in so that we
> can make it easier for our customers to use specific Valgrind executables that
> we provide.
> 
> For example, we may have two Valgrind executables in addition to
> /usr/bin/valgrind.  In our case, we can have /opt/at4.0/bin/valgrind and
> /opt/at5.0/bin/valgrind, and perhaps others in the future.
> 
> We would like to make it simple for users of our SDK to choose a profile config
> that already has the path to the valgrind executable pre-configured correctly.
> 
> Reproducible: Always
> 
> Steps to Reproduce:
> Notice that the current upstream remote Valgrind code doesn't provide a simple
> way to accomplish the above.

Corey, can you provide some more detail on the desired use case?  Do you expect the end-user to have more than one choice of Valgrind in the Profiling configurations dialog or do you just want to override the default to be set up for the Eclipse session?  If the latter, then that would seem to be more of what Otavio is proposing which appears to override the Valgrind executable based on an extension and uses priority to handle the case where more than one extension is specified.  

If the former behaviour is desired, what do you present to the end-user to decide (is it simply Valgrind at4.0, Valgrind at5.0, Valgrind)?

If the latter, I might see a more generic system of using an environment variable and in addition using a generic extension to default variables.  This would allow the user to set their environment manually once or to allow a plug-in to set up a default if the user hasn't specified anything.  The environment variable/default setting mechanism could be used to seed the workspace preferences in the case where project overriding is required.

e.g. EclipseValgrindLocation env-var contains the location of valgrind binary

Valgrind launch looks for environment variable and if found, uses value.
If not found, it looks through list of EclipseDefaultSetting extensions looking for EclipseValgrindLocation.  If found, it sets it appropriately.  I don't believe a priority system is warranted.  I would simply expect the end-user to have the proper override installed or use the environment variable and/or set preferences/properties manually.  First found would be first used.  I would see it as a convenience for having a reasonable set of defaults at start-up.

Finally, if nothing is found, the plug-in already defaults to /usr/bin/valgrind

With this idea, it allows the end-user to install in weird locations that a plug-in can't foresee and avoid having to constantly make changes every new Eclipse session.  This same mechanism could be re-used for the autotool locations, etc...

Right now, ChangeLog uses environment variables to have the settings for name and e-mail address.  I find it very convenient that I have only had to set these once.

An idea.
Comment 4 Corey Ashford CLA 2011-09-07 22:03:19 EDT
(In reply to comment #3)
 > Corey, can you provide some more detail on the desired use case?  Do you expect
> the end-user to have more than one choice of Valgrind in the Profiling
> configurations dialog or do you just want to override the default to be set up
> for the Eclipse session?  If the latter, then that would seem to be more of
> what Otavio is proposing which appears to override the Valgrind executable
> based on an extension and uses priority to handle the case where more than one
> extension is specified.

> 
> If the former behaviour is desired, what do you present to the end-user to
> decide (is it simply Valgrind at4.0, Valgrind at5.0, Valgrind)?

To be perfectly honest, we're not 100% sure what would really be best.  I would like the user to be able to specify on a per-project basis what toolset they want to use.  Some of the tools will come from, /usr/bin in all cases.  For example, our "Advance Toolchain" doesn't contain the autotools executables (e.g. autoconf).

> 
> If the latter, I might see a more generic system of using an environment
> variable and in addition using a generic extension to default variables.  This
> would allow the user to set their environment manually once or to allow a
> plug-in to set up a default if the user hasn't specified anything.  The
> environment variable/default setting mechanism could be used to seed the
> workspace preferences in the case where project overriding is required.
> 
> e.g. EclipseValgrindLocation env-var contains the location of valgrind binary
> 
> Valgrind launch looks for environment variable and if found, uses value.
> If not found, it looks through list of EclipseDefaultSetting extensions looking
> for EclipseValgrindLocation.
> If found, it sets it appropiately.  I don't
> believe a priority system is warranted.  I would simply expect the end-user to
> have the proper override installed or use the environment variable and/or set
> preferences/properties manually.  First found would be first used.  I would see
> it as a convenience for having a reasonable set of defaults at start-up
> 
> Finally, if nothing is found, the plug-in already defaults to /usr/bin/valgrind

The problem with this, though, is that if the user has a project where he's been successfully using valgrind from toolchain version X, then he does an install of the toolchain version X+1 for some newer project, and now he's using valgrind X+1 in his older project, but still using the compiler version X for the build.  We have a potential of a mismatch.  In general, it would be nice to believe that the tools are backward-compatible with earlier compilers, but I think it's safer to assume that Valgrind X+1 should be used with compiler X+1, and Valgrind X with compiler X.
> 
> With this idea, it allows the end-user to install in weird locations that a
> plug-in can't foresee and avoid having to constantly make changes every new
> Eclipse session.  This same mechanism could be re-used for the autotool
> locations, etc...

How about some way of specify the remote tool search path?  We'd just specify a single, per-project setting, that would be similar to the $PATH variable.  It could contain a colon separated list of paths, or perhaps a fancier GUI widget could be used.

> 
> Right now, ChangeLog uses environment variables to have the settings for name
> and e-mail address.  I find it very convenient that I have only had to set
> these once.

One problem is that it's not intuitively obvious how to set the path of Valgrind (or other tools).  The user would somehow have to know to go to the Environment variables to change it.  It becomes a "hidden option", unless maybe you offer a duplicate interface inside of the launcher config somehow.

What do you think?
Comment 5 Jeff Johnston CLA 2011-09-08 14:50:03 EDT
> 
> How about some way of specify the remote tool search path?  We'd just specify a
> single, per-project setting, that would be similar to the $PATH variable.  It
> could contain a colon separated list of paths, or perhaps a fancier GUI widget
> could be used.
>

Setting the per-project path makes sense for what you want to do.  There isn't much point to adding a new environment variable since we would need to prepend it to PATH every time we launched.  Much simpler to modify the PATH variable for the project unless there was a reason to not use the environment in certain instances (e.g. want it for tools, but not for executing the program itself).

This is already possible with the current mechanisms in place though not entirely easy for an end-user.  Actually, right now the default autotools are specified without path and it is assumed they are found on the project's PATH.  Thinking about it, it would make sense to the do the same with the Valgrind executable (i.e. just specify valgrind instead of /usr/bin/valgrind).

So how to make this convenient for the end-user?

Here's a thought.  We can append pages to the New project wizard via the current template extension mechanism.

Thus, there could be a custom page added which allows the user to set the PATH before the project is created.

We could potentially add this new template page to all our current Autotool project templates.  The template page could provide a directory search and could have a button to prepend a found directory or append to the current PATH.  There could be a list of commonly used/suggested directories immediately accessible (e.g. /usr/bin, /usr/local/bin).  We could have an extension that would allow additional suggestions.  Thus, you could simply add extensions for each of your tool-chain locations and the locations could be made accessible to the end-user without having to search.  If the locations need
to be calculated, a class could be specified in the extension which does the search and provides the result.

This unfortunately does not solve the non-Autotools issue, but perhaps the CDT could be convinced to add a similar page for their templates.

An alternative would be for you to create your own templates (e.g. Managed TOOLSET x empty project, Managed TOOLSET x hello world project) but that would permutate the number of projects in the C/C++ Project Wizards.

Any of these ideas appeal to you?
Comment 6 Corey Ashford CLA 2011-09-09 15:08:52 EDT
Sorry for the delay in my reply.  I was out yesterday.

(In reply to comment #5)
> > 
> > How about some way of specify the remote tool search path?  We'd just specify a
> > single, per-project setting, that would be similar to the $PATH variable.  It
> > could contain a colon separated list of paths, or perhaps a fancier GUI widget
> > could be used.
> >
> 
> Setting the per-project path makes sense for what you want to do.  There isn't
> much point to adding a new environment variable since we would need to prepend
> it to PATH every time we launched.  Much simpler to modify the PATH variable
> for the project unless there was a reason to not use the environment in certain
> instances (e.g. want it for tools, but not for executing the program itself).
> 

You make a good point about the executing program "seeing" the modified $PATH.  It would be safest not modify the PATH variable, but that would mean that the launch delegate may have to locate executable on it's own by searching through the directories in the Project's path config.  I think each launch delegate could cache up the location of the executable that it found.  Ideally, would display what it found, but allow the user to select a different one or add one of his own.

> This is already possible with the current mechanisms in place though not
> entirely easy for an end-user.  Actually, right now the default autotools are
> specified without path and it is assumed they are found on the project's PATH. 
> Thinking about it, it would make sense to the do the same with the Valgrind
> executable (i.e. just specify valgrind instead of /usr/bin/valgrind).
>

For autotools, you could just use $PATH because there's no user executable involved.
 
> So how to make this convenient for the end-user?
> 
> Here's a thought.  We can append pages to the New project wizard via the
> current template extension mechanism.
> 
> Thus, there could be a custom page added which allows the user to set the PATH
> before the project is created.
> 
> We could potentially add this new template page to all our current Autotool
> project templates.  The template page could provide a directory search and
> could have a button to prepend a found directory or append to the current PATH.
>  There could be a list of commonly used/suggested directories immediately
> accessible (e.g. /usr/bin, /usr/local/bin).  We could have an extension that
> would allow additional suggestions.  Thus, you could simply add extensions for
> each of your tool-chain locations and the locations could be made accessible to
> the end-user without having to search.  If the locations need
> to be calculated, a class could be specified in the extension which does the
> search and provides the result.

I like this idea of extensions allowing a user to choose among some available paths, instead of trying to figure out for him which is the best or "correct" path to use.

> 
> This unfortunately does not solve the non-Autotools issue, but perhaps the CDT
> could be convinced to add a similar page for their templates.

Autotools is a weird beast.  Using $PATH is dangerous, because then you are requiring or relying on the person always to invoke the "make" from inside of Eclipse.  If they go to the command line, and forget to correctly configure their $PATH, they will get the wrong compilers path.

My original solution for that was to offer a way for extension to provide additional switches to the configure command.  Unfortunately, this doesn't work if you have more than one tool chain installed.

So I think autotools needs a way to select a toolchain, and then from that automatically supply CC=... and CXX=... options to the configure command.


> 
> An alternative would be for you to create your own templates (e.g. Managed
> TOOLSET x empty project, Managed TOOLSET x hello world project) but that would
> permutate the number of projects in the C/C++ Project Wizards.

Yeah, we did that actually, and it's OK with two toolchains, but as we go forward in time, it could get unwieldy.

> 
> Any of these ideas appeal to you?

Yes, I think we need a few more iterations of these ideas.
Comment 7 Jeff Johnston CLA 2011-09-14 16:39:25 EDT
(In reply to comment #6)

> > Setting the per-project path makes sense for what you want to do.  There isn't
> > much point to adding a new environment variable since we would need to prepend
> > it to PATH every time we launched.  Much simpler to modify the PATH variable
> > for the project unless there was a reason to not use the environment in certain
> > instances (e.g. want it for tools, but not for executing the program itself).
> > 
> 
> You make a good point about the executing program "seeing" the modified $PATH. 
> It would be safest not modify the PATH variable, but that would mean that the
> launch delegate may have to locate executable on it's own by searching through
> the directories in the Project's path config.  I think each launch delegate
> could cache up the location of the executable that it found.  Ideally, would
> display what it found, but allow the user to select a different one or add one
> of his own.
> 

A setting for TOOLS_PATH could be made for a tool-chain path to be pre-pended to the PATH before running tools.  This could be presented to the user in a template page with suggestions which would make it more obvious.  The choices could be seeded via extensions.

There should be no reason for searches for invoking tools such as valgrind or autotools via Eclipse.  The PATH should contain all that is needed.  For your other idea below in modifying the configuration, that's another story.

> 
> Autotools is a weird beast.  Using $PATH is dangerous, because then you are
> requiring or relying on the person always to invoke the "make" from inside of
> Eclipse.  If they go to the command line, and forget to correctly configure
> their $PATH, they will get the wrong compilers path.
> 
> My original solution for that was to offer a way for extension to provide
> additional switches to the configure command.  Unfortunately, this doesn't work
> if you have more than one tool chain installed.
> 
> So I think autotools needs a way to select a toolchain, and then from that
> automatically supply CC=... and CXX=... options to the configure command.
> 

There can be much more than just CC and CXX.  There all the binutils as well (e.g. LD, RANLIB, AS, AR, READELF, ADDR2LINE, STRIP).

To support a toolchain path, then each one of the flags above has to have a tools assoicated with it that needs to be located (e.g. CC needs to look for gcc in the toolchain path and possibly fall back to cc).  This presents a bit of a challenge if/when the toolchain path gets changed.  I think an option to do generation of various tool flags would be needed.  Default could be on if the toolchain is actually specified.  Majority of end-users will be happy to just use their default PATH as they would on the command line.

For now, I would suggest fixing the list of tools to be set and force the user to specify any obscure ones.
Comment 8 Otavio Pontes CLA 2011-09-15 13:25:52 EDT
> Valgrind launch looks for environment variable and if found, uses value.
> If not found, it looks through list of EclipseDefaultSetting extensions looking
> for EclipseValgrindLocation.  If found, it sets it appropriately.  I don't
> believe a priority system is warranted.  I would simply expect the end-user to
> have the proper override installed or use the environment variable and/or set
> preferences/properties manually.  First found would be first used.  I would see
> it as a convenience for having a reasonable set of defaults at start-up.

If we are going to use this approach I don't mind removing the priority system.
Comment 9 Jeff Johnston CLA 2011-09-15 14:24:20 EDT
(In reply to comment #8)
> > Valgrind launch looks for environment variable and if found, uses value.
> > If not found, it looks through list of EclipseDefaultSetting extensions looking
> > for EclipseValgrindLocation.  If found, it sets it appropriately.  I don't
> > believe a priority system is warranted.  I would simply expect the end-user to
> > have the proper override installed or use the environment variable and/or set
> > preferences/properties manually.  First found would be first used.  I would see
> > it as a convenience for having a reasonable set of defaults at start-up.
> 
> If we are going to use this approach I don't mind removing the priority system.

Thanks Otavio, appreciated.  Right now we're looking into a more generic mechanism.  Corey is looking to present a choice of toolchains to the user rather than dictate one in particular and it appears he wants to able to let the user change on the fly.  So, an extension that sets just the valgrind location is too specific as they want to let the user pick a gnu toolchain for the whole project and then have valgrind/gcc/etc.. defaulted from that toolchain location.

The current thought on solving this is a toolchain path/directory.

For valgrind and the autotools, they should just be defaulted without a path and expect the PATH at execution times to be properly set.
Comment 10 Corey Ashford CLA 2011-09-20 21:14:15 EDT
(In reply to comment #9)
> (In reply to comment #8)
> > > Valgrind launch looks for environment variable and if found, uses value.
> > > If not found, it looks through list of EclipseDefaultSetting extensions looking
> > > for EclipseValgrindLocation.  If found, it sets it appropriately.  I don't
> > > believe a priority system is warranted.  I would simply expect the end-user to
> > > have the proper override installed or use the environment variable and/or set
> > > preferences/properties manually.  First found would be first used.  I would see
> > > it as a convenience for having a reasonable set of defaults at start-up.
> > 
> > If we are going to use this approach I don't mind removing the priority system.
> 
> Thanks Otavio, appreciated.  Right now we're looking into a more generic
> mechanism.  Corey is looking to present a choice of toolchains to the user
> rather than dictate one in particular and it appears he wants to able to let
> the user change on the fly.  So, an extension that sets just the valgrind
> location is too specific as they want to let the user pick a gnu toolchain for
> the whole project and then have valgrind/gcc/etc.. defaulted from that
> toolchain location.
> 
> The current thought on solving this is a toolchain path/directory.
> 
> For valgrind and the autotools, they should just be defaulted without a path
> and expect the PATH at execution times to be properly set.

Hi Jeff.  Otavio is working on the same SDK that I am, and he is tackling this issue for us now.

We discussed this a bit more today, and we came up with the idea of a project properties page, which allows setting the tool paths for each of the tools that are part of the the Linux tools suite.  One thought would be to provide an extension point that would add a radio button to the page.    The default linux tools extension would add a "Distro toolchain" which would set all of the paths to /usr/bin.  Each additional extension would add a new radio button and its own set of tool paths.  There would be a separate "Set" button that would set the all of the paths based on which radio button was selected.  The paths for each tool are specified by the extension.  After the paths are set by clicking the Set button, you are free to alter them however you want, i.e. the path dialog boxes can always be edited.

What do you think?
Comment 11 Jeff Johnston CLA 2011-09-21 14:25:39 EDT
(In reply to comment #10)
> (In reply to comment #9)
> > (In reply to comment #8)
> > > > Valgrind launch looks for environment variable and if found, uses value.
> > > > If not found, it looks through list of EclipseDefaultSetting extensions looking
> > > > for EclipseValgrindLocation.  If found, it sets it appropriately.  I don't
> > > > believe a priority system is warranted.  I would simply expect the end-user to
> > > > have the proper override installed or use the environment variable and/or set
> > > > preferences/properties manually.  First found would be first used.  I would see
> > > > it as a convenience for having a reasonable set of defaults at start-up.
> > > 
> > > If we are going to use this approach I don't mind removing the priority system.
> > 
> > Thanks Otavio, appreciated.  Right now we're looking into a more generic
> > mechanism.  Corey is looking to present a choice of toolchains to the user
> > rather than dictate one in particular and it appears he wants to able to let
> > the user change on the fly.  So, an extension that sets just the valgrind
> > location is too specific as they want to let the user pick a gnu toolchain for
> > the whole project and then have valgrind/gcc/etc.. defaulted from that
> > toolchain location.
> > 
> > The current thought on solving this is a toolchain path/directory.
> > 
> > For valgrind and the autotools, they should just be defaulted without a path
> > and expect the PATH at execution times to be properly set.
> 
> Hi Jeff.  Otavio is working on the same SDK that I am, and he is tackling this
> issue for us now.
> 
> We discussed this a bit more today, and we came up with the idea of a project
> properties page, which allows setting the tool paths for each of the tools that
> are part of the the Linux tools suite.  One thought would be to provide an
> extension point that would add a radio button to the page.    The default linux
> tools extension would add a "Distro toolchain" which would set all of the paths
> to /usr/bin.  Each additional extension would add a new radio button and its
> own set of tool paths.  There would be a separate "Set" button that would set
> the all of the paths based on which radio button was selected.  The paths for
> each tool are specified by the extension.  After the paths are set by clicking
> the Set button, you are free to alter them however you want, i.e. the path
> dialog boxes can always be edited.
> 
> What do you think?

I'm not keen on this idea.  Less is more.  I think this should be done via a single path.  In fact, I am thinking that we should just override the PATH on the configure call.  I tried this out and indeed, it finds the gcc from my path.  This would work for all proper AC_CHECK_TOOL and AC_CHECK_PROG calls.  This allows one to override tools and they will be built into the Makefile so that this will handle the command line scenario as you wanted and we avoid having to generate any tool variables on the configure line.

So, we prepend the toolpath to PATH whenever we execute a tool and when we run configure, iff a toolpath is specified.  A single point of input for the end-user.  For the users who don't need this, nothing to worry about (they expect everything is on the path).  The toolpath won't be used to execute/debug the executable except under a tool (this can't be avoided in the case of valgrind where valgrind runs the executable beneath it).  Any oddball scenarios will require the user to do some more work (either in writing the configure script properly) or by specifying a tool absolutely in the UI and/or adding overriding variables on the configure call (e.g. SOMETOOL=).

Allowing the toolpath to change means the user can switch between your toolchains easily.  As mentioned before, with an extension, you can provide a location or sets of locations to help build the toolpath (either calculated or hard-coded).
Comment 12 Corey Ashford CLA 2011-09-26 19:21:50 EDT
Comment on attachment 200320 [details]
Adds capability of creating multiple Valgrind remote configs via new abstract classes

My original solution is obsolete.
Comment 13 Corey Ashford CLA 2011-09-28 15:23:44 EDT
Otavio ran across this article today.  It might put a hamper on the "just need to modify the PATH variable" idea:

http://weblog.dangertree.net/2007/04/13/changing-path-and-environment-in-java-processbuilder/
Comment 14 Otavio Pontes CLA 2011-09-28 16:22:47 EDT
(In reply to comment #13)
> Otavio ran across this article today.  It might put a hamper on the "just need
> to modify the PATH variable" idea:
> 
> http://weblog.dangertree.net/2007/04/13/changing-path-and-environment-in-java-processbuilder/

When I use Runtime.getRuntime.exec or ProcessBuilder java is using the System PATH to look for the binary, but environment of the new process is updated:
Runtime.getRuntime().exec("test", {"PATH=/my_path"}); fails, but 
Runtime.getRuntime().exec("which test", {"PATH=/my_path"}); works fine. So I could run the which command every time before running valgrind, but I'm avoiding this.
There are some places in code that we use ProcessFactory from CDT. When I use ProcessFactory it works. ProcessFactory.getFactory().exec("test", {"PATH=/my_path"}) works. But I am having trouble in getting the inputstream from the process.
Any idea?
Comment 15 Otavio Pontes CLA 2011-09-29 13:06:46 EDT
Hi,
I implemented the proposed solution and integrated it into valgrind. I'm using the 'which' command to find out the full path of the desired binary before each execution to fix the problem I reported in previous message.
If there is any suggestion, or something to change just let me know that I'll work to fix it.
I have more than one path to apply so I create the change_path branch in my github tree to share it with you. (https://github.com/obusatto/linuxtools/tree/change_path).
Comment 16 Otavio Pontes CLA 2011-09-30 16:36:00 EDT
Created attachment 204413 [details]
Screenshot of the Linuxtools Path Project Property Page
Comment 17 Otavio Pontes CLA 2011-09-30 16:36:59 EDT
Created attachment 204414 [details]
Screenshot of the Linuxtools Path Project Property Page - Option added By an extension point

The AT 4.0 option in this screenshot was added by the Extension Point defined in process_execution.ui.
Comment 18 Andrew Overholt CLA 2011-09-30 16:50:38 EDT
(In reply to comment #16)
> Created attachment 204413 [details]
> Screenshot of the Linuxtools Path Project Property Page

How does the user get another entry in the selector box thingy in this case?  Will it just show the path if the user has added one?

(In reply to comment #17)
> Created attachment 204414 [details]
> Screenshot of the Linuxtools Path Project Property Page - Option added By an
> extension point
> 
> The AT 4.0 option in this screenshot was added by the Extension Point defined
> in process_execution.ui.

Other than s/Linuxtools/Linux tools/ (not a capital 'T' since it's not the project, it's the actual path of the underlying tools themselves) this UI seems alright to me.  I prefer the extension point method for now.  I can't recall many (any?) users asking for this feature so I'd rather not add the UI and have people start depending on it.  I could be wrong, though, so if anyone else on CC has had reports of people asking for custom tool paths, please speak up.  Otavio, I would appreciate it if you could search old Linux Tools bugs, mailing list posts, and forum [1] posts and see if anyone has requested such a feature.  It'll help us decide if we have data on it.  Thanks!

[1]
http://www.eclipse.org/forums/index.php?t=thread&frm_id=60
Comment 19 Otavio Pontes CLA 2011-09-30 16:58:03 EDT
(In reply to comment #18)
> (In reply to comment #16)
> > Created attachment 204413 [details] [details]
> > Screenshot of the Linuxtools Path Project Property Page
> 
> How does the user get another entry in the selector box thingy in this case? 
> Will it just show the path if the user has added one?
> 
> (In reply to comment #17)
> > Created attachment 204414 [details] [details]
> > Screenshot of the Linuxtools Path Project Property Page - Option added By an
> > extension point
> > 
> > The AT 4.0 option in this screenshot was added by the Extension Point defined
> > in process_execution.ui.
> 
> Other than s/Linuxtools/Linux tools/ (not a capital 'T' since it's not the
> project, it's the actual path of the underlying tools themselves) this UI seems
> alright to me.  I prefer the extension point method for now.  I can't recall
> many (any?) users asking for this feature so I'd rather not add the UI and have
> people start depending on it.  I could be wrong, though, so if anyone else on
> CC has had reports of people asking for custom tool paths, please speak up. 
> Otavio, I would appreciate it if you could search old Linux Tools bugs, mailing
> list posts, and forum [1] posts and see if anyone has requested such a feature.
>  It'll help us decide if we have data on it.  Thanks!
> 
> [1]
> http://www.eclipse.org/forums/index.php?t=thread&frm_id=60

I didn't get one thing. I proposed to remove the dependency so that users will have to install the page manually if they want that, but I was intending to keep using a project property to store the path without creating a new extension point. Is it ok? And is it good to commit the screen in the linuxtools repository or should I keep it as a local plugin?
Comment 20 Jeff Johnston CLA 2011-09-30 18:04:32 EDT
(In reply to comment #15)
> Hi,
> I implemented the proposed solution and integrated it into valgrind. I'm using
> the 'which' command to find out the full path of the desired binary before each
> execution to fix the problem I reported in previous message.
> If there is any suggestion, or something to change just let me know that I'll
> work to fix it.
> I have more than one path to apply so I create the change_path branch in my
> github tree to share it with you.
> (https://github.com/obusatto/linuxtools/tree/change_path).

Otavio,

Thanks for the proposed solution.  A couple of comments.

1. Not fond of process_execution.  How about org.eclipse.linuxtools.tools.launch.core and org.eclipse.linuxtools.tools.launch.ui since you are actually handling the launching of tools?

2. The tool path property should be totally within the non-UI plugin so that
the UI plugin does not have to be included by a core package like
autotools.core

3. It would be helpful to include here some details on the various factories
and when you expect them to be used.  What is the plan for remote support?  I am just finishing a version of autotools that uses the RDT ProcessBuilder to do launching for both local and remote projects.  Perhaps we can discuss when you get a chance to see what I have done.  I'll post something to the mailing list.

4. The property page is a little confusing.  We're actually polling for a path to prepend to the system path.  So the default is to prepend nothing as opposed to System PATH.  So, perhaps use a radio button to be use default path and another button to say, prepend to path.  I think it will take some iterations to get this right.

5. The property page could possibly be in its own category (Linux Tools) as opposed to being under C/C++.  Anyone else have thoughts on this?
Comment 21 Otavio Pontes CLA 2011-09-30 18:55:57 EDT
> Otavio,
> 
> Thanks for the proposed solution.  A couple of comments.
> 
> 1. Not fond of process_execution.  How about
> org.eclipse.linuxtools.tools.launch.core and
> org.eclipse.linuxtools.tools.launch.ui since you are actually handling the
> launching of tools?

Ok. I agree with you.

> 
> 2. The tool path property should be totally within the non-UI plugin so that
> the UI plugin does not have to be included by a core package like
> autotools.core

ok. no problem.

> 
> 3. It would be helpful to include here some details on the various factories
> and when you expect them to be used.  What is the plan for remote support?  I
> am just finishing a version of autotools that uses the RDT ProcessBuilder to do
> launching for both local and remote projects.  Perhaps we can discuss when you
> get a chance to see what I have done.  I'll post something to the mailing list.

Nice. I'll document that classes. We can talk later about remote execution too.

> 
> 4. The property page is a little confusing.  We're actually polling for a path
> to prepend to the system path.  So the default is to prepend nothing as opposed
> to System PATH.  So, perhaps use a radio button to be use default path and
> another button to say, prepend to path.  I think it will take some iterations
> to get this right.

I see. I'll try to improve the layout. What about a checkbox?

> 
> 5. The property page could possibly be in its own category (Linux Tools) as
> opposed to being under C/C++.  Anyone else have thoughts on this?

It is not under C/C++. It is in a new category and bellow C/C++.
C/C++ is collapsed in this ss.
I'll take another one.
Comment 22 Otavio Pontes CLA 2011-09-30 18:57:23 EDT
Created attachment 204417 [details]
Another ss to show that Linuxtools Path is not inside C/C++
Comment 23 Otavio Pontes CLA 2011-10-04 13:02:36 EDT
Created attachment 204527 [details]
Screenshot of the Linuxtools Path Project Property Page -- new version
Comment 24 Otavio Pontes CLA 2011-10-04 13:02:49 EDT
Created attachment 204528 [details]
Screenshot of the Linuxtools Path Project Property Page2 -- new version
Comment 25 Otavio Pontes CLA 2011-10-04 13:03:01 EDT
Created attachment 204529 [details]
Screenshot of the Linuxtools Path Project Property Page3 -- new version
Comment 26 Otavio Pontes CLA 2011-10-04 13:03:35 EDT
Created attachment 204530 [details]
Screenshot of the Linuxtools Path Project Property Page4 -- new version

This option was added using an extension point. It will not be visible for regular users of Linuxtools
Comment 27 Otavio Pontes CLA 2011-10-04 13:06:32 EDT
I just added new screenshots for the Project Property Page.
I added two radio buttons as suggested (it was better than the checkbox) and remove some of the information in the screen.
The code was pushed to github too.
What do you thing now, Jeff?
Comment 28 Otavio Pontes CLA 2011-10-04 13:30:39 EDT
> > 3. It would be helpful to include here some details on the various factories
> > and when you expect them to be used.  What is the plan for remote support?

The idea we had was to use this property in the remote plugins to look for the binary in the remote machine. Corey is working in letting some plugins to run remotely and he said that this approach can be used by him to set the remote path. Maybe he can explain to you better than I what he is doing.
Comment 29 Corey Ashford CLA 2011-10-04 17:48:25 EDT
>> Corey is working in letting some plugins to run
>> remotely and he said that this approach can be used by him to set the remote
>> path. Maybe he can explain to you better than I what he is doing.

Let me be clear.  I didn't have in mind the creation of any factories.  My near-term goal was simply to get several of the tools working remotely so that we can ship something soon rather than waiting for a redesign, which I think Jeff Johnston is working on.

I have a Valgrind launcher working over RDT, but it uses a very simple-minded approach to get there.  It mostly re-implements the RemoteConnection class that
is part of org.eclipse.linuxtools.profiling.launch.remote using RDT instead of RSE.  It also adds a few more commonly used public static methods to the class for convenience.

We created a separate package for this class and its related exception and message classes, but we haven't done any further abstraction.

I should say that I am an inexperienced Java / Eclipse programmer, so while I understand the basic ideas of factories and so forth, I've don't have experience writing them.

I'm working on the OProfile plug-in at the moment.  My first inclination was to branch the plug-in and develop a RDT-only solution.  However, Otavio brought up the idea of using extensions to plug in a remote solution, and then we discussed copying in the RDT interfaces, cutting out the unneeded methods, and then utilizing those interfaces for remote operation.  The API's would be implemented using wrappers on the RDT code, and then plugged in via an extension point.  But after our discussion on IRC today, it's not clear to me that it's worth the extra trouble until Jeff makes a decision whether to incorporate that (or some other) extra proxy layer or not.
Comment 30 Otavio Pontes CLA 2011-10-05 12:55:50 EDT
I pushed a new commit to my branch to use the path property in gcof and gprof too.
Comment 31 Jeff Johnston CLA 2011-10-13 15:56:02 EDT
(In reply to comment #27)
> I just added new screenshots for the Project Property Page.
> I added two radio buttons as suggested (it was better than the checkbox) and
> remove some of the information in the screen.
> The code was pushed to github too.
> What do you thing now, Jeff?

Looks much better Otavio.  Tools should probably be capitalized in the Properties page.
Comment 32 Otavio Pontes CLA 2011-10-13 16:00:19 EDT
(In reply to comment #31)
> (In reply to comment #27)
> > I just added new screenshots for the Project Property Page.
> > I added two radio buttons as suggested (it was better than the checkbox) and
> > remove some of the information in the screen.
> > The code was pushed to github too.
> > What do you thing now, Jeff?
> 
> Looks much better Otavio.  Tools should probably be capitalized in the
> Properties page.

Not really.
Overholt wrote in a comment: "[...] s/Linuxtools/Linux tools/ (not a capital 'T' since it's not the project, it's the actual path of the underlying tools themselves) [...]"
Comment 33 Jeff Johnston CLA 2011-10-13 17:43:28 EDT
(In reply to comment #32)
> (In reply to comment #31)
> > (In reply to comment #27)
> > > I just added new screenshots for the Project Property Page.
> > > I added two radio buttons as suggested (it was better than the checkbox) and
> > > remove some of the information in the screen.
> > > The code was pushed to github too.
> > > What do you thing now, Jeff?
> > 
> > Looks much better Otavio.  Tools should probably be capitalized in the
> > Properties page.
> 
> Not really.
> Overholt wrote in a comment: "[...] s/Linuxtools/Linux tools/ (not a capital
> 'T' since it's not the project, it's the actual path of the underlying tools
> themselves) [...]"

I can't find a single instance of Project Property or Preference that uses lower case to start a word in the left-hand tree view so I would tend to disagree with this using the "when in Rome" argument :)
Comment 34 Otavio Pontes CLA 2011-10-17 08:55:11 EDT
Created attachment 205314 [details]
Screenshot of the Linuxtools Path Project Property Page -- new version
Comment 35 Otavio Pontes CLA 2011-10-17 08:55:32 EDT
Created attachment 205315 [details]
Screenshot of the Linuxtools Path Project Property Page2 -- new version
Comment 36 Otavio Pontes CLA 2011-10-17 08:56:03 EDT
Created attachment 205316 [details]
Screenshot of the Linuxtools Path Project Property Page3 -- new version
Comment 37 Otavio Pontes CLA 2011-10-17 08:58:17 EDT
(In reply to comment #33)
> (In reply to comment #32)
> > (In reply to comment #31)
> > > (In reply to comment #27)
> > > > I just added new screenshots for the Project Property Page.
> > > > I added two radio buttons as suggested (it was better than the checkbox) and
> > > > remove some of the information in the screen.
> > > > The code was pushed to github too.
> > > > What do you thing now, Jeff?
> > > 
> > > Looks much better Otavio.  Tools should probably be capitalized in the
> > > Properties page.
> > 
> > Not really.
> > Overholt wrote in a comment: "[...] s/Linuxtools/Linux tools/ (not a capital
> > 'T' since it's not the project, it's the actual path of the underlying tools
> > themselves) [...]"
> 
> I can't find a single instance of Project Property or Preference that uses
> lower case to start a word in the left-hand tree view so I would tend to
> disagree with this using the "when in Rome" argument :)

Good point. I changed that and pushed to github. It is also rebase to the current linuxtools master branch.
Comment 38 Jeff Johnston CLA 2011-10-26 15:29:28 EDT
(In reply to comment #37)
> (In reply to comment #33)
> > (In reply to comment #32)
> > > (In reply to comment #31)
> > > > (In reply to comment #27)
> > > > > I just added new screenshots for the Project Property Page.
> > > > > I added two radio buttons as suggested (it was better than the checkbox) and
> > > > > remove some of the information in the screen.
> > > > > The code was pushed to github too.
> > > > > What do you thing now, Jeff?
> > > > 
> > > > Looks much better Otavio.  Tools should probably be capitalized in the
> > > > Properties page.
> > > 
> > > Not really.
> > > Overholt wrote in a comment: "[...] s/Linuxtools/Linux tools/ (not a capital
> > > 'T' since it's not the project, it's the actual path of the underlying tools
> > > themselves) [...]"
> > 
> > I can't find a single instance of Project Property or Preference that uses
> > lower case to start a word in the left-hand tree view so I would tend to
> > disagree with this using the "when in Rome" argument :)
> 
> Good point. I changed that and pushed to github. It is also rebase to the
> current linuxtools master branch.

Looks fine Otavio.  Let's try and target the changes for 0.9.1.  It was already too late for 0.9.0 and Juno M3.  Please change your MANIFEST.MF and plugin.xml files to use a plugin.properties file for at least bundle name and provider.  See autotools docs plug-in for example.  You will need to add a Bundle-Localization clause.  Very minor change.  pom.xml file additions/changes should also be made so that everything builds using Tycho.

When you have been approved as a committer, you can open bugs for the changes (e.g. base tools.launch plug-ins, Valgrind support, gcov support, gprof support).  It will be simpler than going through the IPLog process now.
Comment 39 Otavio Pontes CLA 2011-10-27 15:39:47 EDT
New version pushed to github
Comment 40 Wainer dos Santos Moschetta CLA 2011-11-09 11:25:52 EST
I think it is in good shape to be pushed upstream, isn't it?
Comment 41 Otavio Pontes CLA 2011-11-09 14:19:06 EST
Patches applied upstream in commits: 19e0b8811e39591c124e0152c35c5a9058f9d9d4, 2ce832d53f111a53fc2b23d903c4b700c684fd9c, dd38d8163d294834c08fd6630c1e67dedd28ff90, 2900e65d8e0ff29a64d68b7c44d04503d7e6ec08, 0513f1fc6a3ff095d33d47984e6ff1577d11a830, 6c7ad8886d30f36f38c2b86bca4e9cbddc3d3782 and f0edddf0ad9cddfb4229b7915fa79d5f70149691