Bugzilla – Full Text Bug Listing
| Summary: | [refactoring] Generalize naming of Tycho's embedded OSGi runtime | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | z_Archived | Reporter: | Tobias Oberlies <t-oberlies> | ||||||
| Component: | Tycho | Assignee: | Tobias Oberlies <t-oberlies> | ||||||
| Status: | RESOLVED FIXED | QA Contact: | |||||||
| Severity: | enhancement | ||||||||
| Priority: | P3 | CC: | igor | ||||||
| Version: | unspecified | ||||||||
| Target Milestone: | --- | ||||||||
| Hardware: | All | ||||||||
| OS: | All | ||||||||
| Whiteboard: | |||||||||
| Attachments: |
|
||||||||
|
Description
Tobias Oberlies
Created attachment 207529 [details]
core modules dependencies - initial
When looking at the current state of the dependencies between the tycho modules, merging tycho-core and tycho-p2-facade would also require to include tycho-equinox-launching...
Created attachment 207530 [details]
core modules dependencies - after cleanup with minimal changes
... however it turns out that with just minimal changes - the move of four classes - this can be cleaned up to this structure.
Independent on whether we merge tycho-core and tycho-p2-facade or not, this seems like a very good, structural clean-up.
(In reply to comment #2) > Independent on whether we merge tycho-core and tycho-p2-facade or not, this > seems like a very good, structural clean-up. Since this shouldn't affect many files respectively be easy to merge, I will do that now. Please do this on a branch first, I want to see the actual code before I can form my opinion about this change. The first change - removing the dependency between tycho-p2-facade and tycho-equinox-launching - is straightforward, so I have already done it in master (8bb13ff). The change of removing the dependencies onto tycho-equinox is less trivial, so I can prepare a branch for this. My first attempt was to move Tycho-specific classes into tycho-equinox, but then I realized that tycho-equinox is completely independent of tycho. This should probably be kept, but that means that... You'll see in the branch in a minute. (In reply to comment #5) > tycho-equinox is completely independent of tycho. This should probably be kept, ... On second thought, tycho-equinox may depend on the rest of Tycho (obviously as long as there are no circles). If we want to support the embedded Tycho use case however, the rest of Tycho must not depend on tycho-equinox, so that tycho-equinox can be omitted. I have this proposed in the branch bug364837_isolate_tycho-equinox [1] [1] http://git.eclipse.org/c/tycho/org.eclipse.tycho.git/log/?h=bug364837_isolate_tycho-embedder tycho-core defines target platform resolution API and extension points. tycho-p2-facade provides a facade to the p2-based target platform resolution implementation. I believe this arrangement provides good separation of concerns between tycho core and p2-specific implementation. I also believe that "tycho p2 runtime" name correctly reflects what this subsystem does, i.e. provides p2-based target platform resolution and p2 metadata generation/manipulation services to the rest of Tycho. You mention file locking service from bug 347963, but LocalRepositoryP2Indices introduced by commit 0e00411d fits quite well into "p2 metadata generation/manipulation services" definition. I am +0 on renaming "tycho-p2-facade" to "tycho-p2-<something-more-meaningful>", but I am not convinced major rearrangement of functionality between tycho-core and tycho-p2-facade is required or justified. I assume org.eclipse.tycho package present in multiple modules only temporary, until you finish this refactoring. Is this right? I am -1 on tycho-equinox+tycho-equinox-api changes. tycho-equinox+tycho-equinox-api provide generic plexus/osgi bridge which is useful without rest of tycho, for example in nexus-p2 and nexus-onboarding plugins. Lets keep it separate from tycho, maybe even move out of the main git repo to emphasize the fact. Likewise, I am -1 on tycho-equinox-launching changes. tycho-equinox-launching provides generic equinox launching service, which can be useful independently from p2 and tycho. (In reply to comment #7) > tycho-core defines target platform resolution API and extension points. > tycho-p2-facade provides a facade to the p2-based target platform resolution > implementation. I believe this arrangement provides good separation of concerns > between tycho core and p2-specific implementation. I don't see a real benefit of this, and a couple of disadvantages, e.g. we can't get rid of the LocalTargetPlatformResolver because this would break the tests. But if you insist, we can keep it this way. > I also believe that "tycho p2 runtime" name correctly reflects what this > subsystem does, i.e. provides p2-based target platform resolution and p2 > metadata generation/manipulation services to the rest of Tycho. You mention file > locking service from bug 347963, but LocalRepositoryP2Indices introduced by > commit 0e00411d fits quite well into "p2 metadata generation/manipulation > services" definition. The name is too specific. Tycho uses other things from the OSGi world (at least since cf45462). But more importantly, the name stands in my way for new functionality. If I want to implement something, and I may implement it in OSGi, I don't want to be forced to put things into the "tycho-p2-facade". > I am +0 on renaming "tycho-p2-facade" to "tycho-p2-<something-more-meaningful>", > but I am not convinced major rearrangement of functionality between tycho-core > and tycho-p2-facade is required or justified. Actually I am convinced that tycho-core needs access to OSGi implementations. Ask Jan about the hoops he had to jump through for bug 347963. You don't insist on keeping Tycho independent of OSGi, do you? > I am -1 on tycho-equinox+tycho-equinox-api changes. > tycho-equinox+tycho-equinox-api provide generic plexus/osgi bridge which is > useful without rest of tycho, for example in nexus-p2 and nexus-onboarding > plugins. Lets keep it separate from tycho, maybe even move out of the main git > repo to emphasize the fact. Fair enough. As long as this is clear - it wasn't to me. Rather than separating these modules physically, a logical separation would be more helpful, e.g. by removing "tycho" from the name > Likewise, I am -1 on tycho-equinox-launching > changes. tycho-equinox-launching provides generic equinox launching service, > which can be useful independently from p2 and tycho. tycho-equinox-launching depends on tycho-core, and it is not trivial to remove this dependency. But tycho-equinox-launching _should_ be independent of Tycho, right? I'll see if I can make this work. (In reply to comment #8) > (In reply to comment #7) > > Likewise, I am -1 on tycho-equinox-launching > > changes. tycho-equinox-launching provides generic equinox launching service, > > which can be useful independently from p2 and tycho. > tycho-equinox-launching depends on tycho-core, and it is not trivial to remove > this dependency. But tycho-equinox-launching _should_ be independent of Tycho, > right? I'll see if I can make this work. This would require to move the BundleReader from tycho-core to tycho-equinox-launching. This doesn't seem right, so I won't pursue this idea any further. (In reply to comment #9) > (In reply to comment #8) > > (In reply to comment #7) > > > Likewise, I am -1 on tycho-equinox-launching > > > changes. tycho-equinox-launching provides generic equinox launching service, > > > which can be useful independently from p2 and tycho. > > tycho-equinox-launching depends on tycho-core, and it is not trivial to remove > > this dependency. But tycho-equinox-launching _should_ be independent of Tycho, > > right? I'll see if I can make this work. > This would require to move the BundleReader from tycho-core to > tycho-equinox-launching. This doesn't seem right, so I won't pursue this idea > any further. Indeed, there is tycho-equinox-launching->tycho-core dependency already (and I am the one who introduced it). I still believe it would be nice to make tycho-equinox-launching standalone, but we don't have to do this now. "P2ApplicationLauncher" can be used to launch any Eclipse application from Tycho OSGi runtime, so "P2" prefix is not appropriate. Otherwise I am +0 on this change. (In reply to comment #10) > Indeed, there is tycho-equinox-launching->tycho-core dependency already (and I > am the one who introduced it). I still believe it would be nice to make > tycho-equinox-launching standalone, but we don't have to do this now. In order to clarify the purpose of the "tycho-equinox-*", i.e. to be generally usable Plexus/Equinox bridges, their names have been changed to "sisu-equinox-*". Before moving these modules to sisu, we will need to remove the dependency to tycho-core - but I agree that this isn't necessary right now. > "P2ApplicationLauncher" can be used to launch any Eclipse application from Tycho > OSGi runtime, so "P2" prefix is not appropriate. Otherwise I am +0 on this > change. The name P2ApplicationLauncher still needs to be replaced by something appropriate. (In reply to comment #11) > In order to clarify the purpose of the "tycho-equinox-*", i.e. to be generally > usable Plexus/Equinox bridges, their names have been changed to > "sisu-equinox-*". For reference: This change is tracked as bug 364967. Renamed [1] classes in org.eclipse.tycho.osgi.runtime which still contained the term "p2 runtime" to use the general term "osgi runtime" instead. With this, the renaming&restructuring is mostly finished. Leftovers which may be addressed in future: * Break the dependency from sisu-equinox-launching to tycho-core to make sisu-equinox-launching independent of Tycho. * Move and/or rename the P2ApplicationLauncher class (in sisu-equinox-launching). As far as I understand it, the class is used to fork an application with a runtime that includes the same bundles as the embedded OSGi runtime. So it would be good to also move that class into org.eclipse.tycho.osgi.runtime. This is not possible because tycho-core (currently) can't have a dependency to sisu-equinox-launching. Alternatively, this class could be a general utility class to fork applications from any EquinoxRuntimeLocator. In this case sisu-equinox-launching is probably the right place, but the name would need to be changed. [1] http://git.eclipse.org/c/tycho/org.eclipse.tycho.git/commit/?id=9bd74cf819230c12b6e8466ce5bfcd3f93f8101b |