Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.

Bug 566642

Summary: Target platform for "Running Platform" assumes a collocated bundle pool
Product: [Eclipse Project] PDE Reporter: Ed Merks <Ed.Merks>
Component: UIAssignee: Julian Honnen <julian.honnen>
Status: VERIFIED FIXED QA Contact:
Severity: normal    
Priority: P3 CC: alex.blewitt, jhelming, julian.honnen, mn, stepper, Vikas.Chandra
Version: 4.17   
Target Milestone: 4.18 M1   
Hardware: PC   
OS: Windows 7   
See Also: https://bugs.eclipse.org/bugs/show_bug.cgi?id=527378
https://git.eclipse.org/r/c/pde/eclipse.pde.ui/+/170146
https://git.eclipse.org/r/c/pde/eclipse.pde.ui/+/170148
https://git.eclipse.org/c/pde/eclipse.pde.ui.git/commit/?id=d4a49972c4610da37f102c8755a54e88d73f86b5
https://git.eclipse.org/c/pde/eclipse.pde.ui.git/commit/?id=87c008e3edd6715b9d4a4320417875042a917446
https://git.eclipse.org/r/c/pde/eclipse.pde.ui/+/170232
https://bugs.eclipse.org/bugs/show_bug.cgi?id=567318
https://git.eclipse.org/c/pde/eclipse.pde.ui.git/commit/?id=f03516f8e776c403293a9c6f1a923d2deaa60c14
https://git.eclipse.org/r/c/pde/eclipse.pde.ui/+/170238
https://git.eclipse.org/c/pde/eclipse.pde.ui.git/commit/?id=23239f8df4dc8f6081f8abb3d0ca403999724bd8
Whiteboard:

Description Ed Merks CLA 2020-09-03 07:50:17 EDT
The changes for https://bugs.eclipse.org/bugs/show_bug.cgi?id=527378 have caused a major regression.

Details of the investigation are described here:

https://www.eclipse.org/forums/index.php/mv/msg/1105083/1831927/#msg_1831927

The fundamental problem is that the org.eclipse.pde.internal.core.PluginPathFinder.getFeaturePaths(String) method's implementation only works for installations with a collocated bundle pool, i.e., it only finds features in the features folder of the installation.  An installation that uses a shared bundle pool does not have such a folder.

I believe the implementation should be reading this file

configuration/org.eclipse.update/platform.xml

But with all the changes for 527378 , it's not clear if the intent is for this file to continue to exist and if not, where the information about the installation's features should properly come from.

This file is still present in the EPP package distros...

Furthermore, there are more complex scenarios such as shared read-only installations with cascaded configurations where the user could install features in their surrogate and I expect the current implementation does not work properly for these either.
Comment 1 Alex Blewitt CLA 2020-09-04 13:47:42 EDT
The getFeaturePaths was re-implemented so that we could break a dependency on update.configurator for PDE's use. However, there is an implementation of P2 that provides the IBundleGroupProvider in the update.configurator bundle which hasn't been back-ported yet, so provided that the installation still has that bundle in it then services that depend on it should still work as expected.

This is probably the specific change that introduced the delta in behavour:

https://git.eclipse.org/r/c/pde/eclipse.pde.ui/+/164035/12/ui/org.eclipse.pde.core/src/org/eclipse/pde/internal/core/target/ProfileBundleContainer.java

The comment in the deleted code suggests the plan was to drop support for platform.xml since that was only used by the old Eclipse update manager, rather than P2.
Comment 2 Alex Blewitt CLA 2020-09-04 14:00:30 EDT
Regarding the package installed; do the features show up in the list of the 'about' section of the running Eclipse (i.e. outside of PDE)? If they're not showing there, does the update.configurator bundle exist in the runtime and started? There's a small possibility that if the list of features in the parent Eclipse is empty then they won't be propagated to the running instance, in which case the fix might be to ensure that the parent instance has that bundle installed and running. (PDE brought it in as a dependency before; it may be that having removed that dependency, it's no longer included and in which case the list of features might not be showing elsewhere.)

If the update.configurator bundle is present, and the list of features aren't shown, then it may be due to the change above which stopped scanning the platform.xml file for a list of features but only looked in the 'features/' directory instead.

Unfortunately if that doesn't work then we might need to revert a subset of the changes for bug 527378:

386e461afa5eefb733cb0a2ac864dc230b0fe05b
11a09692b07fd57ba1e9e85f225edff215eb5a0f
a113ae7bf4dc063e902ba7a1e700e80d8f40570b

Unfortunately these steps will revert the performance benefits as they'll pull back in the dependencies to update.configurator but at this point in the release lifecycle I'm not sure if there is a more appropriate way of resolving the issues.
Comment 3 Ed Merks CLA 2020-09-04 14:14:56 EDT
(In reply to Alex Blewitt from comment #2)
> Regarding the package installed; do the features show up in the list of the
> 'about' section of the running Eclipse (i.e. outside of PDE)? 

Yes. And that was answered in the forum thread.  The profile definitely knows about the features.

> If they're not
> showing there, does the update.configurator bundle exist in the runtime and
> started? 

I'm not sure this is a relevant question.  But yes, it's present.

> There's a small possibility that if the list of features in the
> parent Eclipse is empty then they won't be propagated to the running
> instance, in which case the fix might be to ensure that the parent instance
> has that bundle installed and running. 

It's present in the profile.

> (PDE brought it in as a dependency
> before; it may be that having removed that dependency, it's no longer
> included and in which case the list of features might not be showing
> elsewhere.)
> 
> If the update.configurator bundle is present, and the list of features
> aren't shown, then it may be due to the change above which stopped scanning
> the platform.xml file for a list of features but only looked in the
> 'features/' directory instead.
> 

Yes, that's exactly which the debugger tells me.

> Unfortunately if that doesn't work then we might need to revert a subset of
> the changes for bug 527378:
> 

It doesn't work in general because the bundle pool can be shared instead of collocated.

> 386e461afa5eefb733cb0a2ac864dc230b0fe05b
> 11a09692b07fd57ba1e9e85f225edff215eb5a0f
> a113ae7bf4dc063e902ba7a1e700e80d8f40570b
> 
> Unfortunately these steps will revert the performance benefits as they'll
> pull back in the dependencies to update.configurator but at this point in
> the release lifecycle I'm not sure if there is a more appropriate way of
> resolving the issues.

It seems risky to try to change it back now.  I'm not sure the best alternative going forward.  Certainly the p2 profile knows exactly what's installed and I think it knows where those bundles and features are physically located...
Comment 4 Alex Blewitt CLA 2020-09-04 14:25:07 EDT
This can only looks for features, not bundles, so should be capable of resolving bundles in a shared bundle pool. It’s just that it won’t find expanded features this way, which in turn may not include those bundles from the pool. 

What kind of platform.xml does Oomph write out? Is it just a list of features that don’t happen to live in the platform itself? At the end of the day the method listed in my first comment returns a list of features which could be re-implemented in an alternate way to restore the behaviour before.
Comment 5 Ed Merks CLA 2020-09-04 15:04:50 EDT
(In reply to Alex Blewitt from comment #4)
> This can only looks for features, not bundles, so should be capable of
> resolving bundles in a shared bundle pool. It’s just that it won’t find
> expanded features this way, which in turn may not include those bundles from
> the pool. 
> 

Yes, the bundles are in the TP, but the features are not.  I'm not sure if any bundles are missing...

> What kind of platform.xml does Oomph write out? 

In both cases, the features are present in the platform.xml.  But with the new logic, nothing looks in that file.

> Is it just a list of
> features that don’t happen to live in the platform itself? 

No, generally the platform.xml should be all features, but with a shared bundle pools, all features in in the shared pool.  E.g., it looks like this:

<?xml version="1.0" encoding="UTF-8"?>
<config date="1599028765140" transient="false">
	<site updateable="true" url="file:/D:/Users/merks/.p2/pool/" enabled="true" policy="MANAGED-ONLY">
		<feature id="org.eclipse.emf.codegen.ecore.source" version="2.23.0.v20200701-0840" url="features/org.eclipse.emf.codegen.ecore.source_2.23.0.v20200701-0840/">
		</feature>
		<feature id="org.eclipse.emf.codegen.ecore.ui.source" version="2.23.0.v20200703-0737" url="features/org.eclipse.emf.codegen.ecore.ui.source_2.23.0.v20200703-0737/">
		</feature>

> At the end of the
> day the method listed in my first comment returns a list of features which
> could be re-implemented in an alternate way to restore the behaviour before.

Yes, somehow the features need to be computed without assuming they are located in the feature folder of the installation (collocated).  I expect/suspect that the p2 profile could be used more directly, but I'd need to investigate if the artifact locations are known in that manner.  I expect that is the case because Oomph's bundle pool analyzer knows how to find all the artifact from the profiles...
Comment 6 Alex Blewitt CLA 2020-09-04 16:55:26 EDT
Is the platform.xml file always at configuration/org.eclipse.update/platform.xml?

I could have a go at using that is present in that location to parse the file to compute the list of features.

What’s the best way of getting an environment installed to test this?
Comment 7 Ed Merks CLA 2020-09-05 03:05:30 EDT
(In reply to Alex Blewitt from comment #6)
> Is the platform.xml file always at
> configuration/org.eclipse.update/platform.xml?
> 
> I could have a go at using that is present in that location to parse the
> file to compute the list of features.
> 
> What’s the best way of getting an environment installed to test this?

Yes it's there in the packaged downloads and in the installations created by the installer.  Any environment created by the installer based on the 2020-09/4.17 version of Eclipse will illustrate the problem.

The installer ends up calling the following to fix the problems in that file in the case of a shared bundle pool:

https://git.eclipse.org/c/oomph/org.eclipse.oomph.git/tree/plugins/org.eclipse.oomph.p2.core/src/org/eclipse/oomph/p2/internal/core/AgentImpl.java#n1130

This code also illustrates how the platform.xml can be (and is) loaded via org.eclipse.equinox.internal.p2.update.Configuration.load(File, URL).

For a packaged download the platform.xml looks like this:

<?xml version="1.0" encoding="UTF-8"?>
<config date="1598532645972" transient="false">
	<site updateable="true" url="platform:/base/" enabled="true" policy="USER-EXCLUDE">
		<feature id="org.eclipse.equinox.p2.discovery.feature" version="1.2.700.v20200705-1016" url="features/org.eclipse.equinox.p2.discovery.feature_1.2.700.v20200705-1016/">
		</feature>


Note in particular the url="platform:/base/" which must be resolved to a file: URL to find the actual files referenced by the features' relative URLs, i.e., url="features/*"

For an installation produced by the installer with a shared bundle pool, the platform.xml looks like this:

<?xml version="1.0" encoding="UTF-8"?>
<config date="1599028765140" transient="false">
	<site updateable="true" url="file:/D:/Users/merks/.p2/pool/" enabled="true" policy="MANAGED-ONLY">
		<feature id="org.eclipse.emf.codegen.ecore.source" version="2.23.0.v20200701-0840" url="features/org.eclipse.emf.codegen.ecore.source_2.23.0.v20200701-0840/">
		</feature>

Note that fixing the site url to be a proper absolute file URI with an absolute path (rather than relative) is what org.eclipse.oomph.p2.internal.core.AgentImpl.adjustPlatformXML(String, File) does.  Also the policy is changed to MANAGED-ONLY so that only the explicitly listed features are included in the installation, not all features in the shared pool.
Comment 8 Ed Merks CLA 2020-09-05 03:11:17 EDT
I imagine this will be tricky to debug/test changes.

What I did for that purpose is add this to the eclipse.ini of the Oomph installed version of 2020-09:

-Xdebug
-Xrunjdwp:transport=dt_socket,server=y,suspend=y,address=8123

And the remote debugged it from my Eclipse SDK IDE.  That works well if the source is in sync with what version of PDE/p2 is actually installed in that installation...

That being said, if you ensure that the platform.xml is properly processed in terms of how the url of the site(s) and the features are resolved, the same logic should work well for collocated and shared bundle pools.
Comment 9 Julian Honnen CLA 2020-09-30 08:00:10 EDT
(In reply to Ed Merks from comment #7)
> Also the policy is changed to MANAGED-ONLY so that only the
> explicitly listed features are included in the installation, not all
> features in the shared pool.
I'm trying to figure out where that aspect was handled previously.

AFAICS PDE simply consumed the IPlatformConfiguration:
https://github.com/eclipse/eclipse.pde.ui/blob/38ca2f903f105ac23b0d576c3d8fb3749fbc4ea7/ui/org.eclipse.pde.core/src/org/eclipse/pde/internal/core/PluginPathFinder.java#L242

In update.configurator itself, I can't find anything that handles the site policy for features (only for plugins):
https://github.com/eclipse/eclipse.platform/blob/d447133afceb72ed54eac0a0cdb5dfd6aa420544/update/org.eclipse.update.configurator/src/org/eclipse/update/internal/configurator/SiteEntry.java#L126


I guess for PDE the only feasible solution is to copy the required code from update.configurator to parse that file.
Relying on IBundleGroupProvider to get the features is not an option because it
a) is just a masked dependency on update.configurator
b) only works for the ${eclipse_home} installation, not external ones
Comment 10 Eclipse Genie CLA 2020-10-01 04:31:30 EDT
New Gerrit change created: https://git.eclipse.org/r/c/pde/eclipse.pde.ui/+/170146
Comment 11 Eclipse Genie CLA 2020-10-01 04:31:34 EDT
New Gerrit change created: https://git.eclipse.org/r/c/pde/eclipse.pde.ui/+/170148
Comment 12 Eclipse Genie CLA 2020-10-01 04:31:42 EDT
New Gerrit change created: https://git.eclipse.org/r/c/pde/eclipse.pde.ui/+/170149
Comment 13 Eclipse Genie CLA 2020-10-01 04:31:44 EDT
New Gerrit change created: https://git.eclipse.org/r/c/pde/eclipse.pde.ui/+/170147
Comment 16 Ed Merks CLA 2020-10-02 04:09:11 EDT
I can try to test this when it's an an IBuild...
Comment 17 Eclipse Genie CLA 2020-10-02 06:37:47 EDT
New Gerrit change created: https://git.eclipse.org/r/c/pde/eclipse.pde.ui/+/170232
Comment 19 Eclipse Genie CLA 2020-10-02 08:39:16 EDT
New Gerrit change created: https://git.eclipse.org/r/c/pde/eclipse.pde.ui/+/170238
Comment 21 Vikas Chandra CLA 2020-10-06 07:51:20 EDT
Julian, can you please verify this fix.
Comment 22 Julian Honnen CLA 2020-10-07 07:06:23 EDT
A target based on an external oomph-based installation finds the correct features with I20201006-1800.