Community
Participate
Working Groups
Related to bug 311576 and bug 275274 in that we are seeing this problem in those scenarios. In the AttachmentHelper class we have code which attaches IU Fragments to IUs. Note this does not mean bundle fragments... they are represented as regular IUs. IU fragments are (for instance) translations for the metadata or can contain touchpoint actions for installing bundles. In the case of the bugs listed above, we have a particular product which is installing several (well over 1000) bundles via the drop-ins mechanism. As a result, if a bundle and its associated nl bundle are both discovered and installed by the drop-ins, we are creating a special translation IU Fragment. The problem is that there is already an IU Fragment on these IUs (the OSGi default one telling us they are bundles and we should call InstallBundle which adds them to the bundles.info file, etc). The code in the AttachmentHelper class only allows a single IU fragment to be attached to an IU. The reasoning behind this is because the touchpoint data could be conflicting on the fragments and we have no way to know this beforehand. So the logic attaches the fragment with the most requirements satisfied. In our case we are seeing the translation fragment attached to the bundle rather than the default OSGi one, and therefore the IU is being installed into the profile but the bundle is not being added to the bundles.info file. This causes the product not to load because it is missing bundles. Whether the translation fragment or the default OSGi one is attached is random. (first come, first served... stored in a Set so based on VM, etc) We discussed this problem and potential solutions. We cannot attach all fragments because of the potential touchpoint data conflict problem. I will attach a patch which takes the approach of: - retaining the current logic of attaching the fragment with the most satisfied requirements - always attaching all the translation fragments Since translation fragments are 1) known by the p2 core and 2) do not contain touchpoint data which can conflict with other fragments, we feel that this is a relatively safe fix. The IU fragment attachment problem in general still needs to be reviewed in the future but this is a temporary fix for the 3.6 release.
(In reply to comment #0) > I will attach a patch which takes the approach of: > - retaining the current logic of attaching the fragment with the most satisfied > requirements > - always attaching all the translation fragments Just to clarify, the current logic of attaching the fragment with the most satisfied requirement can potentially select a translation fragment. So, don't we really mean to modify the current logic a bit to make this selection from the non-translation fragments.
Created attachment 168264 [details] patch Initial patch. There is a bit of weird logic here because we are trying to keep the first element in the array to be the originally selected non-translation fragment. Andrew: yes we are removing the translation fragments from the selection process so they are never chosen and are always added at the end.
Pascal, what are your thoughts on this approach?
Created attachment 168428 [details] alternative patch Here is an alternative patch if we are not concerned about the non-translation fragment always being the first element in the collection.
Created attachment 168464 [details] alternative keeping theFragment first Here is an alternative to the original which keeps the selected fragment as the first one by using LinkedList instead of ArrayList.
Created attachment 168673 [details] alternative using TranslationSupport Constant This builds on Andrews patch and uses the TranslationSupport constant for the localization namespace. Note, I did have to change a field from package visibility to public (non API field), so this might not be a good idea. I'm giving a +1 to andrews patch (I think we should keep the non-translation fragments at the head of the list since they more important, and while I doubt anybody depends on this, you never know). If you think that we should use the localization constant, then you can apply my patch, otherwise we can go with Andrew's.
I vote for Andrew's patch and then a TODO to change the code and use the constant in 3.7.
Pascal, if you have reservations about these changes please comment. We can also talk about this at the p2 call today.
The patch looks safe. However as discussed in yesterday's call we need to understand what has changed since 3.5. Is the metadata different? Also adding a few tests would be good.
I haven't looked at the behaviour in 3.5.2 but the reason we don't see the problem in 3.4.2 is because the code has changed a lot and we aren't following the same code paths. Although the actual code to attach fragments and keep only one remains the same (second part of ResolutionHelper#attachCUs), the context under which we are evaluating the IUs and fragments has changed dramatically. The code has been re-written and no longer uses the OSGi resolver to resolve the state as well there are generator/publisher changes. I am able to reproduce this problem consistently on 3.6 with some debugged/tweaking. Setup and reproducibility is VM dependent. (noted below) - have a bundle and its nl fragment both in the dropins and start - set a breakpoint in DirectoryWatcher#scanDirectories and when you get the file list back, ensure that the bundle is the first element and the nl fragment is the second (VM dependency issue #1) - set a breakpoint in AttachmentHelper and when we build up the iusToFragment list, you will see that there are multiple potential fragments for your new bundle. (the osgi default and the localization one) - depending on which fragment is first in the list, that is the one which will be attached. the list is actually a Set so the order depends on hash. (VM dependency issue #2) - if the localization fragment was first in the list then the bundle would not be installed correctly. if the osgi tooling fragment was first in the list then the bundle would be missing the localization fragment attachment.
I have released Andrew's patch with a comment about using the constant in the TranslationSupport class in the next release.
