Bugzilla – Full Text Bug Listing
| Summary: | Unexpected extra IU generated for org.eclipse.update.configurator | ||||||
|---|---|---|---|---|---|---|---|
| Product: | [Eclipse Project] Equinox | Reporter: | Igor Fedorenko <igor> | ||||
| Component: | p2 | Assignee: | Tobias Oberlies <t-oberlies> | ||||
| Status: | RESOLVED FIXED | QA Contact: | |||||
| Severity: | normal | ||||||
| Priority: | P3 | CC: | irbull, jan.sievers, pascal, t-oberlies | ||||
| Version: | 3.8.0 Juno | ||||||
| Target Milestone: | Kepler M5 | ||||||
| Hardware: | All | ||||||
| OS: | All | ||||||
| Whiteboard: | |||||||
| Attachments: |
|
||||||
Does anyone from the p2 team remember why this was done? The code looks more than questionable - the extra bundle is taken from the runtime of the publisher - so unless there are good reasons, I will apply Igor's patch which removes that code. Pascal, do you remember why this is done? Since we've already passed the test pass day for M4, this change should wait until M5. @Pascal: Any reasons for this which are still valid? I really want to see this patch in M5. I think we need to approach this from the other direction and only delete the code if we know why we don't need it anymore, instead of deleting it if we can't remember why we did it. Since it is exception code, it was most likely done because it was fixing a real problem. (In reply to comment #4) > I think we need to approach this from the other direction and only delete the > code if we know why we don't need it anymore, instead of deleting it if we can't > remember why we did it. So why did we need it? I failed to find the origin of the "add simple configurator" logic. I could only trace it this far: - In 618de17 [1], the code was copied from Generator.java to BundlesAction.java - In 2ccb446 [2] the Generator.java was renamed from something no present in the history of the rt.equinox.p2 repository. So who could still need that code? Probably only the build of org.eclipse.update.configurator and/or org.eclipse.equinox.simpleconfigurator. Do you know how to do a test build of these projects with a publisher with the patch applied? [1] http://git.eclipse.org/c/equinox/rt.equinox.p2.git/commit/?id=618de1745b89297893c6c48e8bd36972a1a92d87 [2] http://git.eclipse.org/c/equinox/rt.equinox.p2.git/diff/bundles/org.eclipse.equinox.p2.metadata.generator/src/org/eclipse/equinox/prov/metadata/generator/Generator.java?id=2ccb446cc885495686cc9eb430c4a98031b3cfc5 We could add a condition based on a System property (default value to maintain current behaviour) and then run a test build with the property set. Having a test case which shows the behaviour helps some scenarios would help. John and I talked about this a bit and it was most likely added when we were doing the initial conversion and needed to have the simple configurator IU but it wasn't in the SDKs that we were generating metadata from. Going forward for this we should: - create test cases for both scenarios - ensure the test cases pass with the fix in place The first test case should: - Run the publisher against an RCP app which doesn't need the simple configurator - use the generated metadata to install the app - run the app and ensure it works There should also be a test case for the same scenario but the app should require the simple configurator. (In reply to comment #7) > - Run the publisher against an RCP app which doesn't need the simple > configurator Is this still a supported use case? I thought that now that most build systems produce p2 metadata, running the publishers on these artifacts again (=re-publishing) is discouraged because it may lead to violations of the id+version=unique metadata assumption. This was marked M6. I'm moving to M7. Does anybody plan on working on this? @DJ: Are you still interested in providing a test for the "publish existing applications" use case? If not, I'd say we apply the patch and see if any of the users of the (non-API) publisher bundle reminds us what this was ever needed for. I'm no longer working on p2 as part of my day job so I don't have time to contribute a test case here. Perhaps Pascal can comment if he remembers why we had the code initially, etc. Since we are sitting at M7, and I don't know why this code is here (and I don't plan on removing code without a very good reason), I'm going to move this out. The patch has been integrated [1] @Igor: Thank you for the contribution. [1] http://git.eclipse.org/c/equinox/rt.equinox.p2.git/commit/?id=98ec6c9475bc2732d92a8800e5deb0bfc436696a |
Created attachment 207514 [details] proposed fix For apparently historical reasons, BundlesAction has special logic that makes org.eclipse.update.configurator bundle "provide" additional installable unit with id org.eclipse.equinox.simpleconfigurator. This causes problems when building org.eclipse.update.configurator bundle with Tycho -- the build fails because two unrelated bundles provide the same IU. Attached is a proposed fix that removes special handling of org.eclipse.update.configurator bundle.