Move or reuse third-party classes out of org.eclipse.embedcdt.core

jonahgraham commented 3 years ago

org.eclipse.embedcdt.core currently exports some classes/packages for use within embedcdt bundles. This can cause weird and terrible wiring errors. For example, if another bundle does Import-Package: org.json.simple they can get the one provided in the embedcdt which may not be expected, especially if that pulls in other stuff like CDT. No one has complained about it in 2020-12 milestones, so we are OK for now, but it should be fixed ASAP.

As a temporary workaround the exports are being marked as non-API for the 6.0.0 release.

jonahgraham commented 3 years ago

There are places which the API takes or returns these non-API classes. For now I am marking these methods as non-API until we can resolve this issue fully.

ilg-ul commented 3 years ago

Thank you for detecting this.

What would be possible solutions?

jonahgraham commented 3 years ago

Reuse and/or contribute the shared libraries to Orbit. I can help with that.

However for semver it is probably is best to rename the packages because it is a fork.

ilg-ul commented 3 years ago

Reuse and/or contribute the shared libraries to Orbit. I can help with that.

Sure.

for semver it is probably is best to rename the packages because it is a fork.

I guess we can do this, but this means there will probably never be more updates, since further merges will be impractical, almost impossible.

If you can explain me exactly what the problem is, perhaps I can figure out a solution. If the actual semver classes are not public, is it still a problem?

jonahgraham commented 3 years ago

for semver it is probably is best to rename the packages because it is a fork.

I guess we can do this, but this means there will probably never be more updates, since further merges will be impractical, almost impossible.

It seems strange to worry about further merges on code that has not had a commit upstream in >6 years.

Is Semver modified, I thought it was, but I see that liqp is also modified. If Semver is not modified, then adding it to orbit is a good solution.

If you can explain me exactly what the problem is, perhaps I can figure out a solution.

If one bundle exports a package, any other bundle (embedcdt or not) can import it. So there are a few issues:

If the package you are exporting is not versioned and identical, then depending on how OSGi and p2 resolve stuff, simply having embedcdt installed can break other people.
If multiple bundles export the same package (e.g. org.json.simple) then p2 can decide to use either one to resolve it. This means that embedcdt's presence can cause unexpected results for other packages
for places where these types are used in API, it the consume of the API does not use the same loaded versions of these classes, then at runtime you can class exceptions that are very hard to track down

For all the above cases, the results are generally not deterministic, so depending on random other factors, such as version of Java, how many plug-ins are installed, etc you can run into a problem.

None of the above is urgent. However it is technical debt that can come back to bite us down the road. After having spent days resolving a similar issue in simrel I am trying to protect against that for the future.

If the actual semver classes are not public, is it still a problem?

If the packages are not exported it is not a problem. If they are exported, even if x-internal, then it is a problem. The modifier (public/private) on the class itself does not come into play.

ilg-ul commented 3 years ago

I understood only that we have a problem, but I don't get the details yet.

For semver, how about renaming all packages to org.eclipse.embedcdt.semver? I can do this relatively easily and have it fixed in 6.0.0.

As for Liqp, there were several small patches, we need to use the patched version. Any quick fix for it?

jonahgraham commented 3 years ago

Same quick fix for liqp, rename package to org.embedcdt namespace.

ilg-ul commented 3 years ago

Now semver and liqp are fixed, please check the separate commits.

Any other improvements?

ilg-ul commented 3 years ago

Especially please check the export definitions, I don't know how they should look like, I just fixed the details for the build to pass.

jonahgraham commented 3 years ago

aef3d92dba makes sure none of liqp or semver is exposed as API.

If someone needs this as API it is easier to expose as API than to hide it later.

jonahgraham commented 3 years ago

Now semver and liqp are fixed, please check the separate commits.

org.jsoup is already available in Orbit with vetted IP, so no CQ needed for the dependency. But since it is in orbit it would be better to pull in that copy. Add cross-ref to #453

Any other improvements?

Just at some point the stuff in #453. But I wouldn't do that now right before the release. The API in regards to #453 is clean so I think this issues is done.

ilg-ul commented 3 years ago

org.jsoup is already available in Orbit with vetted IP, so no CQ needed for the dependency. But since it is in orbit it would be better to pull in that copy.

I got the org.jsoup_1.7.2.v201411291515.jar file from Orbit R20201130205003.

What else needs to be done?

ilg-ul commented 3 years ago

Apart from the 'Activator' warning, I would say that we are ready with the develop-internal branch.

When you confirm this, I would merge it into develop.

jonahgraham commented 3 years ago

Ready. Merge away!

jonahgraham commented 3 years ago

org.jsoup is already available in Orbit with vetted IP, so no CQ needed for the dependency. But since it is in orbit it would be better to pull in that copy.

I got the org.jsoup_1.7.2.v201411291515.jar archive from Orbit R20201130205003.

What else needs to be done?

It shouldn't be a nested jar. But we can fix that later as it is quite minor.

ilg-ul commented 3 years ago

I'm not sure what merge away means, but I merged it aNYway ;-)

We're back on develop.

How can the org.jsoup jar be used without having to include it in the project? Just curious, not urgent, we can fix it at the same time when we move all jars to Orbit.

jonahgraham commented 3 years ago

How can the org.jsoup jar be used without having to include it in the project? Just curious, not urgent, we can fix it at the same time when we move all jars to Orbit.

You add the Orbit repo as another entry to https://github.com/eclipse-embed-cdt/eclipse-plugins/blob/2e12c1b97d96a588fac81ef443792b82b905c85b/parent%2Fpom.xml#L66-L75

and then you republish these extra dependencies to your p2 site in https://github.com/eclipse-embed-cdt/eclipse-plugins/blob/e89616ff5cbbf06ef977124e2e4ee5d63e081aff/repositories%2Forg.eclipse.embedcdt-repository%2Fcategory.xml#L110-L111

a diff like this would probably work, but I haven't had a chance to try it.

diff --git a/parent/pom.xml b/parent/pom.xml
index 5d58a229c3..946897de41 100644
--- a/parent/pom.xml
+++ b/parent/pom.xml
@@ -77,6 +77,11 @@
                        <url>http://download.eclipse.org/cbi/updates/license/</url>
                        <layout>p2</layout>
                </repository>
+               <repository>
+                       <id>orbit</id>
+                       <url>https://download.eclipse.org/tools/orbit/downloads/drops/R20201130205003/repository</url>
+                       <layout>p2</layout>
+               </repository>
        </repositories>
        <pluginRepositories>
                <pluginRepository>
diff --git a/plugins/org.eclipse.embedcdt.core/META-INF/MANIFEST.MF b/plugins/org.eclipse.embedcdt.core/META-INF/MANIFEST.MF
index 7508217d9c..2aa43d3f30 100644
--- a/plugins/org.eclipse.embedcdt.core/META-INF/MANIFEST.MF
+++ b/plugins/org.eclipse.embedcdt.core/META-INF/MANIFEST.MF
@@ -565,7 +565,8 @@ Require-Bundle: org.eclipse.cdt.core;bundle-version="7.0.0",
  org.eclipse.cdt.managedbuilder.core;bundle-version="9.0.0",
  org.eclipse.core.runtime;bundle-version="3.19.0",
  org.eclipse.core.variables;bundle-version="3.4.800",
- org.eclipse.debug.core;bundle-version="3.16.0"
+ org.eclipse.debug.core;bundle-version="3.16.0",
+ org.jsoup;bundle-version="[1.7.2,1.8.0)"
 Bundle-ClassPath: lib/antlr-runtime-3.5.2.jar,
  lib/jackson-annotations-2.9.9.jar,
  lib/jackson-core-2.9.9.jar,
@@ -573,6 +574,5 @@ Bundle-ClassPath: lib/antlr-runtime-3.5.2.jar,
  lib/liqp-0.6.8.jar,
  lib/ST4-4.0.8.jar,
  .,
- lib/json-simple-1.1.1.jar,
- lib/org.jsoup_1.7.2.v201411291515.jar
+ lib/json-simple-1.1.1.jar
 Automatic-Module-Name: org.eclipse.embedcdt.core
diff --git a/plugins/org.eclipse.embedcdt.core/build.properties b/plugins/org.eclipse.embedcdt.core/build.properties
index eba12f008e..40ffc24dc5 100644
--- a/plugins/org.eclipse.embedcdt.core/build.properties
+++ b/plugins/org.eclipse.embedcdt.core/build.properties
@@ -25,7 +25,6 @@ bin.includes = META-INF/,\
                lib/jackson-databind-2.9.9.3.jar,\
                lib/ST4-4.0.8.jar,\
                lib/json-simple-1.1.1.jar,\
-               lib/org.jsoup_1.7.2.v201411291515.jar,\
                about.html,\
                .options
 src.includes = about.html
diff --git a/repositories/org.eclipse.embedcdt-repository/category.xml b/repositories/org.eclipse.embedcdt-repository/category.xml
index 0f0a3200dd..9240b31ac5 100644
--- a/repositories/org.eclipse.embedcdt-repository/category.xml
+++ b/repositories/org.eclipse.embedcdt-repository/category.xml
@@ -109,4 +109,5 @@
    <feature id="org.eclipse.embedcdt.debug.gdbjtag.qemu.source">
       <category name="org.eclipse.embedcdt.category.source"/>
    </feature>
+   <bundle id="org.jsoup" version="1.7.2.v201411291515"/>
 </site>

I noticed that orbit has jsoup 1.7.2 and 1.8.3. Assuming there was a reason you went with the older version I added the extra version ranges above to make sure that the 1.7.2 version was chosen over the 1.8.3 version.

BTW - putting together the above diff reminded me of another problem with nested jars. It removes the compile time checks for dependencies. i.e. removing lib/org.jsoup_1.7.2.v201411291515.jar does not cause a compilation error, but presumably will cause a runtime error.

ilg-ul commented 3 years ago

presumably will cause a runtime error.

yes, actually that was my concern, that without the nested jar I cannot test anything locally, which makes things more complicated and time consuming.

jonahgraham commented 3 years ago

presumably will cause a runtime error.

yes, actually that was my concern, that without the nested jar I cannot test anything locally, which makes things more complicated and time consuming.

The current development flow for embedcdt (as documented on https://eclipse-embed-cdt.github.io/develop/build-prerequisites/#install-several-eclipse-plug-ins) is designed to be the only thing in the workspace. So in this development flow, developers would just have to in the "Install several Eclipse plug-ins" section also install org.jsoup and other dependent plug-ins.

However, moving to a target file means that you can detach the development version of Eclipse from the target version. That is what happens with CDT, developers use cdt.target to define the target platform instead of "Running Platform" in target platform preferences.

ilg-ul commented 3 years ago

Since we decided to move all jars to Orbit, can we fix the org.jsoup at the same time?

jonahgraham commented 3 years ago

Since we decided to move all jars to Orbit, can we fix the org.jsoup at the same time?

Yes.

eclipse-embed-cdt / eclipse-plugins

Move or reuse third-party classes out of org.eclipse.embedcdt.core #453