Community
Participate
Working Groups
org.eclipse.equinox.p2.metadata.RequiredCapability is designed to work as both IRequiredCapability and IRequirement. IRequirement is the core p2 abstraction for a requirement and it is expressed in the most general terms as a match expression over the entire metadata of an IU. It therefore does not have nor namespace nor name nor version range. An IRequiredCapability is a specialization of IRequirement that simply builds a match expression over the provided capabilities: E.g. Import-Package: org.example; version="[2.0.0, 3)" is mapped to providedCapabilities.exists(pc | pc.namespace == 'java.package' && pc.name == 'org.example' && pc.version >= '2.0.0' && pc.version < '3') RequiredCapability implements getName() getNamespace() getRange() by assuming the underlying expression has the above format and parsing for the respective elements in the expression AST. However when RequiredCapability is used as a general IRequirement the expression does not have to follow that format. RequiredCapability then breaks the contract of the IRequiredCapability interface. Any code that stumbles on such an RequiredCapability instance can test with "instanceof IRequiredCapability" then cast to IRequiredCapability then break upon calling getName() or other such methods. The p2 implementation should have separate implementations for the separate abstractions.
New Gerrit change created: https://git.eclipse.org/r/113152
As an additional remark: It appears that the only supported public API for requirements is org.eclipse.equinox.p2.metadata.IRequirement The package of org.eclipse.equinox.internal.p2.metadata.IRequiredCapability implies it is for internal use by the p2 engine only. IRequiredCapability is a way to optimize the majority of requirements to a (namespace; name; range) triplet in order to improve matching performance and reduce the xml footprint. External users must not rely on any of these things. All public representations of p2 metadata are defined by org.eclipse.equinox.p2.metadata.MetadataFactory and it never returns an IRequiredCapability. I.e. the p2 internals being a closed codebase can workaround the broken nature of the current RequiredCapability class on a case by case basis. This bug should not have been encountered by any non-p2 project.
One broken client is the CBI aggregator at this locations: http://git.eclipse.org/c/cbi/org.eclipse.cbi.p2repo.aggregator.git/tree/org.eclipse.cbi.p2repo.p2/src/org/eclipse/cbi/p2repo/p2/util/P2Bridge.java#n456 Can you recommend how this section should be re-written? AFAICS, this code simply converts p2 data into the EMF model from http://git.eclipse.org/c/cbi/org.eclipse.cbi.p2repo.aggregator.git/tree/org.eclipse.cbi.p2repo.p2/model/p2.ecore If the change in p2 requires to change that model, then the necessary changes in CBI aggregator could turn out to be quite pervasive ...
I am not familiar with CBI and have not used EMF, but from what I can gather the P2Bridge already can handle IRequirement, not only IRequiredCapability. Won't it just work if you remove all handling of IRequiredCapability and just stick with IRequirement? Which should have been the case to begin with? FYI I am not saying I won't fix the bug - just that in the future somehow the p2 "internal" packages should be closed off to the general public. Maybe just don't support backward compatibility for them. If this is not done the p2 project can't be developed further.
(In reply to Todor Boev from comment #4) > I am not familiar with CBI and have not used EMF, If needed, Ed is already here for help with EMF :) (but frankly, EMF has the least part in the problems at hand). > but from what I can gather > the P2Bridge already can handle IRequirement, not only IRequiredCapability. > Won't it just work if you remove all handling of IRequiredCapability and > just stick with IRequirement? Which should have been the case to begin with? I'm not 100% sure, I haven't authored this code, but I could imagine that the EMF representation of these data needs to be "complete" so that after various operations / transformations a new complete p2 repository can be generated (see, e.g., methods exportFromModel()). > FYI I am not saying I won't fix the bug - just that in the future somehow > the p2 "internal" packages should be closed off to the general public. Maybe > just don't support backward compatibility for them. If this is not done the > p2 project can't be developed further. I agree. The specific problem regarding CBI aggregator is: - it's basically in maintenance mode, hardly any active development - the simultaneous release train depends on it So when p2 evolves in ways that break CBI aggregator, this must be coordinated, or SimRel will fail. Hopefully, at the end we have a CBI aggregator without dependency on internals, but we aren't there, yet. Similarly, coordination with Oomph is necessary (bug 528266), but there the situation is much brighter, with Ed actively driving this :)
Certainly this code should not blindly cast: mrqc.setApplyOn((IRequiredCapability) importToModel(rqChange.applyOn())); mrqc.setNewValue((IRequiredCapability) importToModel(rqChange.newValue())); For Oomph I'll probably add this test: public static boolean isSimpleRequiredCapability(IRequirement requirement) { IMatchExpression<IInstallableUnit> matches = requirement.getMatches(); return matches != null && matches.getParameters().length >= 2 && requirement instanceof org.eclipse.equinox.internal.p2.metadata.IRequiredCapability; } which I can probably change just to be an instanceof test once https://git.eclipse.org/r/#/c/113152/ is completed.
FYI: bug 508626 comment 13.
To add insult to injury we don't only have references to internal classes, CBI aggregator illegally implements IProvidedCapability (twice) and thus can not be compiled against p2 >= 4.8M2. Houston ...
(In reply to Stephan Herrmann from comment #8) > To add insult to injury we don't only have references to internal classes, > CBI aggregator illegally implements IProvidedCapability (twice) and thus can > not be compiled against p2 >= 4.8M2. Interested parties could update the CBI aggregator to adapt to the recent changes. Or the CBI aggregator can still be executed against 4.7 and we declare it incompatible with 4.8. I don't think this is a blocker for making p2 better.
More compile errors in CBI aggregator when compiling against https://git.eclipse.org/r/#/c/113152/6 http://git.eclipse.org/c/cbi/org.eclipse.cbi.p2repo.aggregator.git/tree/org.eclipse.cbi.p2repo.aggregator.engine/src/org/eclipse/cbi/p2repo/aggregator/engine/internal/MultiRangeRequirement.java#n255 http://git.eclipse.org/c/cbi/org.eclipse.cbi.p2repo.aggregator.git/tree/org.eclipse.cbi.p2repo.aggregator.engine.maven/src/org/eclipse/cbi/p2repo/aggregator/engine/maven/InstallableUnitMapping.java#n146 The constructor about to be removed *is* used.
(In reply to Mickael Istria from comment #9) > (In reply to Stephan Herrmann from comment #8) > > To add insult to injury we don't only have references to internal classes, > > CBI aggregator illegally implements IProvidedCapability (twice) and thus can > > not be compiled against p2 >= 4.8M2. > > Interested parties could update the CBI aggregator to adapt to the recent > changes. Or the CBI aggregator can still be executed against 4.7 and we > declare it incompatible with 4.8. > I don't think this is a blocker for making p2 better. I strongly disagree. > Interested parties this is all of SimRel, not some exotic group > the CBI aggregator can still be executed against 4.7 No, the CBI aggregator is dead when it finds metadata created by a recent p2. It must be fixed, and I guess it must be fixed against the new API of p2.
(In reply to Stephan Herrmann from comment #11) > No, the CBI aggregator is dead when it finds metadata created by a recent > p2. It must be fixed, and I guess it must be fixed against the new API of p2. Let's keep discussing that on bug 528387.
(In reply to Mickael Istria from comment #12) > (In reply to Stephan Herrmann from comment #11) > > No, the CBI aggregator is dead when it finds metadata created by a recent > > p2. It must be fixed, and I guess it must be fixed against the new API of p2. > > Let's keep discussing that on bug 528387. OK. Two comments before we move over (and before I have to leave this discussion for today): - I do applaud Todor for his efforts - We still need that coordination. As long as CBI aggregator is broken, this issue cannot be closed.
Bringed back the two constructors for cbi but marking them as deprecated and for removal. If CBI is that dead we will face the issue in few months again. Version #11 of the patch set to be delivered in master to ease further testing.
Actually #12 went in so there is @noreference too. Please everyone test with tomorrow's I-build.
Gerrit change https://git.eclipse.org/r/113152 was merged to [master]. Commit: http://git.eclipse.org/c/equinox/rt.equinox.p2.git/commit/?id=cf04c14fb70f9579577a5dcd3df5a71366cbeb97
This might probably caused a test failure in PDE Build: http://download.eclipse.org/eclipse/downloads/drops4/I20171212-2000/testresults/html/org.eclipse.pde.build.tests_ep48I-unit-cen64-gtk2_linux.gtk.x86_64_8.0.html N/A junit.framework.AssertionFailedError at org.eclipse.pde.build.tests.PDETestCase.assertLogContainsLines(PDETestCase.java:294) at org.eclipse.pde.build.internal.tests.p2.PublishingTests.testDirectorLogging(PublishingTests.java:1521) at org.eclipse.pde.build.tests.PDETestCase.runTest(PDETestCase.java:60) at org.eclipse.test.EclipseTestRunner.run(EclipseTestRunner.java:726) at org.eclipse.test.EclipseTestRunner.run(EclipseTestRunner.java:348) at org.eclipse.test.CoreTestApplication.runTests(CoreTestApplication.java:38) at org.eclipse.test.CoreTestApplication.run(CoreTestApplication.java:34) at org.eclipse.equinox.internal.app.EclipseAppContainer.callMethodWithException(EclipseAppContainer.java:587) at org.eclipse.equinox.internal.app.EclipseAppHandle.run(EclipseAppHandle.java:198) at org.eclipse.core.runtime.internal.adaptor.EclipseAppLauncher.runApplication(EclipseAppLauncher.java:134) at org.eclipse.core.runtime.internal.adaptor.EclipseAppLauncher.start(EclipseAppLauncher.java:104) at org.eclipse.core.runtime.adaptor.EclipseStarter.run(EclipseStarter.java:388) at org.eclipse.core.runtime.adaptor.EclipseStarter.run(EclipseStarter.java:243) at org.eclipse.equinox.launcher.Main.invokeFramework(Main.java:653) at org.eclipse.equinox.launcher.Main.basicRun(Main.java:590) at org.eclipse.equinox.launcher.Main.run(Main.java:1499) at org.eclipse.equinox.launcher.Main.main(Main.java:1472) at org.eclipse.core.launcher.Main.main(Main.java:34)
This test fails for me at assertNotNull(delta); I thought we were long done with delta packs, right?
(In reply to Alexander Kurtakov from comment #18) > This test fails for me at assertNotNull(delta); > I thought we were long done with delta packs, right? This is true, but are you 100% sure the changes don't impact other things outside delta pack?
(In reply to Dani Megert from comment #19) > (In reply to Alexander Kurtakov from comment #18) > > This test fails for me at assertNotNull(delta); > > I thought we were long done with delta packs, right? > > This is true, but are you 100% sure the changes don't impact other things > outside delta pack? Still failing. See bug 528899.
Can we close this one now?
I think this can be closed. Any remaining issues with the general capability/requirement support are being addressed by bug 528387
(In reply to Alexander Kurtakov from comment #18) > This test fails for me at assertNotNull(delta); > I thought we were long done with delta packs, right? Perhaps things have changed (more) in the past year or so, but last I recall "we" (the Platform) stopped producing the delta pack, but we did not prohibit users from "rolling their own". See https://wiki.eclipse.org/A_Brief_Overview_of_Building_at_Eclipse#What_if_you_really_want_the_traditional_Delta_Pack_zip_file.3F It is also still mentioned as an option for the packager! https://help.eclipse.org/oxygen/index.jsp?topic=%2Forg.eclipse.pde.doc.user%2Ftasks%2Fpde_packager.htm Hence, I'd say if producing or consuming a delta pack is *now* broken, then at a minimum "we" would owe the community a wide spread announcement. And, as Dani implied, since much of the "delta pack" operations are simply p2 mirroring commands, and similar, if the delta operations are broken, I'd be concerned that something else was as well. [I will admit, I have not looked at the unit test that started this discussion].
(In reply to David Williams from comment #23) > (In reply to Alexander Kurtakov from comment #18) > > This test fails for me at assertNotNull(delta); > > I thought we were long done with delta packs, right? > > Perhaps things have changed (more) in the past year or so, but last I recall > "we" (the Platform) stopped producing the delta pack, but we did not > prohibit users from "rolling their own". See > > https://wiki.eclipse.org/ > A_Brief_Overview_of_Building_at_Eclipse#What_if_you_really_want_the_tradition > al_Delta_Pack_zip_file.3F > > It is also still mentioned as an option for the packager! > https://help.eclipse.org/oxygen/index.jsp?topic=%2Forg.eclipse.pde.doc. > user%2Ftasks%2Fpde_packager.htm > > Hence, I'd say if producing or consuming a delta pack is *now* broken, then > at a minimum "we" would owe the community a wide spread announcement. > > And, as Dani implied, since much of the "delta pack" operations are simply > p2 mirroring commands, and similar, if the delta operations are broken, I'd > be concerned that something else was as well. [I will admit, I have not > looked at the unit test that started this discussion]. The tests are still relying on deltapacks which are generated by our testfamework but this made it way too hard to understand as I didn't expect that the key part for running the tests would be in test.xml file. So no change for the things you were concerned. Still this makes the tests way too hard for maintenance that way so no one should be expected if failures in them are just ignored or disabled. One can't even run/debug them freely from inside the workbench.
