Bug 253898 - [launch] LaunchManager removes launch configurations on project closed
Summary: [launch] LaunchManager removes launch configurations on project closed
Status: RESOLVED WONTFIX
Alias: None
Product: Platform
Classification: Eclipse Project
Component: Debug (show other bugs)
Version: 3.4   Edit
Hardware: PC All
: P3 normal (vote)
Target Milestone: ---   Edit
Assignee: Platform-Debug-Inbox CLA
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on: 245412
Blocks:
  Show dependency tree
 
Reported: 2008-11-05 07:23 EST by James Blackburn CLA
Modified: 2011-06-03 19:13 EDT (History)
4 users (show)

See Also:


Attachments
Patch to LaunchConfiguration (1.08 KB, patch)
2008-11-05 07:53 EST, James Blackburn CLA
no flags Details | Diff
Patch 2 (1004 bytes, patch)
2008-11-12 05:56 EST, James Blackburn CLA
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description James Blackburn CLA 2008-11-05 07:23:08 EST
Build ID: 3.4

Steps To Reproduce:
This problem looks to be related to bug245412 (nested projects), but looking at the code, it think it probably also existing for linked resources.

The problem is that each launch configuration is mapped to a single Eclipse IFile.  When a project is closed -- even if the launch configuration is still present in the workspace -- all the project's ILaunchConfigurations are removed from LaunchManager.

See LaunchManager.projectClosed(...) & LaunchManager.getLaunchConfigurations(...)
Comment 1 James Blackburn CLA 2008-11-05 07:53:36 EST
Created attachment 117063 [details]
Patch to LaunchConfiguration

I thought about patching the problem in LaunchManager, and this worked but the Common tab was still upset that the full path of the resource didn't exist in the workspace.

I've therefore modified LaunchConfiguration.getFile() to do the following:
 - If the IFile exists in more than one project, then attempt to return one from an open project
 - If no open project found, then return the first one discovered
 - Else return null as before

The result is that LaunchManager doesn't remove the ILaunchConfiguration, and the common tab always has a valid full path for it.
Comment 2 James Blackburn CLA 2008-11-11 17:03:20 EST
Adding dependency on 245412 as a fix there may fix this issue.  However a fix there is likely in the e4 timeframe whereas we see this for real in 3.4.
Comment 3 Martin Oberhuber CLA 2008-11-11 20:06:07 EST
James, could you please explain what's the expected behavior vs. observed behavior (also to simplify verification once this is fixed)?

Just looking at your summary - if a Launch is associated with a single project and that project is closed, I'd expect the Launch to go away. Is the bug that if the Launch is linked into a second (open) project, it is also removed but should remain visible? 

I'm not so sure if Launches should really be associated with Aliases. The mechanism of associating a Launch with a project, is by entering a "shared file" in the Common Tab. Since that "shared file" is a workspace path, I'd expect the Launch to react to changes of exactly that workspace path, regardless of whether that workspace path happens to be aliased somewhere else.

I'd assume that aliases of that workspace path might result in multiple copies of the Launch to become listed (which would not be desirable, but would be expected behavior in this case).
Comment 4 Mauro Molinari CLA 2008-11-12 03:26:50 EST
(In reply to comment #3)
> Just looking at your summary - if a Launch is associated with a single project
> and that project is closed, I'd expect the Launch to go away. Is the bug that

I agree with Martin's point of view. I find it very comfortable to have launch configurations removed when I close the associated project!! If I needed to launch that project again, I wouldn't have closed it!

> I'd assume that aliases of that workspace path might result in multiple copies
> of the Launch to become listed (which would not be desirable, but would be
> expected behavior in this case).

I agree.

Mauro.
Comment 5 James Blackburn CLA 2008-11-12 04:03:39 EST
Hi Martin,

Sorry if I was unclear, the concrete example of this problem is in the overlapping project case.

(In reply to comment #3)
> James, could you please explain what's the expected behavior vs. observed
> behavior (also to simplify verification once this is fixed)?

I have two projects:
/some_view/path/to/an/inner_project/...
&
/inner_project/.launches/debug.launch

'some_view' and 'inner_project' are workspace projects, and they overlap.  'inner_project' is a cdt project, with a launch configuration persisted in .launches.  I only use the outer 'some_view' project to aid in creating the 'inner_project' -- there are no natures on 'some_view'.

Now when I close 'some_view', my debug.launch disappears from the launch configuration dialog.  There's no way to way to get it to show (as it's persisted in the filesystem and not with the workspace metadata), and the launch manager really has removed it.

The reason is, is that the LaunchManager (rightly so, in my book) builds launch configs around filesystem locations -- hence avoiding the duplication problem.  LaunchConfigurations resolves their locations to IFiles when requested in the LaunchConfiguration.getFile() method which returns findFilesForLocation(getLocation())[0].

The result is that that the my launch disappears when some_view is closed even though inner_project is still open.  This happens because projectClosed() attempts to clean up all launch configs which were accessible from the project being closed -- ignoring the fact that some launches may still be available through other workspace paths.

> The
> mechanism of associating a Launch with a project, is by entering a "shared
> file" in the Common Tab. Since that "shared file" is a workspace path, I'd
> expect the Launch to react to changes of exactly that workspace path,
> regardless of whether that workspace path happens to be aliased somewhere else.

What CommonTab actually does is this:
		fSharedLocationText.setText(getDefaultSharedConfigLocation(configuration));
if(isShared) {
	String containerName = IInternalDebugCoreConstants.EMPTY_STRING;
	IFile file = configuration.getFile();
	if (file != null) {
		IContainer parent = file.getParent();
		if (parent != null) {
			containerName = parent.getFullPath().toOSString();
		}
	}
	fSharedLocationText.setText(containerName);
}

First it gets the LaunchConfiguration to turn it's location into an IFile with config.getFile(). Then it gets the full path of that IFile.  The result is that the Workspace path you see is generated on the fly from the launch's fs location.

> I'd assume that aliases of that workspace path might result in multiple copies
> of the Launch to become listed (which would not be desirable, but would be
> expected behavior in this case).

This isn't the case because Launches are unique to filesystem locations.

I think, fundamentally, this is one of these cases where the LaunchManager doesn't care about IResources or aliases (bug253912).  It only cares whether a launch contained at a given filesystem location is still accessible through the workspace.  If the launch is available (i.e. I can double click to open it) then it should show in the launch dialog.

Given the above, I think the supplied patch is a low-risk localised fix for the issues of the launch being removed when it shouldn't be. I've made LaunchConfiguration.getFile() return the first IFile found for getFilesForLocation() that exists in an open project.  If none is found, the first IFile found is returned as before.
Comment 6 Martin Oberhuber CLA 2008-11-12 05:29:32 EST
So, if I'm not mistaken, a workaround for your issue is: after closing outer_project (which removes the Launch), close and re-open inner_project (should make the Launch available again because LaunchManager listens to the open events). Right?

Your patch looks good, though I think it can be simplified (and brought closer to the original code) a little:

    if (files.length > 0) {
	for (int i = 0; i < files.length; i++) {
            // Bug253898 ensure that we return an IFile from an open project
	    if (files[i].getProject().isOpen()) {
                return files[i];
            }
        }
        return files[0]; //first closed project
    }
    return null;
Comment 7 James Blackburn CLA 2008-11-12 05:56:29 EST
Created attachment 117643 [details]
Patch 2

(In reply to comment #6)
> So, if I'm not mistaken, a workaround for your issue is: after closing
> outer_project (which removes the Launch), close and re-open inner_project
> (should make the Launch available again because LaunchManager listens to the
> open events). Right?

Well that _almost_ works.  It doesn't quite, because projectOpened(inner_project) => findLaunchConfigurations(project) => LaunchConfiguration.exists() => LaunchConfiguration.getFile().exists()

So even though the launch config is resolved, the exists check tests whether the first item returned from findFilesForLocation exists.  In this case the first IFile returned is the one from the closed outer project.

> Your patch looks good, though I think it can be simplified (and brought closer
> to the original code) a little:
> 
>     if (files.length > 0) {
>         for (int i = 0; i < files.length; i++) {
>             // Bug253898 ensure that we return an IFile from an open project
>             if (files[i].getProject().isOpen()) {
>                 return files[i];
>             }
>         }
>         return files[0]; //first closed project
>     }
>     return null;

Indeed, that is cleaner.  When I was writing I thought there might be other cases where you would want to select one IFile in preference to another.  In the CDT world it would be nice if the full path shown in the common tab contained the Project in which the launch is used, but in general I don't think there's no way to know this...

patch updated.
Comment 8 James Blackburn CLA 2008-11-21 07:43:40 EST
Any chance this small patch could be reviewed by a committer - potentially for inclusion in 3.4.x?  I think it's pretty unobtrusive and low risk fix...
Comment 9 Martin Oberhuber CLA 2008-11-21 07:50:42 EST
James, could you check if the following workaround works for you:

Window > Preferences > Run/Debug > Launching > Launch Configurations
  disable "Filter configurations in closed projects"
Comment 10 James Blackburn CLA 2008-11-21 08:01:58 EST
(In reply to comment #9)
> James, could you check if the following workaround works for you:
> 
> Window > Preferences > Run/Debug > Launching > Launch Configurations
>   disable "Filter configurations in closed projects"

I can confirm that this _isn't_ a workaround.  That setting only affects launch configurations persisted in the debug plugin's local state location i.e. launch configs not resident in the workspace tree. 
Comment 11 Martin Oberhuber CLA 2008-11-21 08:14:30 EST
So I guess that for Launch configs persisted in the workspace tree, it's a workaround to create a separate (non-nested) project just to hold the launch configs and move them from their original project into the separate non-nested one.

I'm not trying to push back on this fix (I'd like to see it just like you), but I'm trying to assess risk/benefit of a 3.4.x backport.
Comment 12 James Blackburn CLA 2008-11-21 08:18:19 EST
(In reply to comment #11)
> So I guess that for Launch configs persisted in the workspace tree, it's a
> workaround to create a separate (non-nested) project just to hold the launch
> configs and move them from their original project into the separate non-nested
> one.
> 
> I'm not trying to push back on this fix (I'd like to see it just like you), but
> I'm trying to assess risk/benefit of a 3.4.x backport.

That would work, but it's a pretty annoying / impractical workaround.  The reason we store launch configurations in the filesystem is so that they can be checked into source control.  Our users check-out outer project using CVS, and then run 'Import > Existing projects ...' to import inner project(s) into the workspace tree.  At this point they should be able to close outer project with no ill-effect.
Comment 13 Darin Wright CLA 2008-11-21 12:22:14 EST
The patch will not work in 3.5 (HEAD). We have re-written launch configurations to support EFS (perpahs the patch is against 3.4?). I'd prefer to have this feature tried/tested in 3.5 rather than release to a maintenance build.
Comment 14 Darin Wright CLA 2008-11-21 12:28:19 EST
Actually, it would be helpful for someone to test this setup/scenario in 3.5 to see what happens. I wonder if there will be 2 copies of the launch configuration since we now base shared configs on IFiles. In this case there are two IFiles, each pointing to the same underlying storage.
Comment 15 Pawel Piech CLA 2011-06-03 19:13:47 EDT
Please reopen if still a problem.  Based on last comment, it looks like 3.5 changed things enough.