Community
Participate
Working Groups
Required and Provided capabilities don't currently have an interface, making it difficult to provide custom implementations. I would be happy to provide (and am currently working on) a patch for this. Thoughts?
While the offer still stands to submit a patch for this, it may be easier to run the refactor instead. Does a patch make sense in this case?
If we did want to allow custom implementations, classes would be preferred over interfaces, since an interface that can be implemented by clients can never evolve. However, there's a deeper problem with custom implementations: the provided/required capabilities have to be encoded as a boolean satisfiability problem for consumption by SAT4J. Allowing custom implementations of this would be tricky. Having said that, the current provides/requires mechanism is quite flexible so it would be interesting to hear what kind of constraints you want to express that the current mechanism can't handle.
Can you say more about the interaction with SAT4J? Does it reach into the implementation at all? If it just calls the public API of *Capabilty then we should be good with different implementations no? As for examples... Given various implementation choices (eg., using EMF or various database technologies) there may be requirements to extend other classes for easier/faster serialization. You could also imagine various optimized repo implementations that would manage the numerous capability instances differently and so want a different implementation. Note that given the equals() of the current implementations, the class should be marked final as we are comparing classes directly rather than checking instanceof.
I see. That kind of implementation change should be feasible. I was thinking Ian wanted to override things like ProvidedCapability.satisfies to insert different logic for matching provides with requires. The interaction with SAT4J should be fine as long as you don't introduce new matching logic. We could use interfaces here as long as it didn't constrain us from adding methods in the future. I.e., clients wouldn't be allowed to implement, but service implementators could - in the standard OSGi style where future service versions would be free to add methods. Presumably if you want to do things like changing superclasses, then an abstract base class instead of an interface won't cut it.
John, No, I don't want to change the operations, just the storage, and yes, possibly the class hierarchy. In the case of capabilities, it seems like most of the class is private data with getters and setters. The only logic I saw was ProvidedCapability#satisfies. (This also exists in InstallableUnit). I wonder if this could be moved out somehow, so that these classes are simply just data structures?
Pascal, I'm happy to consider this an enhancement, as it is clearly not a visible problem with p2, although, if we do decide to implement this enhancement we should probably do so earlier rather than later, since: a. it has a pretty far reaching effect (touching a bunch of classes), and b. it will help define API around core p2 components (metadata). It would be good to get the API in place well before M6 for testing. As for John's point about service implementations, while never ideal, it is probably a reality. Maybe when 4.0 ships we can lift that restriction :-). Also, we may want to consider the same time of refactoring to TouchpointType, TouchpointData and TouchpointInstruction.
I agree that this kind of thing needs to be done early and concur with it potential breadth. However I'm not worried about the API side of things as much as the main focus in this release is to make API for users not extenders, and even in that event, the current thinking is to make IU a relatively opaque token.
This should not change the opaqueness of IUs since it would be SPI-related and thus not exposed to API programmers. Nor should it be a particular impact on the code (modulo a lot of type changes). It may even help with future API (end user) evolution and opaqueness.
I have created a repo that uses custom implementations of all the metadata classes. In particular, I have created interfaces for: License (ILicense) Copyright (ICopyright) ProvidedCapability (IProvidedCapability) RequiredCapability (IRequiredCapability) TouchpointData (ITouchpointData) TouchpointInstruction (ITouchpointInstruction) TouchpiontType (ITouchpointType) There are only a few small problems (that I think can easily be addressed) 1. I had to clone the satisfies method (in IU and ProvidedCap). 2. I had to create .equals / hashCode methods (we should document what .equals means). This of course is expected of any SPI, but we just need to make the docs clear 3. ArtifactKey#toExternalForm and #parse need to be supported. I wanted to keep the behaviour the same, so I moved SEPARATOR to the interface. 4. Some of the .equals methods (on the existing classes) do explicit casts to the concrete type. This should be changed to use the interface So really, these classes are just beans, and I think with minimal work we can refactor them so this is clear, and others can contribute service implementations. I'm still not sure how well the two implementations will "play together", that is, if I have a simple Metadata repo and my own, but I'm confident we can work those details out with some more testing and small tweaks (if needed). Thoughts?
I'm still trying to wrap my head around what value anyone gets from re-implementing these classes. These objects are just values / data structures rather than references or handles. As they are today they don't imply any particular storage. If these objects became handles referring to some backing storage, would they become objects whose values could change if the backing storage changed? What would happen for example if I did this: IInstallableUnit unit1 = ...;//some IU InstallableUnitDescription desc2 = new InstallableUnitDescription(); desc2.setRequiredCapabilities(unit1.getRequiredCapabilities()); IInstallableUnit unit2 = MetadataFactory.createInstallableUnit(); And unit1 was pulled from some metadata repository that is being changed (say a build or mirroring operation is going on). Would a change to unit1 affect unit2? Or in this case: IMetadataRepository repo1 = ...;//some repo IMetadataRepository repo2 = ...;//another repo Collector units = repo1.query(...some query...); repo2.addInstallableUnits(units.toArray(IInstallableUnit.class)); I.e., some IUs are copied from one repository to another. Now the IUs in repo1 are changed, will it affect repo2? (for example LocalMetadataRepository just hangs onto the IU objects passed into the addInstallableUnits method). It seems to retain their current semantics, these objects would have to remain as immutable value objects (simple data structures), in which case I'm having trouble seeing what an alternate implementation buys you.
> It seems to retain their current semantics, these objects would have to remain > as immutable value objects (simple data structures), in which case I'm having > trouble seeing what an alternate implementation buys you. > When we make changes, the IUs would likely have to be tied to a particular implementation. That is, IUs from implementation, would probably not work if you provided Capabilities from another. The reason for this request is so I can make use of alternative implementations and representations of IUs. For example, EMF. Without the interfaces, I will have to create my own representation of an IU (capability, touchpoint, etc..) and then map that to the existing representation during the query. This works, but I end up with two different representations at all times.
John, You would have a better sense than me, but could the InstallableUnitDescriptor be considered "tied" to the current metadata implementation. That is, the descriptor will act on the actual metadata classes (not the interface). In my case, I have a different manager to control IUs, and I only expect my particular classes. This would force all add operations to use a particular implementation of metadata (i.e. you could not add my capabilities to your IU), but the get operations are generic. As for the: IMetadataRepository repo1 = ...;//some repo IMetadataRepository repo2 = ...;//another repo Collector units = repo1.query(...some query...); repo2.addInstallableUnits(units.toArray(IInstallableUnit.class)); this is when I actually check that particular IU that is being added. If it is not one of mine, I create it. (This way I can mirror a simple metadata repo into one of my own). Again, this may not make sense, but I'm willing to prototype it.
I have separated the setting from the getting. Since the interfaces only expose get methods, I have changed these to use the interfaces (if you ask for the required capabilities, you get the IRequiredCapabilities). However, I left the InstallableUnitDescription set methods (add required capability, etc..) using the concrete classes. This means that you cannot add my required capabilities to your installable units. It also means that you cannot implement required / provided capabilities without implementing your own IU. This is a pretty big change, and since I could not use the automatic refactoring, we will need to do some heavy testing before putting this in. I may need to bug someone to help me setup a test environment for p2. (Note: I made the changes against M3, but if we decide to go forward wit this, I will make them against head).
Created attachment 120442 [details] patch to create interfaces for the metadata classes I got the latest from head (Sunday night), and I have prototyped the patch. There were 15 test failures before I started (around reconciler.dropins, TestInstallSDK, and TestEclipseDataArea), and the same 15 tests still fail (but no more). There is still more I can / will do (mainly documentation, but there may be other small issues that arise). This patch should be pretty safe for p2, but for other providers (such as myself) it may need some tweaking. Does it make sense to apply the patch, and then fix other things as they come up? The patch affects a number of p2 bundles. Namely: o.e.equinox.p2.director o.e.equinox.p2.engine o.e.equinox.p2.metadata o.e.equinox.p2.metadata.repository o.e.equinox.p2.publisher o.e.equinox.p2.tests o.e.equinox.p2.tests.ui o.e.equinox.p2.tools o.e.equinox.p2.tools.touchpoint.eclipse o.e.equinox.p2.tools.touchpoint.natives o.e.equinox.p2.ui o.e.equinox.p2.ui.admin
This patch doesn't seem to fix up any references to the concrete classes. I would expect when finished that there would be close to zero references to the concrete classes (perhaps only MetadataFactory). With this patch I still see 500+ references to classes such as RequiredCapability.
Good catch John, I think the patch can be tightened up quite a bit, although there are still a lot of places that need to use the concrete classes. The metadata factory is a good place to start, but anything that calls "createRequiredCapability", also needs to handle the concrete class (All the metadata IO stuff too). Plus there is a lot of places that create arrays of requiredCapabilities. These arrays are often passed around and eventually make there way to setRequiredCapabilities, which use the concrete classes. I'll tighten the patch up a bit. Thanks!
Note there is a "use interface where possible" refactoring that should do most of the work here.
(In reply to comment #17) > Note there is a "use interface where possible" refactoring that should do most > of the work here. > I did that first, however, that doesn't protect against using one providers capability with another providers IUs. This patch does the opposite, never use interfaces for any P2 things, except for where absolutely necessary. Right now I am trying something in between. Use interfaces where possible, but then change the setter methods on the IU to use the concrete classes, (i.e. setRequiredCapabilities(RequiredCapability[] ...). {Note the concrete class here]). I am now working backwards through p2 to make this patch work.
After using the extract interface refactor, I then changed the set method (on InstallableUnit) to use the concrete RequiredCapability Class. I then fixed all compile errors and there are 370 uses of the concrete class. Most of these come from the test cases, but they are in other places too. The reason is because of the following pattern: new RequiredCapability []{ factory.createRequiredCapability(...)} or new (RequiredCapability[]) someList.toArray(new RequiredCapability[...]); These two patterns are used everywhere, and account for almost all the references to the RequiredCapability class.
I looked through the patch and I'm afraid that this is not ready and some aspects have been overlooked. - IInstallableUnit patch has not been changed to return interfaces - The static references in InstallableUnit should probably be changed - Given that your plan is to enable ppl with full implementation of the interfaces, I don't see how this can work w/o fixing the references to RequiredCapability. - Please don't add the /* (non-Javadoc) * @see org.eclipse.equinox.internal.provisional.p2.metadata.IRequiredCapability#getName() */ - We need a test showing that a non standard p2 IU can be properly persisted - Try a complete SDK install with changing the implementation objects to be yours.
(In reply to comment #20) > I looked through the patch and I'm afraid that this is not ready and some > aspects have been overlooked. > - IInstallableUnit patch has not been changed to return interfaces I have fixed this, thanks! > - The static references in InstallableUnit should probably be changed Would this not be the particular p2 implementation of these interfaces, and thus should use the concrete classes? > - Given that your plan is to enable ppl with full implementation of the > interfaces, I don't see how this can work w/o fixing the references to > RequiredCapability. I think this is the crux of the problem and here is what it boils down to. How should the following method be specified? InstallableUnit#setCapabilities(ProvidedCapability[] newCapabilities) or InstallableUnit#setCapabilities(IProvidedCapability[] newCapabilities) (Note: these are only specified on the class, not the interfaces) If we go with the second, then there will be almost no references to the concrete classes. If we go with the first, there will be > 300 references.
I don't see how you can go with InstallableUnit#setCapabilities(ProvidedCapability[] newCapabilities) without introducing a nightmare of casting...
(In reply to comment #22) > I don't see how you can go with > InstallableUnit#setCapabilities(ProvidedCapability[] newCapabilities) without > introducing a nightmare of casting... > It is actually not that bad. 1. There are no untyped lists (p2 just uses arrays for this) 2. p2 does a great job separating the wiring / creating IUs from the reading of them. If something is being created, it creates the concrete class, and uses this all the way down to the setCapabilities call. If something is being read from an IU, it uses the interfaces. 3. There are no calls such as setCapabilities(unit.getCapabilities()). So once all the compile errors were fixed (first use the Eclipse refactor, then change the mutators to use the classes), the code runs. I would be happy to use the Interface here (it would be a lot easier for me, since the patch takes a long time to prepare as I don't have to change all the uses of setCapabilities), I just wanted to ensure that different implementations could not be mixed. If you would rather, I will change it to use: InstallableUnit#setCapabilities(ProvidedCapability[] newCapabilities)
>1. There are no untyped lists (p2 just uses arrays for this) See InstallableUnitDescription#addProvidedCapabilities(Collection) and InstallableUnitDescription#addRequiredCapabilities(Collection) > 3. There are no calls such as setCapabilities(unit.getCapabilities()). See InstallableUnitDescription Overall, my worry here is not so much for the current use of the RequiredCapability in the p2 code for which everything will be fine, however it is more for cases where you try to have your own impl of these classes (btw I'm not convinced that EMF gives you much here or that you loose any EMF goody by directly creating p2 objects, I would highly recommend to not promote the implementation of IRequiredCapability as an extension mechanism) and the potential CCEs. Despite the fact that it seems that all RequiredCapability are always used with the implementation class, I don't know how you can be sure that all code paths using RequiredCapability lead to InstallableUnit. Also I'm not sure that the mixing and matching is really avoidable and should be of worry.
After today's call, I have updated the patch. One more question. There is a public static final TouchpointType called NONE (See TouchpointType#NONE). There are a few ( < 10 ) places where p2 compares something to this via == getTouchpointType() == TouchpointType.NONE In a custom model, this equality won't necessarily hold. Does it make sense to change these few places to use .equals? (.equals() just compares ID and Version).
I think it makes sense to do the .equals check.
Created attachment 121187 [details] Updated Patch This patch incorporates John's and Pascal's comments. I still have to complete the test cases. I also need to make a few changes to PDE. I will open a bug (and attach a patch) for those changes.
Created attachment 121190 [details] Patch with testcases This patch includes some test cases. 1. Writes an SPI implementation of an IU 2. Writes an SPI implementation of an IU with a regular (default) provided capability 3. Writes a regular (default) IU with an SPI required capability. All three test cases read the repo back to ensure that the expected types are written.
Created attachment 121197 [details] Patch - 8:00pm Dec 23 This adds another test cases to check .equals. This test case ensures that the .equals on the default implementation considers SPI implementations. This patch also includes some documentation around .equals().
Created attachment 121223 [details] Patch with updated docs, .equals() tests and one more fix. This should be it! I'm happy with these test cases. This patch fills out the rest of the .equals tests for all the SPI implementations of the interfaces. It actually found a few small issues with the existing implementations of .equals (yah! for tests), which I have fixed. This also added test cases for IRequirementChange. I also added / updated some of the documentation. Enjoy the holidays everyone :)
Created attachment 121328 [details] Updated Patch, Dec 29 Jeff reviewed the patch and gave me some comments, these are the fixes. In particular: - Updated copyright notices where they were wrong - Fixed imports (seems that organize imports doesn't run after a refactor) - Added documentation around .equals(). - Changed the NONE touchpoint. Callers can use == on the NONE touchpoint, and implementors *must* use the NONE touchpoint defined in ITouchpointType
Comment on attachment 121328 [details] Updated Patch, Dec 29 committed with a few minor changes.
The latest patch has been released with minor modifications. There is an isssue in one test case where it passes on some JREs (likely a hash ordering issue). I commented out the test and opened bug 259793 to track the issue.
I think the iplog+ flag was added to this bug incorrectly, so I am removing it. Adding this flag to the bug flags every comment in the bug as an IP contribution in the Equinox IP log. I think Ian's patch covers the interesting IP here and it has the iplog+ flag on the patch, which is all we need.
I saw some changes coming in that were unrelated to the fix - it looked like some changes in the organization of imports, where the classes imported were not actually changing, just the order. Not a big deal right now, but will be later in the cycle for reviewing patches. Is it possible that one of you were using customized settings for these projects?
(In reply to comment #35) > I saw some changes coming in that were unrelated to the fix - it looked like > some changes in the organization of imports, where the classes imported were > not actually changing, just the order. > > Not a big deal right now, but will be later in the cycle for reviewing patches. > Is it possible that one of you were using customized settings for these > projects? > Actually, the refactor doesn't properly organize imports (Bug #244458). So, we explicitly had to "organize imports" and it turns out that some other classes didn't have properly organized imports either (mostly related to the version stuff that was recently done). Since both patches came so close together, and it would have been difficult to only organize imports with respect to this work, we felt it was better to fix them all at the same time. I guess the other option would have been to release this patch (with improperly organized imports), and then created a new bug called fix p2 imports. Hopefully with Bug# 244458 fixed, we won't have this problem :)
(In reply to comment #36) <snip> > Actually, the refactor doesn't properly organize imports (Bug #244458). So, we > explicitly had to "organize imports" and it turns out that some other classes > didn't have properly organized imports either (mostly related to the version > stuff that was recently done). Since both patches came so close together, and > it would have been difficult to only organize imports with respect to this > work, we felt it was better to fix them all at the same time. Now that you mention this I vaguely remember some chatter about this problem in the mailing list. > > I guess the other option would have been to release this patch (with improperly > organized imports), and then created a new bug called fix p2 imports. Oh..gosh no...I just wanted to make sure we knew why it happened... Thanks for the explanation.
