Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.
Bug 195454 - PDE supplies -pdelaunch to IApplication launches, which is not stripped out by platform
Summary: PDE supplies -pdelaunch to IApplication launches, which is not stripped out b...
Status: RESOLVED FIXED
Alias: None
Product: PDE
Classification: Eclipse Project
Component: UI (show other bugs)
Version: 3.3   Edit
Hardware: All All
: P3 major (vote)
Target Milestone: 3.3.1   Edit
Assignee: Brian Bauman CLA
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2007-07-04 18:47 EDT by Alex Blewitt CLA
Modified: 2007-08-29 13:19 EDT (History)
2 users (show)

See Also:


Attachments
Demonstration project that -pdelaunch is being passed through (4.87 KB, text/plain)
2007-07-04 18:48 EDT, Alex Blewitt CLA
no flags Details
Patch that hacks the fix (1.28 KB, patch)
2007-08-08 10:58 EDT, Alex Blewitt CLA
no flags Details | Diff
Demonstrates the fix (4.95 KB, application/octet-stream)
2007-08-08 11:01 EDT, Alex Blewitt CLA
no flags Details
patch to pass parameter as a VM argument (5.57 KB, patch)
2007-08-08 17:09 EDT, Brian Bauman CLA
no flags Details | Diff
patch (8.62 KB, patch)
2007-08-15 12:05 EDT, Brian Bauman CLA
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alex Blewitt CLA 2007-07-04 18:47:50 EDT
When running an IApplication (see attached) the PDE launcher automatically appends -pdelaunch. However, this argument isn't stripped out of the 'special arguments' with the result that it's visible to the runtime Eclipse application. This behaviour is not seen when launching from the command line startApp, or presumably from the command line (though I've not tried the latter).

This project has a simple IApplication (.launch included) that runs and prints out 'application.args'. When running from a PDE supplied launch (Launch as Eclipse Application, with selected class in -application) it's clear that it's passing the -pdelaunch to the runtime:

Hello RCP World![-pdelaunch]

Run from a startApp launch, you see:

Hello RCP World![]

This basically means you can't test IApplications from PDE that expect arguments. You can run as an OSGi framework, so it's just this extraneous flag that's preventing it from working, so I'm flagging as major.
Comment 1 Alex Blewitt CLA 2007-07-04 18:48:51 EDT
Created attachment 73076 [details]
Demonstration project that -pdelaunch is being passed through

NB this isn't a runnable bundle, it's a PDE project you can load in.
Comment 2 Alex Blewitt CLA 2007-07-04 18:57:10 EDT
If -pdelaunch is obsolete, it should be put into CommandLineArgs in the org.eclipse.equinox.internal.app as something that gets consumed. If it's not obsolete (and it's used elsewhere) then it probably shouldn't be supplied when an -application launch is invoked.
Comment 3 Alex Blewitt CLA 2007-07-04 19:09:25 EDT
OK, so it's added in org.eclipse.pde.ui.launcher.EclipseApplicationLaunchConfiguration:

    		// necessary for PDE to know how to load plugins when target platform = host platform
    		// see PluginPathFinder.getPluginPaths()
     		programArgs.add("-pdelaunch"); //$NON-NLS-1$           

     		
 NB it appears twice; the first time (when features aren't used) it always adds it; the second time, presumably when features are used, it only does this if the pluginMap containse the PDECore.PLUGIN_ID.
 
     		// necessary for PDE to know how to load plugins when target platform = host platform
    		// see PluginPathFinder.getPluginPaths()
    		if (pluginMap.containsKey(PDECore.PLUGIN_ID))
    			programArgs.add("-pdelaunch"); //$NON-NLS-1$	
 
 Why is this not done in the other location? If we're launching an IApplciation, and we don't have PDE installed, do we even need this as an argument?
Comment 4 Alex Blewitt CLA 2007-07-05 06:18:05 EDT
NB running the app without the PDE plugin makes the problem go away. However, since 'run as application' automatically selects all workspace plugins and target plugins, and people are always going to be using PDE to put/test this stuff together, it's still a bit of a blocker. Ideally the flag should be filtered from the args passed to the application, because otherwise it makes testing inside and outside of Eclipse different, which is going to lead to all sorts of problems for others ...
Comment 5 Brian Bauman CLA 2007-08-06 14:52:58 EDT
Alex, we can't strip out that argument because PDE itself actually uses the argument.  This is why we try to only include the argument when the pde plug-ins are included in the launch.  When using features, it is more time consuming to parse features to see if the plug-in is included, therefore we just include it just in case.

This also does not stop people from testing IApplications that expect arguments.  You can still specify arguments in the program argument section of the launcher and have them picked up in the IApplication.
Comment 6 Alex Blewitt CLA 2007-08-06 16:27:00 EDT
I'm not saying it shouldn't be passed, but I am saying that it should be invisible to the application. If you're expecting args[0] to be a number, and use Integer.parseInt(args[0]), then it barfs because although  you have '1234' in the arguments, it is supplied with -pdelaunch.

Furthermore, there's no (obvious) benefit to the runtime instance for having this flag.

The point is that having an extra argument, which you can't get rid of and exists in args[0], makes it impossible to write or debug IApplications that take arguments, because they'll all have to deal with an off-by-one error if -pdelaunch is specified.

"This also does not stop people from testing IApplications that expect
arguments.  You can still specify arguments in the program argument section of
the launcher and have them picked up in the IApplication."

I'd like to see you write an IApplication that performs the unix 'copy' command ...

Comment 7 Alex Blewitt CLA 2007-08-06 17:59:47 EDT
Why does it even need to be supplied as an argument? Why not use a non-invasive way of passing the information e.g. -Declipse.pde.launch=true?
Comment 8 Wassim Melhem CLA 2007-08-07 01:08:42 EDT
The -pdelaunch program argument has been around longer than I have on PDE.  So, unfortunately, we will always need to maintain it (for backward compatibility for target platforms <= 3.3.x).

However, going forward, e.g. for target platforms >= 3.4, I think it would be better/cleaner to pass a VM argument to the runtime Eclipse instance instead of a program argument.
Comment 9 Alex Blewitt CLA 2007-08-07 04:00:41 EDT
Well, putting in a fix so that the -pdelaunch isn't visible to the Equinox application is desirable, then. Note that many arguments get consumed in this way (e.g. -console, -noExit etc.) -- it's just -pdelaunch that isn't taken out. It doesn't need to be taken out of the eclipse args, just what's passed into the application.args that's consumable by an IApplication.
Comment 10 Jeff McAffer CLA 2007-08-07 20:46:28 EDT
To do what you suggest would essentially require application definers to describe their command line to the runtime and then have the runtime parse the command line and include only what was deemed interesting on to the application.  without that information the best any given layer (including the framework and runtime) can do is strip off the args it knows about/consumes (the known knowns as it were) and pass on the rest.  The framework and runtime don't know anything about -pdelaunch so, as you see, ignore it.

As suggested, the best way forward seems to be using a system property.
Comment 11 Alex Blewitt CLA 2007-08-08 05:49:43 EDT
I agree that command-line handling is difficult at best, and that a system property is probably good going forwards (incidentally, I suspect that the same argument can be applied to -dev as well). However, 3.3 is the first time that applications have been written using the IApplication interface along with arguments, and the '-dev' flag is not seen in the arguments (which I believe is also supplied internally by PDE's launch). The question I have is why '-dev' is understood but '-pdelaunch' isn't? Ultimately, they're all hard-coded anyway, and I don't think it necessary to provide command-line flag registries etc; I suspect that ultimately, it would be desirable for all of these flags to be passed by system property instead of a command-line argument anyway.

The thing is that as the IApplication interface takes off, you're going to see more and more places where exact command-line handling is required e.g. in the 'tar' type example. And as people will be running with PDE installed during development, and that the default is for all workspace enabled plugins, it puts a damper on developing any command-line arguments using this method. There's already code in place in some areas to strip this arg (e.g. see GenericServer mungeArguments method) and it would be a shame if this had to be replicated by anyone wanting to write a command-line argument-processing application:

http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.ecf/plugins/org.eclipse.ecf.provider/src/org/eclipse/ecf/provider/app/GenericServer.java?root=Technology_Project&view=co

Instead of trying to define a generic solution, can't we treat -pdelaunch as a special case and have it removed from the arguments passed to an IApplication in the same way that -dev is?
Comment 12 Jeff McAffer CLA 2007-08-08 08:34:53 EDT
the difference is that the module that does the stripping "knows" about -dev as -dev is actually consumed by the framework.  the framework does not, in general, know about -pdelaunch.  That is a PDE thing.  Of course, by virtue of being told, it becomes one of our known knowns and we could strip it.  What about the next plugin that looks for a command line arg? -showlocation looked for by the UI for example?  If we start stripping these then those plugins may actually be upset depending on how they look for their args.  

Oh and there is a reverse situation.  Say you are running an app that wants an arg called -showlocation and the UI is not installed (i.e., there is no actual collision?)  I had this the other day with -install.  Apparently this is consumed somewhere but yet I want to use if for my app.  I suspect that its consumption is a legacy thing from the early update days.  so if the runtime starts consuming other people's arguments then it is vulnerable to a) those other plugins not being present and the consumption being unwarranted and b) those plugins changing and no longer using said argumnts.

The failure here is that random plugins are looking for command line args.  As has been pointed out, that style came about many years ago and before it became "popular" / easy to use system properties.  the need to pass something in in turn is a sign of a failure to have/use some reasonable configuration mechansims (e.g., preferences, config admin, ...) to set the scene for an execution.  

we are attempting to do something about the latter.  in the mean time, bundles can /should do the system property thing.  as for stripping the things we know about now

Do not misinterpret these comments as "not wanting to do something" or even disagreement that it is a problem.  Quite the contrary.  The challenge is what to do.  What we really need is someone (Alex?) to open bug reports against the various plugins that use command line args for configuration and request that they change to use system props.  This change can be done for 3.4

It would also be good to review the framework's consumption of args to ensure that we are consuming only the bare minimum.
Comment 13 Alex Blewitt CLA 2007-08-08 09:47:28 EDT
I understand the comments, and I'm not interpreting them as negativity or not wanting to fix. I also agree that where possible, the command line stuff shouldn't be stripped out by the platform.

However, PDE is somewhat of a special case. It must be in the target platform; if you didn't have PDE, you wouldn't be launching as an Equinox or Eclipse Application framework. :-) Furthermore, this arg gets added whenever PDE detects that PDE is in the target platform. It's not something you can change. Granted, if there were a '-pdelaunch' that were being added in the .launch configuration that I could remove myself, I'd grumble a bit, and then remove it. 

But the fact is that I have no control over when -pdelaunch is added; and I can't forcibly remove it. In fact, the only way I can remove it is by disabling the PDE plugin from running in my app, which is less than desirable; it means that instead of tracking all workspace and installed plugins, I have to go out of my way to take out PDE to take this arg out.

Up until now, the focus on the platform hasn't been in the command-line arena, but it's going to start going there with IApplication. This will be one of the first problems that people hit. Furthermore, PDE isn't an IApplication, so it doesn't need to know about it at that level, even if it consults the Platform.getApplicationArgs() (or whatever the call is; I forget off the top of my head). What I'm suggesting is that as well as moving towards a property-based mechanism for supplying commands (instead of tacking on args) for the 3.4 timeframe, we look at a workaround for 3.3.1 that can strip -pdelaunch specifically from IApplication's use of args. I believe that we will be able to do that without affecting PDE's behaviour generally, seeing as Platform.args (or osgi.args?) will still have -pdelaunch, but we just don't have to pass that in to the application args that get set up in the IAppication's appicationcontext.getArgs("appication.args").

(Of course, this plan goes out of the window if that is how the platform args are set up ...)

What I'm really trying to argue for is a special-casing patch for -pdelaunch specifically for the 3.3.1 timeframe, as well as a proper solution for the 3.4 timeframe. There's enough difficulty in getting applications up and running as is, and it would be good to get as many issues like this fixed if possible for the 3.3.1 service pack (incidentally, is there a document that says when that will happen?)

Alex.
Comment 14 Alex Blewitt CLA 2007-08-08 10:58:11 EDT
Created attachment 75675 [details]
Patch that hacks the fix

This patch hacks the -pdelaunch arg, so that it's not visible to any IApplciations that are launched. Note that Platform.getArgs() still shows it up, so that PDE will continue to work as before.

NB Yes, I know it's ugly; it's just to demonstrate the idea :-)
Comment 15 Alex Blewitt CLA 2007-08-08 11:01:43 EDT
Created attachment 75676 [details]
Demonstrates the fix

This project demonstrates the fix; run it on a normal Eclipse install, and the printout shows [-pdelaunch] for both. Run it with the patch (attached) and it only shows [-pdelaunch] for the Platform case.

I know that -pdelaunch is a special case, but I don't think it necessarily opens up a floodgate of others (e.g. -showlocation etc.), because PDE doesn't automatically put any of those others on; and if it did put them in the arguments box, you could remove them. That's not the case with -pdelaunch; if PDE detects that PDE is in the target platform, it puts this on outside of your control.

I hope that this (or a cleaner version) can be considered for 3.3.1.
Comment 16 Jeff McAffer CLA 2007-08-08 13:31:48 EDT
fwiw I would be much happier with a fix in PDE to use a system property.  That is sustainable and in the right direction.  Every time we add one of these special case things it comes back to bite us somehow later on
Comment 17 Wassim Melhem CLA 2007-08-08 13:50:15 EDT
I agree with Jeff.  I don't feel comfortable having PDE-specific knowledge in the runtime.
Comment 18 Brian Bauman CLA 2007-08-08 17:09:48 EDT
Created attachment 75708 [details]
patch to pass parameter as a VM argument

Attaching the patch for M2.  This will pass a VM argument instead of a program argument, which PDE will use to determine if it is in "pde dev mode".
Comment 19 Alex Blewitt CLA 2007-08-08 18:50:59 EDT
I agree that a system property for 3.4 would be a good way to go. However, what will happen in the case of 3.3, or do we just say that nothing's going to be addressed for 3.3.1? If so, I think we impact the adoption of IApplication for command line applications by about a year.
Comment 20 Wassim Melhem CLA 2007-08-08 19:26:30 EDT
Brian, I glanced at the patch, and unless I am missing something, it is insufficient.

It does not continue to pass -pdelaunch for targets that contain org.eclipse.pde.core whose version is <= 3.3.0.  
Comment 21 Alex Blewitt CLA 2007-08-10 18:19:28 EDT
Given the need to pass -pdelaunch for targets <= 3.3.0, can the workaround I suggested in the first patch be applied to the newly-introduced Equinox application handler in the meantime for 3.3.1, regardless of what state this takes in its final incantation for 3.4? I don't see the patch I submitted as being deprecated by the other one at all; in fact, I see them as complementary. One is fixing the immediate problem fir the IApplication case, the other is a strategic fix for the future.
Comment 22 Brian Bauman CLA 2007-08-13 10:42:30 EDT
Alex, the reason we marked the first patch as deprecated, is because we don't want to start complicating the runtime with tooling specific information.  This really is a tooling problem which should not be fixed in the runtime.

I will rework the patch so that we can apply it to PDE for both 3.3.1 and 3.4M2.
Comment 23 Brian Bauman CLA 2007-08-15 12:05:04 EDT
Created attachment 76131 [details]
patch

backwards compatible patch
Comment 24 Brian Bauman CLA 2007-08-29 13:19:41 EDT
Patch is applied to both 3_3 Maintenance and HEAD to be included in 3.3.1 and 3.4M2 and after.