multiple versions of gson bundle in target platform causing issues #482
Comments
|
Your import forbids gson 2.10.1 (if we assume that bundleversion == package version what is not a requirement), so probably you have some other requirement (transitively?) that requires a >= 2.10.1 gson? So one would need a bit more information, at best:
|
|
There are definitely bundles in the target environment that require a gson >= I will try to create a small sample to reproduce this and update this issue here once I have that. |
|
Here are steps how to reproduce this issue:
The target environment will automatically be the running Eclipse instance, which contains both versions of the |
|
@laeubi Hope this helps you to reproduce the issue. Felt like the easiest way to reproduce this without anything specific from my side, like a specific target environment, a specific project, or anything in this area. If you need anything else, let me know. Happy to help as good as I can here. |
|
This reproduces the problem in an easy to debug way: You can import this in a debug-launched IDE (the Oomphed Platform SDK IDE), activate the gson.target as the active target platform and you'll see the problem. The underlying problem is that this helper method assumes that each package is exported once:
I added printing logic to show what's not being added to the map (or being replaced in the map) and the variables view shows the contents of the map that's used later to verify package imports... |
|
Looking more closely, I'm not sure this is the only problem or the problem. I think it's also a (more fundamental) problem that these gson bundles end up being marked as unresolved as a whole. I'm guessing that their package import |
|
@martinlippert Which exact error message do you see? |
|
|
|
That's very different from what I see. Are you seeing this is a recent 4.27 2023-03 build? Debugging deeper, it's this constraint that is not satisfied by the 2.10.1 version:
And this for the 2.9.1.x version
But that doesn't really make much sense... This is the error I see:
|
|
For what it's worth, here's the debug tracing from org.eclipse.osgi.internal.module.ResolverImpl
|
|
@martinlippert I see you did say a 2023-03 JEE package so it should be quite a recent build... |
|
I think the failures I saw were due to OSGi being missing from the target platform. This slightly larger target platform: reproduces the error bad message:
|
|
At this point, the resolver decides the import is not satisfied by the export because the substitution is not null, but the substitution also does not satisfy the import while the ignored export does:
I think this is because several of the packages from com.google.gson_2.9.1.v20220915-1632 are wired to packages exported by com.google.gson_2.10.1:
And that wiring happens because the Orbit versions imports its own exported packages but with a very wide version range.
I'm not sure there is bug in the resolver or if the wide version ranges on the bundle are sort of wrong. One question though is how does this work and runtime and shouldn't the resolver reflect that same behavior? |
|
I think this is a bug in the resolver logic, but I'd have no clue how to fix it. There are other possible alternative wirings where there would not be a problem... Also, given that the bundle exports package |
|
@merks PDE uses the old no longer supported resolver so more than possibly a "hack" to prevent this special case might not happen if even that. I wanted to migrate this to the new OSGi Resource Model (and Resolver) but that's something that will need a lot of effort. |
|
All the newer Orbit things like this use BND recipes, e.g., https://git.eclipse.org/c/orbit/orbit-recipes.git/tree/google/com.google.gson_2.10.1/osgi.bnd Indeed something that is equivalent to what really happens at runtime would be better. So far my attempts to hack something haven't worked well. One thing I haven't tried is to eliminate this hooking up of substitutions. I just seems question to substitute a versioned package with a package with a different version because clearly the substitute might fail to satisfy some import package constraints... Perhaps something very narrow like don't substitute a versioned package with a package with a different version... I'll play some more, time permitting... |
|
The official gson uses the following: and that's pretty much what one would expects, so why do orbit need to bundle this at all with different headers? Maybe you can just replace the orbit variant with the official maven artifact and see if it behaves different? by the way that's exactly the reason why I'm not really a fan of using substitution packages "just in case" if they are actually nothing one would ever like to substitute.... |
|
@jonahgraham FYI, I've found this "import your own exports" thing kind of strange too... Here's a hack that fixes the problem:
The candidates are normally listed with the highest version first. This special case is just to make the first candidate be your own exported package if such a one is in the list... I would appreciate @tjwatson expert opinion on such a hack. Perhaps guarded by dev mode? |
probably you can submit this as an PR so we see if maybe any test fails? |
|
I'm not sure about your fix. It reorders the candidates which seems problematic. What concerns me is that we have a substitute version |
Thanks for the FYI. Interesting to note @laeubi's #482 (comment) shows that even the official one imports its own exports (at least one of them). |
Package |
|
The self-import of
That's what the hack fixes so the wiring is like this:
That's what makes the 2.9.1.x package available to be imported by the example bundle. I know it's a hack and questionable at that. But the overall behavior of not finding a solution when one definitely exists and is also bug. I don't know how to solve that... |
|
Then I do not understand what you are showing in #482 (comment) Where it looks like the resolverImport and resolverExport are from the same bundle yet the export is substituted with what I thought was an incorrect version according to the range of the importer. |
|
In #482 (comment) it shows the import from the example:
being resolved/tested against the exported package from com.google.gson 2.9.1.v20220915-1632 (the only valid resolution of the import) but being ignored (treated as not satisfying the constraint) because it's substituted by the substitution package exported from com.google.gson 2.10.1 which fits correctly into the import range of So to summarize, the substitution being set is not incorrect, but that results in the overall resolution no longer being possible because the package with the necessary version has been substituted and hence excluded from the packages available for resolution elsewhere. |
|
To see a simple analog of this problem, paste the following into org.eclipse.osgi.tests.services.resolver.SubstitutableExportsTest: A simpler "hack" in org.eclipse.osgi.internal.module.ResolverImpl.resolveImport(ResolverImport, List) is to change the following:
I.e., substitute the export only if it's the same version. This will cause other tests to fail. I can't say whether the current behavior is correct or not. What is clear though is that this is not an issue that can be fixed by PDE. |
|
I'll try to take a look at this today:
I'm not too worried about making the changes here needed by PDE, although I would like to make sure it is a correct solution. PDE is one of the last (the last?) users of the old resolver. We could almost consider it to simply be part of PDE at this point. |
|
Note that on epp-dev, they (DSL package M3) are having runtime problems related to these plugins as well: |
|
I'm convinced we should do anything here. The fact is the bundle says its export can be substituted and they allow it to be substituted with another minor version increase of the package seems wrong. That makes contracts that use require-bundle against such a bundle very unpredictable because they can all of a sudden get access to a minor version increase without having changed their requirements at all. The suggested hack will result in almost nothing getting substituted unless it results in a uses constraint violation because most all bundles will import using a range that can be satisfied by their own export. |
|
Why does bnd generate these self imports then? Is that a bug in bnd? |
|
The meta-data is not incorrect, it might not what one desire though. Still PDE is handling this case wrong as shown by @merks example actually can be resolved by Tycho (and Tycho also uses OSGi resolver!) and by an OSGi framework, so the message that no one export that package is wrong and substitution (for the simple example) should not happen. |
|
I think the Orbit metadata is questionable at best, though not strictly incorrect. If you export a package x.y.z and then in the same bundle import that package with version range [x.y.z,x+1.0.0), no one can safely import your package with range [x.y.z, x.y.z+1) because you've allow that to be replaced/substituted. We see that sometimes it works with multiple versions, i.e., JEE product and sometimes it doesn't, i.e., DSL product. So such an import just seems like a bad idea to me, asking for problems to happen. The PDE error message is also unhelpful and misleading, though perhaps not strictly wrong because it's exactly the problem/error one sees in the DSL product. Anyway, I'm trying to make this problem go away by making the Orbit gson 2.9.1 go away... |
|
@merks I agree that the metadata is questionable, but your conclusion is wrong. The import can safely be done, see my example! It is just be problematic in the case here where old+new lsp4x and they do not declare requirements correctly (but that's not to blame on the gson completely) or we even have an "impossible" state where also the maven-gson will just show another error I fear, but we will see how it works out, maybe this change is the required difference to guide the resolver "on the right track" ... |
|
Yes the Maven gson 2.9.1 bundle shows a different error:
Looking at the bundle gson maven 2.9.1 bundle we see the exported com.google.gson.annotations package has been substituted by the imported com.google.gson.annotations package which has resolved to the 2.10.1 package from bundle 2.10.1.*
So again we have the case of a restricted package import, i.e., The import that allows this, and that declares it's appropriate, has in fact been applied, and now the package is invisible such that no narrow import can resolve to it. So I again, incorrectly it would seem, reach the conclusion that allowing a versioned package to be replaced by a different version causes problems because the replaced version is no longer available for resolution. The suggestion seems to be that something else is doing something wrong. It's just not clear to what that something wrong is. Is there something being done wrong somewhere else that somehow forces the export to be replaced when that's not necessary to replace it but we just can't see what that something is? An alternative conclusion here is that all the resolvers are doing something wrong at least sometimes and are not finding a correct solution that does in fact exist... |
As mentioned above |
|
I recall someone explaining that they used narrow version ranges for gson because gson tends to break APIs without doing major version increments, i.e., the suggestion was that gson is not doing proper semantic versioning. But I don't know if that's true. Given that @martinlippert is using such a narrow version range, which is likely also wrong, maybe he can explain why he is using that narrow range rather than |
Which comment do you say this is wrong? I can fix LSP4J. At some point in the past GSON did not do semantic versioning properly so the GSON version ranges were adjusted to make sure known compatible versions of GSON were consumed (IIRC the issue was adding new API in a maintenance version bump) |
|
GSON has violated semantic versioning in the past - once bitten twice shy as a result. LSP4J also does not do semantic versioning which is why there is tight requirements on LSP4J. If you want the historical details I can look them up, but please only ask if it is needed. FTR there are lots of problems with semantic versioning anyway, especially considering even the leading project (Eclipse Platform) has a policy to circumvent semantic versioning (with good cause!). |
It does not surprise me that the resolver used by the framework behaves differently with respect to substitutable packages. The resolver used by the framework adds the decision to substitute as another branch in the decision tree so that it can backout and use another permutation if the decision leads to a class space issue. But the new resolver and the old equinox resolver both do not backtrack the decision to substitute if that decision removes a capability that is the sole provider of another requirement that is being resolved. Both resolvers certainly could do this but it will result in expanding the decision tree leading to more possibility of "endless" resolves. One thing the to remember is the new resolver reverts to resolving bundles "individually" if the resolve process is taking too long (by default 2 minutes) in order to reduce the decision tree size. Resolving bundles individually causes different decisions to be made and could impact the decision to substitute a package. |
|
@jonahgraham I don't say any comment is wrong, I said the versionrnage is wrong (given the metadata we have, of course a project like gson might not "play with the rules") gson exports a package lsp4x now imports that package with this exact version (well it actually would allow micro and qualifier changes), what is usually only the case if lsp4x provides an implementation of that package. That means, if any newer API is available, lsp4x can not be used (as an implementer can't implement "future" versions... Now regarding the violations, if thats the case, it should be reported to gson and they should not import/export any package, still lsp4x should use a version range that is as open as possible, e.g. instead of (assume there is no break in API) of it previously has imported |
|
I don't think there is actually an API break between GSON 2.9 and 2.10 so we can have a workaround in LSP4J for this issue and defer the underlying problem to the future.
Sorry for being unclear, my comment was asking for which comment you had said this in previously. Thanks for reexplaining it in a standalone comment. |
@tjwatson I'm wondering if it would be possible to instead of using a new branch, just keeping each branch and just use the one that can resolve all items if the other branch did not succeed? |
I'm not sure I follow. Resolving all items on the branch involves checking class space consistency which may lead to a conflict that needs to be backed out to a previous decision higher up in the tree. Here I am talking in terms of the new resolver. The old resolver is implemented a bit different with respect to the branches when it makes a decision. I'm not sure how easy it would be to do something like that in the old resolver. |
That would probably help. And from the analysis you showed in M2E when we updated gson there I agree that it should be save. Should I revive eclipse-lsp4j/lsp4j#690 ? |
|
I just thought that the (new) resolver would do the following:
so if it yields that (1) results in 1 or more unresolved bundles, but (2) with all bundles resolved, I would have naively assumed that (2) is chosen? But probably its not that simple all together :-) |
|
IIRC, but I haven't had my head deep into this part of the code for a while ... The new resolver only backtracks on decisions based on if a decision causes a uses constraint conflict/violation. What you are suggesting is that the decision to substitute not only be backtracked for uses constraint conflict/violation, but also if the decision causes one or more bundles to not be resolved because of a missing provider for an imported package that got substituted. That may or may not be hard to implement. It is a little different so not sure how easily it will be to fit into the algorithm. The current algorithm is looking for a solution that causes the least number of conflicts. Here we would add a new dimension to the desired solution to include the least number of unsatisified imports to substituted packages. |
|
I have not taken a look at the code, so this is just a rough idea, but from a users POV the actual "optimization goal" is to resolve as much bundles (or maybe better Resources) as possible (at best: all of them), so a solution where one Resource can not be satisfied is better as one where two can not be satisfied. |
The idea of the algorithm is that less conflicts should result in more bundles being resolved. We just ignore changing the decision on substitutes based number of requirements it ends up leaving unsatisfied. That dimension is ignored. |
|
LSP4J contributed a new version to SimRel today that widens the range of gson which effectively works around the issue for now in the deployed products. I don't know if it also works around the PDE case @martinlippert originally described. |
I had a lot of compile-time and build-time related issues around those two versions of the gson bundle, partly because I had a bundle that had a That all finally worked reasonably well, but then PDE wasn't able to deal with this anymore and I filed this issue. So my initial observation was mostly about the inconsistency of PDE flagging this as an error while tycho builds and the runtime were able to resolve all this just fine. So it looks to me like this issue uncovered a number of issues:
Meanwhile, I removed the narrow version constraints from my own bundle, so I am not hitting the PDE issue originally described anymore, and all the other changes (less narrow constraints on lsp4j, more bundles updated to newer version of gson, etc.) make this less of an issue. But I still a bit worried of users running existing installations with gson 2.9.1 and lsp4j 0.19.0 installed - which still have the old and narrow version constraints - and what happens if there is some bundle not updated in those installs, so that those older versions stay around and causing issues... 😱 I am currently running tests of updating an existing installation which uses lsp4j 0.19.0 and somehow, after the update to some nightly builds, I end up having both versions in the install (lsp4j 0.19.0 + 0.20.1), which is causing new problems, but that is probably not related to the original topic of this issue... |
Disclaimer: the following is just an observation, and not meant as a complaintWell as you outlined the issue already started long time ago and we are now paying the dept...
So the only thing one can do is catch up, improve and looking forward.... |
|
The tests that I am running at the moment around updating existing installs also reveals that |
|
I filed eclipse-lsp4j/lsp4j#702 for the problem that I described above (which is most likely unrelated to this one, but similar) |
|
After the problematic imports have been fixed in the affected projects, do you think there is something that hast to be done in PDE or can we close this issue? |
With the LSP4J change the priority of this in PDE is certainly much lower and probably doesn't warrant much time with higher priority things to do. Fundamentally I think there is still a bug in PDE here as the resolution in PDE seems different than runtime. |
|
I agree with @jonahgraham here. A fix for this is not super important anymore (due to the changed import definitions of the mentioned bundles), but I also assume that it is just a matter of time until the difference between the runtime and the PDE resolution mechanisms will bite us again. |
eclipse-pde/eclipse.pde#482 Change-Id: I7380981beb12a742c02e28fb6ccb1c27cbb351f1
I have a target platform that has two versions of the gson bundle:
I have a plugin project in my workspace that has
PDE flags an error saying that it can't find a bundle for
com.google.gson. When I remove the version constraint from the dependency, the error goes away and the dependency is resolved against gson2.10.1.The text was updated successfully, but these errors were encountered: