search

saros-project / saros

Open Source IDE plugin for distributed collaborative software development
https://www.saros-project.org
GNU General Public License v2.0
160 stars 52 forks source link

[BUILD] Extract dependency export from core into seperate bundle #1118

Open FKHals opened 3 years ago

FKHals commented 3 years ago

by creating a new bundle called "saros.libratory" which serves as a dependency-container and replaces the previously used Core-bundle ("saros.core") in this role.

This was done to reduce unnecessary coupling between the Core- and the Eclipse-bundle caused by the Core-bundle's dependency exports. Now the Core-bundle serves exclusively as a central library for a Saros- implementation without also serving as a dependency container. Since other packages (Intellij, Server,..) also used the Core's dependencies those now get it's dependencies from the new Libratory-bundle too.

The following bundle-dependency-relations have been identified and get exported by the Libratory-bundle: (scheme: [bundle(s)]: * dependency of that bundles (comments))

[core, eclipse, stf, sever, intellij]:

[core, eclipse]:

[core, eclipse, stf, sever]:

The motivation is also described here.

srossbach commented 3 years ago

Fine and who ensure the correct versioning ?

FKHals commented 3 years ago

Fine and who ensure the correct versioning ?

The correct versioning of what exactly? It would be great if you could go into more detail about your question.

Nothing has changed regarding the versioning of the used packages. What has changed are the locations of the jar files and the bundles that export them.

Drakulix commented 3 years ago

To me this looks like the proposed split between implementation and library-exports. While we could bike shed over the Libratory name, I see nothing fundamentally wrong with this PR.

But I do not want to overshadow anyone's opinion, so @srossbach could you please elaborate? Also @m273d15 could I ask you providing a quick comment regarding, if this does what you had in mind? After all you were the initial author of the linked proposal.

m273d15 commented 3 years ago

@FKHals Hi, great to see that someone is working on the build and dependency stuff 👍

I think my main idea was that the core is a just a library that is used by the Saros implementations and the Saros implementations (eclipse|intellij|vscode) rely on the transitive dependencies of the Core. This might be useful if we are talking about dependencies that are defining the communication protocol between Core and implementation. However, it is still implicit and we should make the dependency explicit. If we are talking about utility libraries (for example org.apache.commons.lang3.time) it makes no sense. I would not use the tuple implementation of my logging library, why should I use the tuple implementation of the Core? -- I hope my initial german explanation was comprehensible ... I needed a few minutes to understand it 😅

I will review and think about the PR as soon as possible.

tobous commented 3 years ago

This breaks the STF, at least our automated setup. See STF run and build scan for more details.

Also, if possible, I would like to get rid of the plain core configuration as part of this change.

It was initially introduced to avoid duplicating libraries (see issue #830 and PR #835). The issue was that IntelliJ and Eclipse plugins handle dependencies differently. For Eclipse, the dependencies are included in the jar. For Intellij, they are packaged as part of the main plugin. This meant that for Saros/I, the core libraries were included twice. Once in the main plugin lib directory and once as part of the Core jar. To avoid this issue, a plain configuration was introduced for the core which does not include the libraries.

But introducing the plain jar also had the side-effect of breaking/deteriorating the IntelliJ code analysis. The core jar and the core-plain jar are seen as separate entities by IntelliJ (as far as I can tell), meaning the code analysis (e.g. showing the usage of a method) only ever shows the results for one of the two jars. This makes refactoring the Core much more cumbersome as it is much harder to find the usages of the exported Core interfaces for all Saros implementations.

@m273d15 Any comment on the feasibility of this request? If it is not feasible as part of this change, can you think of another way to avoid this problem? I find it quite bothersome.

If there are no good solutions for this issue, we could consider introducing a separate "release build" task that uses the plain jar and revert to using the normal core jar during development. This would allow us to still avoid the library duplication for releases without causing issues for the development workflow. But I would prefer a fix rather than a workaround if possible (as introducing a different version/build process just for releases creates the possibility for issues only occurring with this release build, making testing more cumbersome).

m273d15 commented 3 years ago

@FKHals It seems like you are using the CI for executing the STF tests. You can also execute them on your local Linux machine with ./run_stf.sh (needs docker and the corresponding docker group privileges)

FKHals commented 3 years ago

@FKHals It seems like you are using the CI for executing the STF tests. You can also execute them on your local Linux machine with ./run_stf.sh (needs docker and the corresponding docker group privileges)

I'm actually aware of that but i am currently using a Fedora which does not support docker (because of cgroups v2) and i am not sure what would break when i change that so using the CI and its produced artifacts seemed good enough for now.

tobous commented 3 years ago

@FKHals It seems like you are using the CI for executing the STF tests. You can also execute them on your local Linux machine with ./run_stf.sh (needs docker and the corresponding docker group privileges)

I'm actually aware of that but i am currently using a Fedora which does not support docker (because of cgroups v2) and i am not sure what would break when i change that so using the CI and its produced artifacts seemed good enough for now.

If your comments regard the missing cgroups v2 support of docker, this has been fixed with release 20.10.0. Docker should be compatible with Fedora 32 and 33 (see documentation).

m273d15 commented 3 years ago

It is mainly an issue with the deployment. If you update the bundle you also have to ensure that Eclipse will install it (so basically update the version number along with a proper site configuration). Otherwise the worst thing that happens is that the newest Eclipse version will use an outdated bundle.

@srossbach Thank you for mentioning the issue. I already forgot this problem.

I think this PR is an improvement in terms of separation, but adding an additional OSGi bundle also increases the complexity of the dependency resolution. Therefore, I would propose to solve this with the existing OSGi bundles and the Gradle infrastructure.

The introduction of your libratory project has two effects:

  1. You created a new OSGi bundle
  2. You created a separation of the Gradle configurations of Core and libratory and I think this is a good idea.

I think it is possible to implement the 2nd aspect without the 1th.

I created the releaseDep because I wanted to define a certain set of dependencies that is available during runtime if a project A and is not inherited by all projects that depend on A. This was before the java-library plugin was released. This plugin provides different configurations that allow to define a more fine-grained inheritance of dependencies in multi-project gradle projects. The interesting configuration is api which allows you to define the dependencies that are inherited by other projects.

I think you could (example: core):

Why is this an improvement? All dependencies defined with bundle are included in the jar but are not inherited by other projects (example: eclipse). Therefore, we do not use transitive dependencies of the core project. In contrast bundleApi dependencies are included in the jar AND are inherited by other projects. If we had a complete separation the configuration bundleApi would be useless.

I think if you completely understand the configuration inheritance in the java-library project you can re-work the configuration setup and clean the existing solution which was created before the release of java-library. Probably, you can also find a better solution than the -plain workaround.

If you like we can chat about it via Skype, Zoom, Teams, Two cups and a string or smoke signals.