Bugzilla – Full Text Bug Listing
| Summary: | [patch] Target platform misses bundle after version of workspace bundle gets changed | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | [Eclipse Project] PDE | Reporter: | Markus Keller <markus.kell.r> | ||||||||
| Component: | UI | Assignee: | Vikas Chandra <Vikas.Chandra> | ||||||||
| Status: | VERIFIED FIXED | QA Contact: | |||||||||
| Severity: | major | ||||||||||
| Priority: | P3 | CC: | carsten.pfeiffer, curtis.windatt.public, daniel_megert, Ed.Merks, jonah, jvdneste, Lars.Vogel, markus.kell.r, tjwatson, Vikas.Chandra | ||||||||
| Version: | 4.6 | Flags: | curtis.windatt.public:
review+
markus.kell.r: review+ |
||||||||
| Target Milestone: | 4.6.2 | ||||||||||
| Hardware: | All | ||||||||||
| OS: | All | ||||||||||
| See Also: |
https://git.eclipse.org/r/54491 https://git.eclipse.org/r/84311 https://git.eclipse.org/c/pde/eclipse.pde.ui.git/commit/?id=a67e668fa367982ef6272c38fa44d81488e412e7 |
||||||||||
| Whiteboard: | |||||||||||
| Bug Depends on: | 449102 | ||||||||||
| Bug Blocks: | 498299 | ||||||||||
| Attachments: |
|
||||||||||
|
Description
Markus Keller
New Gerrit change created: https://git.eclipse.org/r/54491 Solutions that remove only some bundles from the state will never work right. Resolver#setSelectionPolicy(..) can be used to teach the resolver about PDE's ".qualifier" qualifier. https://git.eclipse.org/r/54491 reverts the second fix for bug 449102 and adds such a comparator. In my local testing, it worked fine for bug 449102 comment 12 and related scenarios. However, I'm not sure if we also need to duplicate org.eclipse.osgi.internal.module.VersionHashMap#compare(V, V) or parts of it. Patch set 2 of https://git.eclipse.org/r/54491 addresses situations where suppliers have the same name but different versions (newer version wins, and ".qualifier" wins over any other qualifier segment), and when they have different numeric bundle ids. In static cases where neither bundle versions nor wiring constraints are changed after startup, this patch works fine However, when I perform bug 449102 comment 12 point 5, the PDE classpath container doesn't pick up the org.eclipse.pde.core version 3.10.200 that would be available from the target platform. After a restart, everything is fine again. I don't know why the classpath container doesn't get refreshed in that scenario. Either someone performs version calculations outside of PDEState and wrongly decides to drop this specific change, or else we're lacking a trigger that explicitly refreshes the container when the version range of a bundle requirement gets changed. Although this problem sounds similar to the problem I described in comment 0, I think the solution in https://git.eclipse.org/r/54491 is closer to the correct solution than the interim fix we have in master. With the Gerrit, we're just missing some required updates. With master, the PluginModelManager#addWorkspaceBundleToState(..) method permanently loses some bundles from the target platform and will never be able to recover from that loss (except via a restart). IMO, master is good enough for M2 and touching this area is risky, so I'm moving this bug to M3 (and assign it to the inbox, since I'm not currently working on it). Created attachment 259269 [details]
Tentative fix.
Markus, does this fix work for you? Can you please check?
Moving to M6 (PDE has quite a few problems with resolving bundles in a runtime workspace (bug 332112, bug 488694), so I don't really trust setups where the runtime works with bundles that are also checked out in my host workspace. That rules out scenarios with the org.eclipse.pde.core, since that's where the fix will probably go.) Steps adapted from bug 449102 comment 17: - have a host workspace that contains org.eclipse.pde.core, but not any project from the eclipse.platform.runtime repo - launch new runtime workspace - clone git://git.eclipse.org/gitroot/platform/eclipse.platform.runtime.git - import all projects in /bundles => In Package Explorer, "org.eclipse.core.runtime > Plug-in Dependencies" correctly lists org.eclipse.core.contenttype from the workspace as dependency. - open /org.eclipse.core.contenttype/META-INF/MANIFEST.MF - change Bundle-Version to 1.5.100.qualifier - Save => Bug: org.eclipse.core.runtime still doesn't resolve the org.eclipse.core.contenttype from the target platform. A restart solves the issue (but should not be required). When you then revert the Bundle-Version to 3.5.100.qualifier, the PDE classpath container correctly switches to the workspace project again. https://git.eclipse.org/r/54491 fixes issue in comment#6 but there are some intermittent API tools test failure ( I think more because of test related issue than anything else). The fix looks to be in the right direction but needs some more investigation. I'm looking at it. Created attachment 260882 [details]
test code
Although the test run fine with gerrit patch 54491/3, however there is an issue.
With the patch attached, when version of org.eclipse.core.contenttype is changed to 1.x, the target bundle is taken but when org.eclipse.core.contenttype is changed back to 3.x, the workspace bundle is not taken. Running test class \org.eclipse.core.runtime\src\org\eclipse\core\internal\preferences\legacy\maincla.java will back this statement.
Just an observation ( I am *not* suggesting this change)
54491/3
+
if (isActive) {
targetReloaded(null);
in PluginModelManager::handleChange solves all the issues.
1.5.100.qualifier in org.eclipse.core.contenttype
- compiles and picks the target bundle
3.5.100.qualifier in org.eclipse.core.contenttype
- compiles and picks the workspace bundle
This also keeps bug 449102 fixed.
So effectively PluginModelManager::handleChange should change the PDE state for the changed bundle id so that it is in similar state as creating the state afresh ( without having to reload the target).
(In reply to Vikas Chandra from comment #10) That sounds like you're on the right track. It looks like the whole "Map<String, LocalModelEntry> fEntries" in PluginModelManager disregards versions, so everything that only compares bundle ids when reading/updating that table will be lossy. Something like this ( instead of targetReloaded(null) would work too
if (isActive && newID != null && newID.equals(oldID)) {
fEntries.remove(newID);
fState.removeBundleDescription(desc);
for ( int i=0 ; i < fExternalManager.getAllModels().length;i++){
IPluginModelBase modelExternal = fExternalManager.getAllModels()[i];
if (modelExternal.getPluginBase().getId().equals(newID)) {
addToTable(fEntries, new IPluginModelBase[] { modelExternal });
}
}
IPluginModelBase[] models = fWorkspaceManager.getPluginModels();
for (int i = 0; i < models.length; i++) {
IPluginModelBase modelWorkspace = models[i];
if (modelWorkspace.getPluginBase().getId().equals(newID)) {
addToTable(fEntries, new IPluginModelBase[] { modelWorkspace });
addWorkspaceBundleToState(fEntries, modelWorkspace);
}
}
}
Created attachment 261567 [details]
Cumulative patch
Attached cumulative fix which solves all the scenarios mentioned on this bug. ( this patch has contents from 54491) +1 for RC1 I am not doing any development in Eclipse currently so I will not be able to watch for any regressions this might cause. The version selection policy and comparison with qualifier logic looks good. Please check that if you have a feature in your workspace that requires the plugin you are changing that the editor and feature based launches work correctly. There is another issue within here that I can't tell if it is being considered. If you have a plug-in with the same version in your workspace as your target platform then the resolver for the classpaths will use the target version. However, doing a launch with "all workspace and enabled target plug-ins" will use the workspace version of the plug-in. An easy way to reproduce this case in Neon M7: 1) Create plug-in project, use Editor template in the wizard 2) Import org.eclipse.ui.editors as Source (e.g. from Plug-ins view) 3) Restart Eclipse -- this is critical, no restart here means that the workspace version of TextEditor.java will open below 4) Open XMLEditor.java 5) Ctrl-Click on TextEditor 6) Observe that TextEditor.class is opened 7) Open TextEditor.java and do something that will be visible in the launched Eclipse (e.g. add a throw new RuntimeException() in the constructor!) 8) Launch Eclipse Application 9) Create a file with .xml extension (so it opens in XMLEditor) 10) Open file and observe the exception added in Step 7. So now we have a situation that the browsing the Java code leads to target version of TextEditor and running the code uses the workspace version. (In reply to Jonah Graham from comment #16) > So now we have a situation that the browsing the Java code leads to target > version of TextEditor and running the code uses the workspace version. The launcher logic must be changed to match the dev behaviour. I was thinking this might be a problem with feature based launches as the feature models might not be updated. all of the different launcher modes will have to be checked. The scenario in comment#16 is not fixed by "Cumulative patch". For scenario of comment#16, control click should take us to the workspace version of TextEditor ( correct behaviour). Although all the tests ( junits and otherwise ) I have run have passed, I will lean towards moving this to 4.6.1. There are few issues in and around situations with same bundles in workspace and target ( and also multiple version of a bundle). The fix looks pretty good and would solve some of those issues. In some of the scenarios, a relaunch does solve the problem ( workaround). So in that sense this defect is not a blocker. There are some scenarios that may still be not working and since the changes are at the core of things, it "may" be slightly risky at this point of time as I cannot be totally sure if I covered all the possible scenarios of testing. Another option is to go forward with the changes(early rc2) and rollback in case we find any issues in next 2-3 weeks. Fixed this in 4.7 (master) using http://git.eclipse.org/c/pde/eclipse.pde.ui.git/commit/?id=91057909622705d35b95a262fb8492c54c8554a6 http://git.eclipse.org/c/pde/eclipse.pde.ui.git/commit/?id=5256f11f769c968aec010731c541430bc9493557 I will wait for sometime in 4.7 builds before cherrypicking to 4.6.1 Comment#16 should be a new defect. reverted commits of comment#21 as there were some test case failures in ep47N-unit-cen64linux.gtk.x86_648.0. I don't think the fix there is difficult but getting the debug setup there is time taking. Will take this up in early M2 time-frame. This looks similar to https://bugs.eclipse.org/bugs/show_bug.cgi?id=499486 Committed 1st part of solution ( author : Markus) in master via http://git.eclipse.org/c/pde/eclipse.pde.ui.git/commit/?id=468e1db1f482ebf1034aa8ab84b65de8432ff44b With the fix of comment#24, the test results are clean. See http://download.eclipse.org/eclipse/downloads/drops4/I20160920-0800/testResults.php Fixed issue of comment#9 via http://git.eclipse.org/c/pde/eclipse.pde.ui.git/commit/?id=8089e3b76a4ecf8b44a43d346f460eb95a40be46 reverted comment#26 as it was giving few errors on centOS platform. Looking at few other issues. Will commit this again ( and debug the issues) when I have sometime. New Gerrit change created: https://git.eclipse.org/r/84311 Put the change that fixes issue in comment#9 in masters http://git.eclipse.org/c/pde/eclipse.pde.ui.git/commit/?id=0ab09598b514065ee4883963a58599849766bcee There are few failures on centOS platform. Reopening bug 497638 to track failures. This is a very long thread. I feel compelled to argue that in all cases if I have a singleton bundle in my workspace, I always want it to be used. Really always. I have it in the workspace because I want to modify it and I want to use it. When I launch, I want to be able to select it and I want it to be used, even if it doesn't satisfy all the p2 constraints (which will often/generally be the case) and even if it fails at runtime because of API breakage. Along those same lines, I want it to be used to compile other things in my workspace, even if that causes compile errors, because those will be the runtime errors I'd get when I launched this thing. I propose putting "Always prefer workspace bundle" as a preference option as many of the users seems to prefer workspace bundles at all times. (In reply to Vikas Chandra from comment #31) > I propose putting "Always prefer workspace bundle" as a preference option as > many of the users seems to prefer workspace bundles at all times. +1 and this should be the default as that matches prior behaviour. Having two modes sounds good. We have to be careful not to break setups where multiple versions of a bundle are required in parallel (e.g. org.eclipse.jdt.annotation). New modes: (1) Workspace bundles override target platform (default): - If one or more versions of a bundle are open in the workspace, then all bundles with the same name get removed from the target platform. - If no version is available in the workspace, then all bundles from the target platform are active. (2) Merge workspace bundles with target platform, prefer .qualifier - Keep all available bundles from workspace and target platform, with one exception: If the workspace contains a bundle with version <x>.<y>.<z>.qualifier, then all bundles with the same <x>.<y>.<z> major/minor/micro versions get removed from the target platform. Mode switching as well as adding/removing bundles or changing bundle versions should work without restart (in the classpath container as well as during launch). I have made the changes and now we have 2 modes as per comment#33. All the scenarios seem to work OK. I have committed that in early 4.7M4 to find any issues early. For 4.6.2, I suggest 1) Either rollback couple of changes and make workspace the preferred bundle at all times without any option OR 2) Port the option as per Bug 506850 and all relevant fixes that went in that area. As of now for 4.6.2, I am leaning more towards option 1). For 4.6.2, I am recommending rollback of the 2 commits. I've reverted the 2 commits , quashed them and made a single commit for the rollback. https://git.eclipse.org/r/84546 Doing so will ensure that workspace bundle is always preferred and corresponding target bundle is lost ( as per the behavior for last many releases). For oxygen, we do have an option to toggle with the 2 types of option. ( see bug 506850). requesting +1 from Markus for the patch in previous comment for 4.6.2 (In reply to Vikas Chandra from comment #35) > https://git.eclipse.org/r/84546 I've fixed the commit message so that it starts with this bug, and then I pushed to R4_6_maintenance via Gerrit: http://git.eclipse.org/c/pde/eclipse.pde.ui.git/commit/?id=a67e668fa367982ef6272c38fa44d81488e412e7 Unfortunately, I missed that the original commit in Gerrit already lacked a Change-Id, so the Gerrit change didn't get closed automatically. I'll abandon it. verified with https://bugs.eclipse.org/bugs/attachment.cgi?id=265170 The compilation fails as is expected since workspace bundle always gets preference. ( in comparison to target bundle). Version: Neon.2 (4.6.2) Build id: M20161116-1100 |