Bad value of osgi.install.area in config.ini causes issues in latest JDK11

[habbit-baggins]

I have a run-of-the-mill RCP application with a plugin and a product. When launching it from inside Eclipse, the launch configuration created automatically by PDE from the product Overview → Testing view fails on JDK 11.0.15 (the latest version now on Adoptium) with the message:

An error has occurred. See the log file C:\workspace\.metadata\.plugins\org.eclipse.pde.core\platform.product\1655828949375.log

Where the log file contained a stacktrace:

java.lang.IllegalStateException: Install location is invalid: file:file:C:\project\target-platform/target-platform.target
    at org.eclipse.equinox.launcher.Main.getInstallLocation(Main.java:1977)
    at org.eclipse.equinox.launcher.Main.processConfiguration(Main.java:1886)
    at org.eclipse.equinox.launcher.Main.basicRun(Main.java:563)
    at org.eclipse.equinox.launcher.Main.run(Main.java:1467)
    at org.eclipse.equinox.launcher.Main.main(Main.java:1440)

The reason seems to be that PDE auto-generates a config.ini file like the following:

#Configuration File
#Wed Jun 22 11:52:11 CEST 2022
(...)
osgi.install.area=file\:file\:C\:\\project\\target-platform/target-platform.target
osgi.framework=file\:C\:/workspace/.metadata/.plugins/org.eclipse.pde.core/.bundle_pool/plugins/org.eclipse.osgi_3.17.0.v20210823-1805.jar
osgi.configuration.cascaded=false
org.eclipse.update.reconcile=false

Note that the value for osgi.install.area written by PDE is invalid if it is supposed to be a URL, for two separate reasons: it has backslashes (even after escaping in the properties file) and the file: prefix is repeated.

When trying to run anything, this causes a failure inside of Equinox launcher as follows:

1. While processing the configuration, the launcher tries to ensure that the install location is set (getInstallLocation)
2. The logic called by it (org.eclipse.equinox.launcher.Main.buildURL) gets really confused with this value, creating a monstrosity of a java.io.File instance with duplicated values, since it looks at osgi.install.area itself: file:C:\project\target-platform\target-platform.target\file:C:\project\target-platform\target-platform.target.
3. The code tries to convert it using the deprecated [File.toURL](https://docs.oracle.com/en/java/javase/11/docs/api/java.base/java/io/File.html#toURL()) method. It is that conversion that passes (again, producing a broken, unusable result) in JDK 11.0.14 and less, but throws in JDK 11.0.15.
4. Unable to build a URL, Equinox launcher bails out with the exception shown above.

I don't know if Equinox launcher could be made more resilient, but on the PDE side, we should not be writing broken URLs into its configuration. I do not know where config.ini gets generated inside PDE, though, so I would request help from wiser minds.

[habbit-baggins]

Other notes:

• I have not tested either on JDK17 or on any non-LTS version, as my product is Java 11-based
• I discovered this by chance, as I had my machine reimaged and after reinstalling my dev environment, things would simply not work, while a mate's nearly identical configuration did. The only difference was the JDK.

[mickaelistria]

Do you think you could provide a patch to fix this issue?

[habbit-baggins]

I could try, sure, but the problem is that the code generating this value was probably bad for a long time and it is only the "bugfix" in the JDK that now causes the Equinox launcher to choke on it. A proper solution might even require a design change (see below)

My limited (and possibly wrong) analysis goes as follows: first, the contents of config.ini seem to be generated in either LaunchConfigurationHelper.addRequiredProperties() or EquinoxLaunchConfiguration.saveConfigurationFile(). For "osgi.install.area", both versions take the output of TargetPlatform.getLocation() and prepend file: to it. The source of that data is ITargetLocation.getLocation() which is supposed to return a String with the path inside the local filesystem.

Thus, it appears that there are two problems:

• Some implementation of ITargetLocation (not sure which one, I have yet to debug PDE itself) is actually returning a URL-like string with the "file:" part already prepended, resulting in a bad URL.
• Also, it is not always possible to take a local path and make it a URL just by prepending "file:", it might need encoding (think special characters or Windows paths). It seems like ITargetLocation.getLocation should actually be modified to return an Eclipse IPath or a JavaSE File/Path instead of a String which can then be properly encoded.
  • Alternatively, considering the comment in the interface method (The current target platform framework requires a local file location but this requirement may be removed in the future), ITargetLocation could just return a full-blown URL, but that might affect too many things in the wider PDE.

That's the extent of my wits in this case... if the majority of this sounds reasonable enough, I am 100% willing to keep digging and contribute a patch. Otherwise, I would rather ask the elders and leave this open for somebody else to tackle at a future date.

[merks]

Simply prefixing with "file:" is just plan bad given this is trying to create a URI and given that the location on Windows will contain \ and not / and will often be of the form c:... where file:c:... is not not a good file: URI, though perhaps the platform tolerates it anyway. But the question is, where does a URI come from? All the implementations except this one return file system paths:

https://github.com/eclipse-pde/eclipse.pde/blob/b0b990ac3c3b46bf3c44b739b0c329beb55d38d9/ui/org.eclipse.pde.core/src/org/eclipse/pde/internal/core/target/TargetReferenceBundleContainer.java#L71-L77

@habbit-baggins Are you using a referenced target location in your target definition?

[habbit-baggins]

Are you using a referenced target location in your target definition?

I am indeed, which itself was the cause of some grief in both Tycho and PDE in the recent past.

where (...) is not not a good file: URI, though perhaps the platform tolerates it anyway

The issue I am seeing with Adoptium JDK 11.0.15 is precisely that the runtime no longer tolerates it, thus causing a crash inside the Equinox launcher. By contrast, JDK 11.0.14 and earlier do parse it - into a useless monstrosity, yes, but without throwing.

[merks]

I don't think anything ever tolerated file:file:... but the references target file is relatively new and I think the fact it returns a URI is fully the problem here.

@laeubi @vik-chand

My sense is that this is purely a new problem introduced by the target reference bundle container returning a URI for the location. The 'blind" prefixing with file: seems bad already but prefixing what's already a URI that way really does not work. I'm not sure how best to address this...

[laeubi]

• It seems like ITargetLocation.getLocation should actually be modified to return an Eclipse IPath or a JavaSE File/Path instead of a String which can then be properly encoded.

Using ITargetLocation.getLocation is bad and schould be avoided, I can't even tell what this has to do here for a launch?

The Javadoc says:

The current target platform framework requires a local file location but this requirement may be removed in the future. This method should not be referenced.

Actually this requirement is already not holding true anymore, especially for referenced target locations that could even reference http or maven URLs...

[habbit-baggins]

Using ITargetLocation.getLocation is bad and schould be avoided, I can't even tell what this has to do here for a launch?

Per the blame, that was changed in bug 283731. Previously it returned a location stored in the Eclipse preferences.

Regarding the Javadoc mention, both locations I found that use the return value of that (LaunchConfigurationHelper and EquinoxLaunchConfiguration) are older than that interface and comment. Thus, it seems that it is time to change them.

I will try to prepare a patch with the following changes:

• Change ITargetLocation.getLocation and all implementations to return a URI (or URL?), and see if that causes any breakage in the rest of PDE.
• Change the two mentioned locations to just use the returned URI/URL to initialize the osgi.install.area property with a properly-encoded form, without trying to prepend "file:", and see if the Equinox launcher copes with that under JDK 11.0.15.

[merks]

@vik-chand Please given Vikas time to respond because what's being proposed in this last comment sound scary and very incompatible for anyone else implement this extensible API. It's the more recently-introduced target location implementation that is inconsistent with the rest and is the one causing the problem. Oomph also has target location implementation and it also returns a file system location; if I make it return something different it won't work with anything older anymore. So given it's all kind of hacky in the first place, better in my opinion would be to check if the value already starts with file: and just use it otherwise do what it's been doing for the past 20 years and hasn't cause any problems during that time...

[vik-chand]

I am not sure when osgi.install.area is required. But if we have the 1st container of target as TargetReferenceBundleContainer, can we not look at the 1st container of the target definition of the TargetReferenceBundleContainer ( keep going on till we get a non-TargetReferenceBundleContainer and use that). Will that work? something like LaunchConfigHelper::getLocationForReferencedTarget

Is it possible to attach a simple project with steps to recreate this scenario so that I could look further.

[habbit-baggins]

what's being proposed in this last comment sound scary and very incompatible for anyone else implement this extensible API

Sorry, I didn't realize that this was something to be implemented outside PDE itself. Instead, we could keep the old method (possibly deprecated) and add a new one with a default implementation instead:

public interface ITargetLocation {
    // ...
    // Existing method, supposed to return a path _in the local FS_. Maybe deprecate it?
    String getLocation(boolean resolve);

    // New method
    default URI getLocationUri(boolean resolve) {
        return new File(getLocation(resolve)).toURI();
    }
}

For implementations whose getLocation already returned normal local FS paths, the default implementation is good. The "nested targets" impl may want to override both:

• the new getLocationUri to return the same URI it already computes
• the old getLocation to call the former. If it returns a file: URI, convert it to an actual filesystem path, otherwise throw UOpEx (e.g. for http/maven locations).

[merks]

I am not sure when osgi.install.area is required. But if we have the 1st container of target as TargetReferenceBundleContainer, can we not look at the 1st container of the target definition of the TargetReferenceBundleContainer ( keep going on till we get a non-TargetReferenceBundleContainer and use that). Will that work? something like LaunchConfigHelper::getLocationForReferencedTarget

No because a target definition might consist only of reference locations.

Is it possible to attach a simple project with steps to recreate this scenario so that I could look further.

This one has a simple example:

https://github.com/eclipse-pde/eclipse.pde/issues/230

[merks]

@habbit-baggins This latest suggestion sounds less scary, but note I believe the install location really must be a file: location in order to launch, so in the end the new getLocationUri could in principle return any old URI. If it's specified it must return a file URI then it's kind of pointless to introduce a new method (or such an overly general one) rather than just make sure that the one new target location implementation returns a file location too (or handle the possibility of either a file location or something that's already a file: URI.

[laeubi]

• Change ITargetLocation.getLocation and all implementations to return a URI (or URL?), and see if that causes any breakage in the rest of PDE.

This method should simply be removed, as it is completely nonsense, a target must not exits on a physical disk location at all, using the target "location" for any data-location in a launch or product makes also absolutely no sense, what do it has to do with osgi.install.area?

An install location is where Eclipse itself is installed. In practice this location is the directory (typically "eclipse") which is the parent of the eclipse.exe being run or the plugins directory containing the org.eclipse.equinox.launcher bundle. This location should be considered read-only to normal users as an install may be shared by many users. It is possible to set the install location and decouple eclipse.exe from the rest of Eclipse.

Why ever should eclipse be installed at the location of the targetfile?

[merks]

@laeubi You do realize that org.eclipse.pde.core.target.ITargetLocation.getLocation(boolean) is API, right? Perhaps it's being misused and there is a better way, but perhaps we can just be pragmatic and try avoid the need to cool and warn the ocean...

[laeubi]

@merks yes and you maybe realized that this is marked as @noreference This method is not intended to be referenced by clients. and its main purpose is to show a nice label to the user?

The only valid choice i could think of would be if there is exactly one location of type Profile and contains an eclipse.exe(cutable), so please don't blame implementations of that API for PDE doing obscure things just because it feels it might be some kind of local file what is returned there and any file must be a directory where an eclipse install is located ...

[laeubi]

By the way I not really propose to remove it so no worries... But trying to"fix" the method is clearly the wrong coice and one should handle this as if it do not exits (as the @noreference indicates), so the location where bad things happen should be fixed instead. Looking at the code it is clear the the Profile location computes a real osgi.install.area (aka eclipse home) and the rest of the implementations might more because of historic reasons than for any real good.

[merks]

Yes, I agree it's all kind of a hack.

Note that @noreference doesn't mean that PDE itself can't/shouldn't reference the method; just the clients. I think the simplest fix is just to check if the string already starts with file: and just use it if that's the case. That seems easiest and least disruptive, but that's just my opinion...

[laeubi]

I think the simplest fix is just to check if the string already starts with file: and just use it if that's the case. That seems easiest and least disruptive, but that's just my opinion...

Simple is not always better, eg:

• IU Location returns the bundle-pool directory
• Directory location return the user specified directory
• Profile returns the path to an Eclipse home
• What should m2e return, maybe the maven local repository?
• Target refrences return the URI specified by the user
• No idea what targlet should/would return ...

None of them (except Profile) seem suitable for an osgi.install.area and just mean nonsense to the Product launched (hopefully just ignored)...

So from the referenced issue, it seems that once there was maybe just "Profile" aka "Target" that has to point e.g. to an SDK directory, but today it rather vague and mostly useless to use by PDE itself (is anyone really using Profile location over simply reference a (local) update-site?)

[merks]

Indeed, simple is not always better. Though a simple fix is better than no fix. The perfect fix can always come later when someone has the time and the desire...

Anyway, there is quite a bit of theorizing here and that won't fix the problem either. I can say though that the install location passed to the runtime is most certainly not nonsense to the target eclipse application and it will most certainly not be ignored . It will definitely be used and there definitely must exist such a location.

[merks]

@laeubi As I think more, I think you're right. This is just broken. A folder location is needed so using the URI return for a reference target (the location of the *.target file) is just not going to work either...

[habbit-baggins]

None of them (except Profile) seem suitable for an osgi.install.area and just mean nonsense to the Product launched (hopefully just ignored)...

I can say though that the install location passed to the runtime is most certainly not nonsense to the target eclipse application and it will most certainly not be ignored

I don't know enough about the inner workings of Equinox and the platform, to be fair. In my setup (vanilla Eclipse PDE launching an RCP product), the auto-generated config.ini that is used while launching in dev mode is indeed set to this nonsense "file:file:C:\xxx" value, and the RCP product does launch and seems to work - unless using JDK 11.0.15.

Thus, I don't think that the launcher cares a lot about the value of that property when launched in dev mode, and with other values such as osgi.framework being provided. Perhaps something in the platform does later; my standalone RCP product does launch, but I have not tried launching the Eclipse IDE product.

A folder location is needed so using the URI return for a reference target (the location of the *.target file) is just not going to work either

The thing is, I think that the Equinox launcher tries to use the current working directory if o.i.a is not specified (see here). Would it be acceptable to simply not provide o.i.a in case the URI from the target is not in a "file:" form? If I understand @laeubi correctly, the folders that are used right now don't make much sense (bundle-pool, maven repo, etc.), so the CWD would be as good or bad a guess as anything, right?

[laeubi]

osgi.install.area should point to "the installation" that is the usual stuff you see when you extract an eclipse product (eclipse.exe, plugins/, features/, ...) and this is what a Profile location suppose to provide. So if such a location is found, it seems valid to use that ProfileBundleContainer.getLocation(true) if there is exactly one! For all other location types is questionable that they return any useful.

So if nothing that found, specify some empty folder or even do not specify it at all seems suitable, e.g. launch configs of products use already similar to

Config Area: ${workspace_loc}/.metadata/.plugins/org.eclipse.pde.core/<name of runconfig> User Area (aka workspace): ${workspace_loc}/../<lowercase of name of run config replacing any space by hyphen>