Bug 309701 - Persisted launch configuration file changes unnecessarily
Summary: Persisted launch configuration file changes unnecessarily
Status: RESOLVED FIXED
Alias: None
Product: PDE
Classification: Eclipse Project
Component: UI (show other bugs)
Version: 3.6   Edit
Hardware: PC Mac OS X - Carbon (unsup.)
: P3 normal with 3 votes (vote)
Target Milestone: 3.7 M7   Edit
Assignee: Curtis Windatt CLA
QA Contact:
URL:
Whiteboard:
Keywords: contributed
: 329308 (view as bug list)
Depends on:
Blocks: 352808
  Show dependency tree
 
Reported: 2010-04-19 11:56 EDT by Peter Nehrer CLA
Modified: 2011-07-25 15:30 EDT (History)
10 users (show)

See Also:
curtis.windatt.public: review+


Attachments
An OSGI launch config when first created. Note errors (5.48 KB, application/octet-stream)
2011-03-07 10:04 EST, Dean Roberts CLA
no flags Details
An OSGI launch config after opening in UI a second time (5.46 KB, application/octet-stream)
2011-03-07 10:05 EST, Dean Roberts CLA
no flags Details
Patch for 3.7 (13.60 KB, patch)
2011-03-10 10:04 EST, Dean Roberts CLA
no flags Details | Diff
New patch for which also addresses plug-in ordering problem (21.29 KB, patch)
2011-03-11 13:21 EST, Dean Roberts CLA
curtis.windatt.public: iplog+
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Peter Nehrer CLA 2010-04-19 11:56:01 EDT
It seems that whenever I touch a PDE-based launch configuration in the launch dialog, the persisted *.launch file shows VCS changes even though I haven't made any changes. The culprits appear to be the workspace_bundles and target_bundles stringAttribute elements -- even though the set of selected plugins is unchanged, their ordering seems to be completely arbitrary.

This makes it very difficult to tell if someone changed a configuration and how (and why).

Would it be possible to impose some sort of ordering, or simply maintain whatever ordering there might be?

E.g., instead of HashMap use a SortedMap or at least LinkedHashMap.

See org.eclipse.pde.internal.launching.launcher.BundleLauncherHelper.getTargetBundleMap(ILaunchConfiguration, Set, String) and org.eclipse.pde.internal.launching.launcher.BundleLauncherHelper.getWorkspaceBundleMap(ILaunchConfiguration, Set, String).
Comment 1 Ankur Sharma CLA 2010-04-19 12:02:11 EDT
On which build you are facing this problem?
Comment 2 Chris Aniszczyk CLA 2010-04-19 12:03:40 EDT
This has been a problem in PDE for a long time.

There's another bug open for this issue but I can't find it now.

A SortedMap would fix this problem.
Comment 3 Curtis Windatt CLA 2010-04-19 12:11:21 EDT
Can you provide a reproducible test case.  The launch config file should only change if changes were made in the launch configuration dialog and those changes were saved.

Looking at the launcher code, while we use a hashmap to load up the list of bundles from the config, we are using an array when saving them.  The order in the array comes from the tree.

What version are you using?  We made some changes to the Eclipse launch configuration code to order the attributes specifically to avoid these kind of problems (bug 252576).  It's possible that we have to do some similar work in the OSGi launcher.
Comment 4 Darin Wright CLA 2010-04-19 12:12:21 EDT
Note, the debug platform fixed bug 295843 in 3.6 M6, which might just fix this.
We now save map/set/list attributes in a consistent order.
Comment 5 Curtis Windatt CLA 2010-04-19 12:24:58 EDT
(In reply to comment #4)
> Note, the debug platform fixed bug 295843 in 3.6 M6, which might just fix this.
> We now save map/set/list attributes in a consistent order.

All the attributes relevant to this bug use comma separated Strings instead of a map/set/list.
Comment 6 Peter Nehrer CLA 2010-04-19 13:04:12 EDT
I'm using 3.6M6. This problem is reproducible with OSGi and JUnit Plug-in Tests. The test scenario:

1. Create a launch configuration (OSGi or JUnit Plugin-Test).

2. Check a workspace plugin (one of a few in the workspace). Uncheck everything else.

3. Add Required Plugins (to a have a subset of target platform).

4. Specify shared file (in Common tab) to save the launch config in workspace.

5. Apply and Close.

6. Copy the launch config file (e.g., "Copy of New_configuration.launchx").

7. Click Run->Run Configurations... and select your confiuguration. Observe that the Apply button remains disabled, but if your *.launch file were version-controlled then you'd see that there are changes to it at this point.

8. Close the lauch dialog.

9. Compare your original *.launch file with the copy. Observe changes. E.g.,

Original:
<stringAttribute key="target_bundles" value="org.eclipse.emf.common@default:default,org.eclipse.equinox.app@default:default,org.eclipse.ant.core@default:default,org.eclipse.osgi.services@default:default,org.eclipse.emf.ecore@default:default,org.eclipse.core.runtime@default:true,org.eclipse.core.contenttype@default:default,org.eclipse.core.filesystem.macosx@default:false,org.eclipse.core.resources@default:default,org.eclipse.core.runtime.compatibility.registry@default:false,org.eclipse.equinox.common@default:default,org.eclipse.core.expressions@default:default,org.eclipse.core.filesystem@default:default,org.eclipse.emf.ecore.xmi@default:default,org.eclipse.core.variables@default:default,org.eclipse.core.runtime.compatibility.auth@default:default,org.eclipse.equinox.preferences@default:default,org.eclipse.core.jobs@default:default,org.eclipse.osgi,org.eclipse.equinox.registry@default:default,javax.servlet@default:default"/>


Copy:
<stringAttribute key="target_bundles" value="org.eclipse.emf.common@default:default,org.eclipse.equinox.app@default:default,org.eclipse.ant.core@default:default,org.eclipse.osgi.services@default:default,org.eclipse.emf.ecore@default:default,org.eclipse.core.runtime@default:true,org.eclipse.core.contenttype@default:default,org.eclipse.core.filesystem.macosx@default:false,org.eclipse.core.resources@default:default,org.eclipse.core.runtime.compatibility.registry@default:false,org.eclipse.equinox.common@2:true,org.eclipse.core.expressions@default:default,org.eclipse.core.filesystem@default:default,org.eclipse.emf.ecore.xmi@default:default,org.eclipse.core.variables@default:default,org.eclipse.core.runtime.compatibility.auth@default:default,org.eclipse.equinox.preferences@default:default,org.eclipse.core.jobs@default:default,org.eclipse.osgi@-1:true,org.eclipse.equinox.registry@default:default,javax.servlet@default:default"/>
Comment 7 Peter Nehrer CLA 2010-07-29 09:47:09 EDT
This continues to be a problem, btw. Any chance this can be fixed in Helios SR1?
Comment 8 Curtis Windatt CLA 2010-07-29 09:57:21 EDT
I would have expected bug 252576 to have fixed the problem for both Eclipse configs and Plug-in Test configs.  It might be appropriate to add the same sort of fix to the OSGi configs.  We will consider for 3.7, but committer time is limited.  If the fix is well contained, we can consider for 3.6.1.
Comment 9 Benjamin Cabé CLA 2010-08-04 12:11:49 EDT
isn't it a dupe of bug 231099?
Comment 10 Peter Nehrer CLA 2010-08-04 12:17:10 EDT
Not really. This happens even when you make no changes at all. Just selecting the launch config in the dialog does the trick.
Comment 11 David Erickson CLA 2011-03-05 05:11:33 EST
Would love to see this get fixed, its been a thorn in my side for the last year working with PDE.

Also as a potential side effect, frequently when I go just 'look' at an OSGi launch configuration without changing anything, then navigate to another launch configuration it asks me to save changes, even though I know without a doubt I haven't changed anything.  Could that be a related issue? In any case, if you change nothing, your launch shouldn't be asking you to save it.
Comment 12 Brandon Heller CLA 2011-03-07 05:05:16 EST
I have the same issue, even when I haven't made changes to the set of launch components, presumably because my system (OS X) had some different ordering than the system on which the launch config file was created.

If you use git, it means you cannot use git commit -a, or to do so you need to 'git stash save', do stuff, and then 'git stash apply' to restore the change.  You don't want to just ignore this file in the VCS, because when the config is changed you wouldn't update it!

SortedMap seems like a simple fix that would save lots of time...
Comment 13 Dean Roberts CLA 2011-03-07 10:02:48 EST
When I run the steps from comment 6 in 3.7 I see similar yet not identical behaviour to what is being reported.  Given the age of the defect, I don't know if this is different than what was originally observed, or if one wrong behaviour has been replaced by another.

I create the new OSGI launch config using the steps as described in steps 1 - 5, inserting a step where I rename the launch config before setting its shared location.  I find that the .launch file that is create has a couple of errors in it.

A) The configLocation value written is the original generated configuration name "New_configuration (1)".
B) In the target bundles value, all of the bundles are listed as default:default

When I complete steps 6-8, the original .launch file is overwritten and the errors are corrected.

A) The configLocation value is written with whatever name I had really used for the config
B) Several (but not all) of the target bundle values have their default:default replaced with different values such as -1:true, 2:true and 3:true

Running steps 6-8 again does not introduce any new differences in the .launch file.  That is, the file remains identical to what was re-written the first time steps 6-8 are executed.

I could only reproduce this behaviour for OSGI plugins I can not seem to reproduce the behaviour for JUnit Plugin test launch configs.  Although I do notice that for these launch configs the configLocation never contains the shared project location or config name.  I don't know if this is expected behaviour or not ... but it seems odd to me.  For JUnit Plugin test launch configs the configLocation value is ${workspace_loc}/.metadata/.plugins/org.eclipse.pde.core/pde-junit"

I'm wondering if other people are seeing this.  That is, currently in 3.7 the change is NOT a result of an ordering problem, but the result of a bug that is causing the .launch file to be incorrectly generated the 1st time, and correctly generated the next time the launch config is touched.

I'll attach the two OSGI launch config files for people that may wish to take a look.
Comment 14 Dean Roberts CLA 2011-03-07 10:04:31 EST
Created attachment 190553 [details]
An OSGI launch config when first created.  Note errors
Comment 15 Dean Roberts CLA 2011-03-07 10:05:20 EST
Created attachment 190554 [details]
An OSGI launch config after opening in UI a second time
Comment 16 Dean Roberts CLA 2011-03-07 10:12:40 EST
I confirmed that when creating the OSGI launch config the first time, all of the selected bundles in the UI page have Start Level and Auto-start set to default.  So this matches what is written to the .launch file the first time.

When opening the launch config the second time, some bundles in the UI show specific run levels and Auto-start settings, consistent with what is being written to the .launch file the 2nd time.

I can only assume the expected behaviour is the correct (?) Start levels and Auto-Start values that end up being written the 2nd time should have been written the fist time the file is created (along with the correct profile name)
Comment 17 Dean Roberts CLA 2011-03-07 15:42:57 EST
After some debugging it appears the issue for the configLocation issue is in (or around) ConfigurationAreaBlock.performApply().  This method takes the configLocation value directly from the Location text field on the settings tab.  However, this field is NOT updated until later in the apply AFTER the .launch file is written to disk.

This is why the correct value is used in subsequent calls.  Either the value from the post save is used, or, if the dialog is closed and reopend the location is reconstituted from the Name (which comes from the file system).

I'm still thinking about the correct way to fix this.  Not sure why the value is not updated as the text field is edited.  There is a callback already set up on the event.
Comment 18 Dean Roberts CLA 2011-03-08 14:29:58 EST
I have a potential fix for the configLocation problem.  I won't attach a patch until I also have a fix for the run levels and auto start issue.

On the run level problem I have some more information as to what is going on.

When the Bundles tab is originally opened and contains all the bundles code in OSGIBundleBlock.initExternalPluginsState() is running and is making sure the correct run levels and auto start values are recorded in the UI for all bundles.

However, when the Deselect All button is pressed and bundles are added back manually and with the add required bundles button, the model and UI are being populated with default:default for run levels and auto start values.

When the launch config is saved these default:default values are being persisted.

When the launch config is opened the 2nd time the code in OSGIBundleBlock.initExternalPluginsState() is run again causing the bogus default:default values to be replaced with the correct run levels and auto start values.  This change in the model causes the model to be saved automatically, leading to the change.

It would seem that initial save should include the correct values.  However, in addition, is it really correct behaviour that the launch config dialog is doing a performApply() on its own initiative?  Perhaps this is a design choice and is correct.
Comment 19 Dean Roberts CLA 2011-03-10 10:04:07 EST
Created attachment 190866 [details]
Patch for 3.7

A patch that fixes the configLocation and the bundle start level/auto start problem.  With this patch installed the test case included in this bug report no longer creates a 2nd erroneous save since the 1st save contains correct information.
Comment 20 Eric Jain CLA 2011-03-10 20:04:07 EST
Related to bug 329308?
Comment 21 Dean Roberts CLA 2011-03-11 13:18:29 EST
Plug 329308 can be considered a duplicate of this bug.

I will release a new patch shortly that will include a fix for the re-ording problem for target and workspace bundles.

Note that changes CAN still occur when the default target is changed if the targets really do have different bundles specified.  However, the target and workspace bundles will now be stored in alphabetical order so that changes should be minimal or non-existent depending on the situation.
Comment 22 Eric Jain CLA 2011-03-11 13:21:38 EST
*** Bug 329308 has been marked as a duplicate of this bug. ***
Comment 23 Dean Roberts CLA 2011-03-11 13:21:49 EST
Created attachment 191013 [details]
New patch for which also addresses plug-in ordering problem

Added new patch that addresses:
1) config location problem
2) run level and auto start problem
3) workspace and target bundle ordering problem
Comment 24 Curtis Windatt CLA 2011-03-11 14:47:50 EST
Patch is working great for me.  Can Eric, Peter and/or Brandon try out the patch and ensure that it works well for your cases as well?
Comment 25 Curtis Windatt CLA 2011-03-14 10:46:45 EDT
Fixed in HEAD.
Comment 26 David Erickson CLA 2011-04-06 02:59:40 EDT
(In reply to comment #25)
> Fixed in HEAD.

Patch looks great, thanks!