Community
Participate
Working Groups
Plug-ins store their preferences in org.eclipse.core.runtime/.settings. If we copy these folders to a new workspace a local target platform breaks, as it references to a file in /home/vogella/workspace/eclipse.platform/.metadata/.plugins/org.eclipse.pde.core/.local_targets This is exposed via Bug 432006.
New Gerrit change created: https://git.eclipse.org/r/96179
Gerrit change https://git.eclipse.org/r/96179 was merged to [master]. Commit: http://git.eclipse.org/c/pde/eclipse.pde.ui.git/commit/?id=f71da47594f7422fd84f1a8015a566dfd0d28166
Merging as this is M7 related and I want to test this on Wednesdays build
Verified in Eclipse SDK Version: Oxygen (4.7) Build id: I20170502-2000 OS: Linux, v.4.4.0-75-generic, x86_64 / gtk 3.18.9 After switching to a new workspace with the "Copy preferences" flag the resulting target platform is still valid.
This causes unit test errors. http://download.eclipse.org/eclipse/downloads/drops4/I20170502-2000/testresults/html/org.eclipse.pde.ui.tests_ep47I-unit-cen64-gtk2_linux.gtk.x86_64_8.0.html
I reverted the change http://git.eclipse.org/c/pde/eclipse.pde.ui.git/commit/?id=bc213cfc61329a26fda225116635ee78edebdb94
New Gerrit change created: https://git.eclipse.org/r/96270
New Gerrit change created: https://git.eclipse.org/r/96269
https://git.eclipse.org/r/#/c/96270/ contains a fix for the unit tests. @Vikas, any objections to apply it again?
>>@Vikas, any objections to apply it again? Can you also do some manual testing in and around this area?
(In reply to Vikas Chandra from comment #10) > >>@Vikas, any objections to apply it again? > > Can you also do some manual testing in and around this area? I tested: - Scenario with preference copy from Bug 432006 now works - Target definition creation still works - Deletion still works - Setting no target and restarting still works
Vikas, I plan to commit the change today, if I do not hear something against it from you.
I plan to try out this patch in the next 2 hours. If you don't hear anything from me, you can go ahead with the fix.
(In reply to Vikas Chandra from comment #13) > I plan to try out this patch in the next 2 hours. If you don't hear anything > from me, you can go ahead with the fix. Thanks.
I find the new code very confusing. The method is to initialize a default target and most of the start, we try to fix invalid target. I suggest something String memento = PDECore.getDefault().getPreferencesManager().getString(ICoreConstants.WORKSPACE_TARGET_HANDLE); boolean updated = updateInvalidTargetMemento(memento); if(updated) memento = PDECore.getDefault().getPreferencesManager().getString(ICoreConstants.WORKSPACE_TARGET_HANDLE); updateInvalidTargetMemento could look something like private boolean updateInvalidTargetMemento(String memento) { boolean updated =false; PDEPreferencesManager preferenceManager = PDECore.getDefault().getPreferencesManager(); // check if preference entry is valid if (memento != null ) { ITargetHandle handle; try { handle = getTarget(memento); if (!handle.exists()) { // preferences points to invalid target definition remove preference entry preferenceManager.setValueOrRemove(ICoreConstants.WORKSPACE_TARGET_HANDLE, preferenceManager.getDefaultString(ICoreConstants.WORKSPACE_TARGET_HANDLE)); preferenceManager.flush(); updated =true; } } catch (CoreException e) { PDECore.log(e); } catch (BackingStoreException e) { PDECore.log(e); } } return updated; updateInvalidTargetMemento could probably be better named as removeInvalidTargetMementoInPreference. Also TargetPlatformService is at the core of things, we should make minimum change as possible. I think this is good to go for 4.7 but it will be better if this is organized.
I hope comment#15 captures what was intended to fix this issue. In case it doesn't, the additional logic should be put in too.
(In reply to Vikas Chandra from comment #16) > I hope comment#15 captures what was intended to fix this issue. In case it > doesn't, the additional logic should be put in too. Thanks Vikas, great suggestion. I adjusted the Gerrit.
Which is the current gerrit patch?
(In reply to Vikas Chandra from comment #18) > Which is the current gerrit patch? https://git.eclipse.org/r/#/c/96270 I'm still testing the updated change.
(In reply to Lars Vogel from comment #19) > (In reply to Vikas Chandra from comment #18) > > Which is the current gerrit patch? > > https://git.eclipse.org/r/#/c/96270 Works fine for me. Let me know if I captured your suggestions correctly.
Lars, when you prepare a change for PDE you *must* run AllPDETests. Gerrit does not run all tests.
(In reply to Dani Megert from comment #21) > Lars, when you prepare a change for PDE you *must* run AllPDETests. Gerrit > does not run all tests. Thanks Dani, didn't know that about the PDE Gerrit. I see that my proposed change creates a test error. I move this out of M7.
This is what I do If I change pde.core, I run 1) AllPDETests 2) ApiToolsPluginTestSuite 3) ApiToolsTestSuite ( as Junit test and not plugin test) If change is pde.ui, I run 1) AllPDETests If change is in api.tools 1) ApiToolsPluginTestSuite 2) ApiToolsTestSuite ( as Junit test and not plugin test)
>>creates a test error. this was happening because when memento was empty string, removeInvalidTargetMementoInPreference returned false if (!removeInvalidTargetMementoInPreference(preferenceManager, memento)) { return; } ensured that the function returned instead of creating a default target. I have changed the code to if (removeInvalidTargetMementoInPreference(preferenceManager, memento)) { memento = preferenceManager.getString(ICoreConstants.WORKSPACE_TARGET_HANDLE); } and kept all of original line statements of the function. As I said in comment#15, we don't change anything in the original function. We just removeInvalidTargetMementoInPreference and if removed, memento is updated. Does the latest patch solve comment#0?
(In reply to Vikas Chandra from comment #24) > Does the latest patch solve comment#0? Thanks. Change looks good and solve the original problem. I leave it to you to merge it or not.
I believe that the change is safe but I will back it up with some more testing. If testing goes fine, I will merge it in 4.7
I will have a look at this later today.
Gerrit change https://git.eclipse.org/r/96270 was merged to [master]. Commit: http://git.eclipse.org/c/pde/eclipse.pde.ui.git/commit/?id=0b436397289d6ee9f3f403feb0605b0e085757e5
A minor issue, I was getting coreexception while setting empty platform. Fixed via http://git.eclipse.org/c/pde/eclipse.pde.ui.git/patch/?id=406d9d8e74d3db52c47969f2c8a1618584c4d3be Lars, It looks like I missed this check from your patch.
Lars, can you please verify comment#0
(In reply to Vikas Chandra from comment #30) > Lars, can you please verify comment#0 Verified with Eclipse SDK Version: Oxygen (4.7) Build id: I20170509-2000 OS: Linux, v.4.4.0-75-generic, x86_64 / gtk 3.18.9, WebKit 2.16.1
