Community
Participate
Working Groups
Although there are many p2.inf-based workarounds for defining nested categories, it would be much cleaner or easier to allow definition of nested categories directly in category.xml.
I see that CategoryXMLAction APIs are currently written so that it appears a category should allow only one parent. I think there are use cases where a category could be in 2 parents (think for example of m2e-wtp integrations which could be in a webtools category as well as in a Maven one). Does anyone have any objection to add ability to have multiple parent categories in API ?
Created attachment 230460 [details] Patch supporting Nested Categories The following patch adds support for nested categories. Some insights: * nested categories are treated as "standard" IUs, because it works like a charm that way. Then I deprecated the method which has a confusing signature and useless parameter. * The current patch does not check circular reference in nested categories. When circular reference happens, it will arbitrary (based on the output of the sort algorithm without order transitivity) break one or several links. * Tested an included unit test and with p2 installer UI. It seems works well. This patch can be seen on GitHub https://github.com/mickaelistria/rt.equinox.p2/commit/cb3f1acc8bf955c648938f5e3516b35c251fc8e4 On a side note, I noticed that a lot of code in SiteXMLAction could be replaced by a simple sort on "Categorizable" elements: take all IUs, and sort them in a way that make that InstallableUnits of "leaves" get added first and iterate until the top-level installation units, and it should build the whole site in a single-block.
I pushed your patch http://git.eclipse.org/c/equinox/rt.equinox.p2.git/commit/?id=98da82bec04ad8547892cb4a9e7da1ce9bb1b92c to make sure it is integrated and we get feedback asap. I will do a more thorough review in the coming days. Quick question: - What happens if you mark the nested categories with the property that identifies categories If this can't be done (which I believe it can't otherwise the UI would show nested categories as top level categories)? Could you please see to add a property to the nested categories so they get easily identifiable. - Could you update the copyright headers?
(In reply to comment #3) > I pushed your patch > http://git.eclipse.org/c/equinox/rt.equinox.p2.git/commit/ > ?id=98da82bec04ad8547892cb4a9e7da1ce9bb1b92c to make sure it is integrated > and we get feedback asap. I will do a more thorough review in the coming > days. Thanks a lot! > Quick question: > - What happens if you mark the nested categories with the property that > identifies categories If this can't be done (which I believe it can't > otherwise the UI would show nested categories as top level categories)? > Could you please see to add a property to the nested categories so they get > easily identifiable. The editor UI currently shows all categories as root categories. When this category.xml file is exported into p2 metadata, the installer UI seems to work well and does only show nested categories as children of parent category (no root). Wouldn't just fixing category editor to check parents be enough. > - Could you update the copyright headers? I'll do it ASAP.
Created attachment 230502 [details] Patch to update copyright headers
Created attachment 230529 [details] Some examples (highlighting a bug) This zipped workspace contains 3 examples for the nested category support. It shows that multiple-depth can sometimges lead to a wrong result because the categories are not added in the right order. Comparator at http://git.eclipse.org/c/equinox/rt.equinox.p2.git/tree/bundles/org.eclipse.equinox.p2.updatesite/src/org/eclipse/equinox/internal/p2/updatesite/SiteXMLAction.java?id=98da82bec04ad8547892cb4a9e7da1ce9bb1b92c#n441 is not transitive. Ie, with this comparator, * nested category is in root category * nested nested category is in nested category does NOT imply * nested nested category is added before root category. This needs to be fixed. Beware that depending on the initial order (as declared in category.xml) of categories, the same comparator can lead to good results "by luck".
Comment on attachment 230502 [details] Patch to update copyright headers This patch has a bug (it removes some imports).
Created attachment 230569 [details] Fix for multiple levels of nested categories This second patch tests and fixes the bug with multiple level of nested categories and fixes copyrights (so previous copyright patch is now obsolete).
I've applied the patch and verified that things are working as expected. Thanks for the contributions Mickael.
Mickael, I think it would be good to add support to the PDE editor. Though this is probably out of scope for this release, this may be something that could be added in SR1.
Thanks for the fast review and merge. I'm working on the PDE editor, and I'm not that far from a good-enough-to-submit patch. I'll track my progress on the PDE side on bug 296392.
Sorry to be the grinch, but, we are in RC1 now, which means: 1. This is feature work, which means it needs PMC approval BEFORE it gets committed. 2. Another committer must agree that it's worth fixing the bug and once a fix is available (in this case PMC approval does that trick), it must be reviewed and the review+ flag must be set in this bug BEFORE the code is committed. http://www.eclipse.org/eclipse/development/plans/freeze_plan_4_3.php explains the endgame in detail.
Although I'm not a committer, I'd like to advocate and highlight that this feature is worth being fixed in Kepler (and upcoming Tycho versions using Kepler), since nested categories are something that have been requested several times -Google "eclipse p2 nested categories" for instance, for Eclipse projects and external projects, and that the only current way to achieve that is to use a p2.inf file which is more like a hack. Democratizing nested categories would have big benefit for end-users who will find it easier to understand which pieces of a project to install. As an example of Eclipse project that would benefit from this feature, I can think about Orbit which uses a lot of hacks instead of a clean approach as the one suggested here, and for non-Eclipse project, that's something we'd really like for JBoss Tools. About the "feature works", you can see that the patch comes with some new tests and they did not break any previous tests.
I need some other p2 committer to give me an opinion before I agree to +1 this. I will say that I think we need to augment the rules in Equinox for PMC approvals. I'm the only RT PMC member that would have any business making such approvals for rampdown process for the Equinox release, but for p2 I don't usually have enough knowledge to decide. I think we need to consider allowing the project leads for p2 to make 'PMC' level decisions for the p2 project. To be honest, in most cases if Pascal has deemed it safe and necessary then I would have to agree with him for p2. At any rate, Pascal for future bug fixes during the rampdown process please get another p2 committer to review.
Actually Ian Bull is now an RT PMC member. He is the best PMC member to approve p2 things.
For the benefit of others reviewing this: p2 already supports nested categories at runtime. This patch just addresses the authoring time/build time problem of allowing the category.xml source file to support authoring them. As such I consider it reasonable level of risk for RC1 - there is no change to runtime installer code. I gave the patch a quick review but didn't try it out. It looks good to me and I'm comfortable with it going in. I have to say I'm really impressed by the patch quality and by all the contributed test suites for verifying it. I love to see this kind of quality contribution!
At first I was reluctant to include feature work in RC1, but there is certainly a good case for this. Because of the churn required to get things into our build: 1. p2 needs to add this and produce a build 2. Tycho needs to update their dependencies to include a new p2 3. Tycho needs to release 4. CBI needs to pick-up the new Tycho If we push this off until SR1, we likely won't get access to this build time feature until Luna M4 (Dec 2013). That seams like a long time to wait for something we already have runtime support for. Since this patch doesn't affect any runtime components (other than the update site publisher), I'm leaning towards a +1. Let me take a closer look at the patch before I make it official though. Thanks everyone!
(In reply to comment #17) Let me take a closer look at the patch before I make > it official though. Ian, our RC1 candidate build is tomorrow.
Yup, looks good. Thanks.
