Community
Participate
Working Groups
Build Identifier: N20100727-2000 Currently, p2 product publisher’s logic is bound by the limitations in editor tool (refer to bug 325614) and “useFeatures” attribute in .product file which does not allow the inclusion of both features and bundles in the same product. Technically, both types of artifacts can be listed in one .product file but publisher discards one of the types in runtime depending on “useFeatures” value. This is applicable even in cases when the attribute is not defined in the .product file. This limitation in product definition is quite artificial because p2 engine can successfully handle product units which require both kinds of artifacts. In this sense, such behavior of product publisher seems to be enforced by the limitations in external components on top of p2 rather than any actual problem in the p2 infrastructure itself. Reproducible: Always Steps to Reproduce: 1. Create a product file which contains both bundles and features 2. Add "useFeatures" attribute to product definition with value "true" 3. Run ProductPublisher 4. Observe that the published product lists only features in its requirements
Created attachment 179132 [details] Fix allows the publishing of bundles and features in the same product The proposed fix allows the publishing of bundles and features in the same product only when “useFeatures” attribute is not defined in .product file. Though not entirely backward compatible, this proposal affects very few use cases and solves the issue with no changes in the .product file format and minimal adjustments in p2 publisher’s logic.
I don't think p2 can do this until PDE supports it, otherwise people may end up with stuff in their product they didnt expect.
(In reply to comment #2) > I don't think p2 can do this until PDE supports it, otherwise people may end up > with stuff in their product they didnt expect. As noted above, the proposal is not entirely backward compatible but will affect a very small number of use cases - only .product files that are created without the useFeatures attribute, most probably outside the official tools. The only way for creating .product files in the Eclipse IDE that I know of (File --> New --> Product Configuration) always puts the attribute inside the file. I think that people who care to put both bundles and features in the same product without the knowledge about the useFeatures attribute, will be surprised to see that their product ends up with only the bundles published in the repository. My opinion is that we should give them a chance to develop such products even in the current framework. Currently the product editor has nothing to say about the semantics of .product files without the useFeatures attribute since it is not able to create them (bug 325614) and I see no need to conform with its present limitations. In this sense, I do not consider the patch as backward incompatible but rather as a proposal how to handle product definitions which seem unresolved yet. At least, you can view the patch as hinting at the directions to how the product editor can be developed in the future and paving the way for the adoption of such changes.
Could you please add a test case verifying that the proper requirements are being generated. Thx.
Created attachment 185068 [details] Test case validating publishing product with mixed content
Andrew's point in comment 2 is still key here. .product files are a PDE artifact and the semantics of having both plugins and features listed in the file are not clear. At least they do not match what is being asked for here. So while p2 might be able to publish the structure, the PDE editors and launch infrastructure and perhaps PDE build are unlikely to give matching behaviour. Adding Curtis for his reading pleasure.
Though on a theoretical standpoint I totally agree with the arguments that talk about consistency with PDE, the reality of the situation is that many people are not using the product editor because of its restrictions or its bugs and have already been circumventing its original purpose. IIRC the epp package product file is one such example. I see this as a grass root movement to make improvements to the status quo by people that are using product files on a day-to-day basis and that this could later lead to improvements to other parts of PDE, instead of waiting for the changes to be done in top down approach.
Sure. Don't get me wrong, I fully support the idea of .products with both bundles and features. Grassroots enhancements are also great. All I'm saying is that adding this to product publishing in and of itself does not help that much. If you expect to publish products like this without building then we also have to make product publishing first class. If you're happy using PDE build to publish your products then we have to ensure that PDE build has the expected behaviour. Further, since PDE UI is creating .product files that have plugin and feature content, the semantics of this need to be clear. Otherwise we'll get people making .products using the editor and publishing/building and getting unexpected behaviour (as Andrew mentions in comment 2). So, for example, rather than implicitly allowing mixed content for all non-feature products, perhaps there needs to be a new property (e.g., type=mixed). Then someone clearly had to step outside the current editor world to get the structure described here.
So just to be clear on the steps: 1) Add a new flag in the product file 2) See for adoption in PDE Build 2) See for adoption in PDE UI
In fact, the initial idea for the patch was introducing a new flag but it was discarded very quickly because of the inconsistencies it may cause. Suppose you have both flags "usefeatures=true" and "mixedContent=true" in one file - which one should take precedence ? I believe that prioritization of the attributes will just lead to unnecessary complexity in interpreting the product definition. The patch has the virtue of not breaking the behavior of the product files produced in PDE UI up to now. Currently there is no semantic behind the absence of "useFeatures" attribute and the proposed solution utilizes that niche. Of course, the items in the agenda about the adoption in PDE stay valid regardless of the patch we choose. If you give it a thought, the adoption of the proposed patch by PDE tools would be much easier.
(In reply to comment #10) > In fact, the initial idea for the patch was introducing a new flag but it was > discarded very quickly because of the inconsistencies it may cause. Suppose you > have both flags "usefeatures=true" and "mixedContent=true" in one file - which > one should take precedence ? I believe that prioritization of the attributes > will just lead to unnecessary complexity in interpreting the product > definition. Notice that the property I proposed was not boolean. It was "type". The values would be something like "features", "bundles", "mixed". BTW, we really need to stop using boolean properties. We always get hosed like this. The problem is that we can use the niche you've discovered but the owner of the resource (PDE owns the .product file format) never defined that niche so it may disappear any time. In the end I think everyone agrees on the usefulness of defining products in terms of features and bundles so we should shot for a solution that has a solid future.
Katya, could you please provide a new patch implementing the solution proposed by Jeff. I don't think it is much more invasive and this will get us going. Once we have that in place we will see for changes in other areas of PDE. Thx.
Let's just clarify a few points about the new property to see if we understand you correctly: 1) in the long term, the new property ("type") shall totally replace the "useFeatures" flag as being more flexible in defining the product's contents 2) future versions of PDE tools shall adopt the new property discarding the old one so that .product files shall not be generated with "useFeatures=true/false" any more 3) for backward compatibility, publisher shall continue to support "useFeatures" if found... 4) ...but if by any chance both properties happen to reside in the same .product file, only the new flag shall be taken into consideration. The latter case should happen only if the .product file has been created or edited by hand. Obviously, we do not wish to allow the tools to generate product definitions that have conflicting attributes. In short, my view is that if we are going to introduce a new property whose domain clearly overlaps the semantics of an existing one, one of both must be elbowed out. Is this your view too ?
(In reply to comment #13) That is the direction I was headed. If I was a PDE committer (errr, I am...) I would more readily adopt a non-boolean property as discussed in comment 11. PDE buy-in to changes to the .product file is pretty important. Otherwise p2 has essentially created a file that looks remarkably like a .product file but cannot be reliably used in any of the ways that current .product files are used (other than publishing). IIRC the .product file format only keeps list of bundles and features to allow the user to switch "modes" and not have to recreate the various lists. The editor, validator, and build would not understand new semantics for free and may well undo them. This would be very confusing to users.
Created attachment 190375 [details] New property "type" introduced to replace boolean flag "useFeatures" The patch covers all the requirements listed in the comments above. Test cases added to validate the new functionality.
Moving to Tobias for release in the 3.8 timeframe. The patch no longer applies because of the reorg in the publisher, but I'm pretty sure it is just a matter of applying the patch on the new classes. I reviewed the patch against an older version and it is good.
(In reply to comment #14) > PDE buy-in to changes to the .product file is pretty important. Otherwise p2 > has essentially created a file that looks remarkably like a .product file but > cannot be reliably used in any of the ways that current .product files are used > (other than publishing). So what is the minimum buy-in from the PDE that we require before going forward with this? There seems to be no interest in adopting a "type" attibute in the PDE - for example there have been no reactions to bug 325614. IMHO the product editor should _at least_ display a warning when the product definition contains the (from the PDE perspective) unsupported "type" attribute. Before we have at least this level of "adoption" in the PDE, I am -0 on integrating this change into p2.
Hello Tobias, Being familiar with PDE code, would it be too much of an effort for you to assist with the merge and contribute the suggested minimal change in the product editor ? I completely agree that there must be some degree of adoption in PDE UI. The product file format and PDE should be in sync in the long term. However, the publishing is often used in headless builds too so the support in PDE UIs should not be the only focus here. The product files are also functional outside the PDE environment and, as Pascal pointed out in one of the previous posts, in reality people very often do not create their products in the editors. It is the user needs that should drive the enhancements in publishing and the adjustments in PDE can follow afterwards. In respect to the current proposal, believe me, there is ample ground for its application - e.g. users gain more freedom how to select the components in their products and won't need to add a whole feature to include just one bundle from it or define a new feature for that purpose.
While slightly off topic from the main issue in this bugzilla ... is it possible to patch a .product that consists of features and bundles? (or, even just IUs?) Perhaps there's another bugzilla for that ability? Thanks for any pointers.
I started a discussion on http://dev.eclipse.org/mhonarc/lists/pde-dev/msg02016.html
(In reply to comment #19) > While slightly off topic from the main issue in this bugzilla ... is it possible to patch a .product that consists of features and bundles? (or, even just IUs?) > > Perhaps there's another bugzilla for that ability? This is seriously off topic :) but I'll reply anyway. p2 patches, like the rest of p2, only deal in terms of IUs when it comes to dependency. As such the scenario you mention can be handled. The only difficulty is in authoring this p2 patch since the feature editor does not allow for this nor does the p2.inf. At this point the only way that comes to mind is to use the programmatic API to create your patch and persist it to a repo. I think a bug that needs to be open is against PDE UI to enhance the feature patch editor and probably just turn it into a IU patch editor.
(In reply to comment #18) > Being familiar with PDE code, would it be too much of an effort for you to > assist with the merge and contribute the suggested minimal change in the product > editor ? Sorry, I don't have any experience with the PDE product editor and I don't have any plans to provide a patch there. But still the change shouldn't be a big deal - you could try to convince our (company-internal) product management to have my team do it.
Move all 3.8 bugs to Juno.
The patch is great. I have ported it to master and committed it locally. The push is only waiting for https access to the p2 git repository (bug 352798). We don't need to wait for full support for the type attribute in the product editor (bug 325614), and IMHO we also don't need to wait for the patch to bug 351984 to be committed. (Is there someone on this bug who is also PDE UI committer?)
Many thanks for the cooperation ! Looking forward to solving the problems with the push ! :)
(In reply to comment #25) > Many thanks for the cooperation ! Looking forward to solving the problems with > the push ! :) Sure! Thank you & Katja for the patch :-) Integrated with commit 0871174 Before I forget, one more question: Do you know why Katja called the new attribute "contentType" rather than "type" (as discussed here) in the patch? It seemed unintentional, so I changed it to "type". Is this okay for you?
(In reply to comment #26) > (In reply to comment #25) > > Many thanks for the cooperation ! Looking forward to solving the problems with > > the push ! :) > Sure! Thank you & Katja for the patch :-) Integrated with commit 0871174 > Before I forget, one more question: Do you know why Katja called the new > attribute "contentType" rather than "type" (as discussed here) in the patch? It > seemed unintentional, so I changed it to "type". Is this okay for you? We thought that "type" sounds more like "product type" instead of "product content type" and tried to make it clearer. No other intentions, type is ok as well :)
Updated documentation: http://wiki.eclipse.org/Equinox/p2/Publisher Is this worth going into a new¬eworthy of p2/equinox. How could this happen?
