[BUILD] Extract dependency export from core into seperate bundle #1118
Conversation
|
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. |
|
To me this looks like the proposed split between implementation and library-exports. But I do not want to overshadow anyone's opinion, so @srossbach could you please elaborate? |
|
@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. I will review and think about the PR as soon as possible. |
|
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 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 But introducing the plain jar also had the side-effect of breaking/deteriorating the IntelliJ code analysis. The @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). |
FKHals
force-pushed
the
feature/libratory
branch
from
dd07235
to
ac4f872
Compare
|
@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 |
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 |
There was a problem hiding this comment.
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?
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. The introduction of your libratory project has two effects:
I think it is possible to implement the 2nd aspect without the 1th. I created the I think you could (example: core):
Why is this an improvement? 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 If you like we can chat about it via Skype, Zoom, Teams, Two cups and a string or smoke signals. |
| org.apache.logging.log4j.status, | ||
| org.apache.logging.log4j.core.lookup, | ||
| org.bitlet.weupnp, | ||
| org.jivesoftware.smack, |
There was a problem hiding this comment.
There are still custom smack patches in the core (see core/patches/org/jivesoftware/smack/).
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)
[core, eclipse, stf, sever, intellij]:
* log4j-1.2-api-2.13.3.jar
* log4j-api-2.13.3.jar
* log4j-core-2.13.3.jar
* picocontainer-2.11.2-patched_relocated.jar
[core, eclipse]:
* weupnp.jar
* smackx-3.4.1.jar
[core, eclipse, stf, sever]:
* smack-3.4.1.jar (Patches for the "smack"-bundle have also been moved from core to
(libratory)
Also the "xpp3"-bundle has been moved to libratory which prevents the following error:
"java.lang.LinkageError: loader constraint violation: loader [...] previously
initiated loading for a different type with name "org/xmlpull/v1/XmlPullParser"
The error is triggered by the fact that both the "smack"- and the "xpp3"-bundles contain
the library "org.xmlpull" while "xpp3" was previously still contained in the core-bundle.
That seemed to cause the error since both bundles tried to load the same library.
If both packages (smack, xpp3) are contained in the same bundle (which is now the
libratory- and was previously the core-bundle) then it does not seem to cause a problem
which is the reason for moving xpp3.
The error is explained further in the following two sources:
* http://frankkieviet.blogspot.com/2009/03/javalanglinkageerror-loader-constraint.html
* https://stackoverflow.com/questions/18127431/spring-java-lang-linkageerror-loader-constraint-violation-loader-previously-in/18315346#18315346
FKHals
force-pushed
the
feature/libratory
branch
from
c95fc84
to
a22d557
Compare
[BUILD] Separate transitive dependencies in core so that the core can act as a dependency-container without leaking it's own (non-transitive) dependencies into the classpaths of the packages importing it while still providing the transitive dependencies that are needed by those packages. This is implemented by creating the two new configurations "bundle" and "bundleApi" which replace the previously used "releaseDep". Dependencies of the "bundle"-configuration gets extend from the "implementation"-configuration (by the gradle-java-plugin) which is not transitive and therefore it's dependencies are only visible to the core package itself while the dependencies in "bundleApi" (which the corresponding "api"-config extends from) can be used by all packages that import the core. Extension in the context of configurations means that that all the dependencies in the "bundle"-config also get added to the "implementation"-config. This solution has the advantage to other solutions (e.g. creating a new OSGI package) that the complexity of the build system is not significantly increased and no versioning of any additional OSGI packages needs to be considered. This solution was proposed and explained to the authors by @m273d15 following the PR saros-project#1118. (saros-project#1118) Co-authored-by: FKHals <30548867+FKHals@users.noreply.github.com> Co-authored-by: paun42 <75132590+paun42@users.noreply.github.com> Co-authored-by: panphil <74254708+panphil@users.noreply.github.com>
so that the core can act as a dependency-container without leaking it's own (non-transitive) dependencies into the classpaths of the packages importing it while still providing the transitive dependencies that are needed by those packages. This is implemented by creating the two new configurations "bundle" and "bundleApi" which replace the previously used "releaseDep". Dependencies of the "bundle"-configuration gets extend from the "implementation"-configuration (by the gradle-java-plugin) which is not transitive and therefore it's dependencies are only visible to the core package itself while the dependencies in "bundleApi" (which the corresponding "api"-config extends from) can be used by all packages that import the core. Extension in the context of configurations means that that all the dependencies in the "bundle"-config also get added to the "implementation"-config. This solution has the advantage to other solutions (e.g. creating a new OSGI package) that the complexity of the build system is not significantly increased and no versioning of any additional OSGI packages needs to be considered. This solution was proposed and explained to the authors by @m273d15 following the PR saros-project#1118. Co-authored-by: FKHals <30548867+FKHals@users.noreply.github.com> Co-authored-by: paun42 <75132590+paun42@users.noreply.github.com> Co-authored-by: panphil <74254708+panphil@users.noreply.github.com>
so that the core can act as a dependency-container without leaking it's own (non-transitive) dependencies into the classpaths of the packages importing it while still providing the transitive dependencies that are needed by those packages. This is implemented by creating the two new configurations "bundle" and "bundleApi" which replace the previously used "releaseDep". Dependencies of the "bundle"-configuration gets extend from the "implementation"-configuration (by the gradle-java-plugin) which is not transitive and therefore it's dependencies are only visible to the core package itself while the dependencies in "bundleApi" (which the corresponding "api"-config extends from) can be used by all packages that import the core. Extension in the context of configurations means that that all the dependencies in the "bundle"-config also get added to the "implementation"-config. This solution has the advantage to other solutions (e.g. creating a new OSGI package) that the complexity of the build system is not significantly increased and no versioning of any additional OSGI packages needs to be considered. This commit includes just the preparations for the following commits that actually make this happen. So at this moment the new configs just get extended from the old "releaseDep" for compatibility reasons (this will change in the following commits). The solution was proposed and kindly explained to the authors by @m273d15 following the PR saros-project#1118. Co-authored-by: FKHals <30548867+FKHals@users.noreply.github.com> Co-authored-by: paun42 <75132590+paun42@users.noreply.github.com> Co-authored-by: panphil <74254708+panphil@users.noreply.github.com>
so that the core can act as a dependency-container without leaking it's own (non-transitive) dependencies into the classpaths of the packages importing it while still providing the transitive dependencies that are needed by those packages. This is implemented by creating the two new configurations "bundle" and "bundleApi" which replace the previously used "releaseDep". Dependencies of the "bundle"-configuration gets extend from the "implementation"-configuration (by the gradle-java-plugin) which is not transitive and therefore it's dependencies are only visible to the core package itself while the dependencies in "bundleApi" (which the corresponding "api"-config extends from) can be used by all packages that import the core. Extension in the context of configurations means that that all the dependencies in the "bundle"-config also get added to the "implementation"-config. This solution has the advantage to other solutions (e.g. creating a new OSGI package) that the complexity of the build system is not significantly increased and no versioning of any additional OSGI packages needs to be considered. This commit includes just the preparations for the following commits that actually make this happen. So at this moment the new configs just get extended from the old "releaseDep" for compatibility reasons (this will change in the following commits). The solution was proposed and kindly explained to the authors by @m273d15 following the PR saros-project#1118. Co-authored-by: FKHals <30548867+FKHals@users.noreply.github.com> Co-authored-by: paun42 <75132590+paun42@users.noreply.github.com> Co-authored-by: panphil <74254708+panphil@users.noreply.github.com>
so that the core can act as a dependency-container without leaking it's own (non-transitive) dependencies into the classpaths of the packages importing it while still providing the transitive dependencies that are needed by those packages. This is implemented by creating the two new configurations "bundle" and "bundleApi" which replace the previously used "releaseDep". Dependencies of the "bundle"-configuration gets extend from the "implementation"-configuration (by the gradle-java-plugin) which is not transitive and therefore it's dependencies are only visible to the core package itself while the dependencies in "bundleApi" (which the corresponding "api"-config extends from) can be used by all packages that import the core. Extension in the context of configurations means that that all the dependencies in the "bundle"-config also get added to the "implementation"-config. This solution has the advantage to other solutions (e.g. creating a new OSGI package) that the complexity of the build system is not significantly increased and no versioning of any additional OSGI packages needs to be considered. This commit includes just the preparations for the following commits that actually make this happen. So at this moment the new configs just get extended from the old "releaseDep" for compatibility reasons (this will change in the following commits). The solution was proposed and kindly explained to the authors by @m273d15 following the PR saros-project#1118. Co-authored-by: FKHals <30548867+FKHals@users.noreply.github.com> Co-authored-by: paun42 <75132590+paun42@users.noreply.github.com> Co-authored-by: panphil <74254708+panphil@users.noreply.github.com>
so that the core can act as a dependency-container without leaking it's own (non-transitive) dependencies into the classpaths of the packages importing it while still providing the transitive dependencies that are needed by those packages. This is implemented by creating the two new configurations "bundle" and "bundleApi" which replace the previously used "releaseDep". Dependencies of the "bundle"-configuration gets extend from the "implementation"-configuration (by the gradle-java-plugin) which is not transitive and therefore it's dependencies are only visible to the core package itself while the dependencies in "bundleApi" (which the corresponding "api"-config extends from) can be used by all packages that import the core. Extension in the context of configurations means that that all the dependencies in the "bundle"-config also get added to the "implementation"-config. This solution has the advantage to other solutions (e.g. creating a new OSGI package) that the complexity of the build system is not significantly increased and no versioning of any additional OSGI packages needs to be considered. This commit includes just the preparations for the following commits that actually make this happen. So at this moment the new configs just get extended from the old "releaseDep" for compatibility reasons (this will change in the following commits). The solution was proposed and kindly explained to the authors by @m273d15 following the PR saros-project#1118. Co-authored-by: FKHals <30548867+FKHals@users.noreply.github.com> Co-authored-by: paun42 <75132590+paun42@users.noreply.github.com> Co-authored-by: panphil <74254708+panphil@users.noreply.github.com>
so that the core can act as a dependency-container without leaking it's own (non-transitive) dependencies into the classpaths of the packages importing it while still providing the transitive dependencies that are needed by those packages. This is implemented by creating the two new configurations "bundle" and "bundleApi" which replace the previously used "releaseDep". Dependencies of the "bundle"-configuration gets extend from the "implementation"-configuration (by the gradle-java-plugin) which is not transitive and therefore it's dependencies are only visible to the core package itself while the dependencies in "bundleApi" (which the corresponding "api"-config extends from) can be used by all packages that import the core. Extension in the context of configurations means that that all the dependencies in the "bundle"-config also get added to the "implementation"-config. This solution has the advantage to other solutions (e.g. creating a new OSGI package) that the complexity of the build system is not significantly increased and no versioning of any additional OSGI packages needs to be considered. This commit includes just the preparations for the following commits that actually make this happen. So at this moment the new configs just get extended from the old "releaseDep" for compatibility reasons (this will change in the following commits). The solution was proposed and kindly explained to the authors by @m273d15 following the PR saros-project#1118. Co-authored-by: FKHals <30548867+FKHals@users.noreply.github.com> Co-authored-by: paun42 <75132590+paun42@users.noreply.github.com> Co-authored-by: panphil <74254708+panphil@users.noreply.github.com>
so that the core can act as a dependency-container without leaking it's own (non-transitive) dependencies into the classpaths of the packages importing it while still providing the transitive dependencies that are needed by those packages. This is implemented by creating the two new configurations "bundle" and "bundleApi" which replace the previously used "releaseDep". Dependencies of the "bundle"-configuration gets extend from the "implementation"-configuration (by the gradle-java-plugin) which is not transitive and therefore it's dependencies are only visible to the core package itself while the dependencies in "bundleApi" (which the corresponding "api"-config extends from) can be used by all packages that import the core. Extension in the context of configurations means that that all the dependencies in the "bundle"-config also get added to the "implementation"-config. This solution has the advantage to other solutions (e.g. creating a new OSGI package) that the complexity of the build system is not significantly increased and no versioning of any additional OSGI packages needs to be considered. This commit includes just the preparations for the following commits that actually make this happen. So at this moment the new configs just get extended from the old "releaseDep" for compatibility reasons (this will change in the following commits). The solution was proposed and kindly explained to the authors by @m273d15 following the PR saros-project#1118. Co-authored-by: FKHals <30548867+FKHals@users.noreply.github.com> Co-authored-by: paun42 <75132590+paun42@users.noreply.github.com> Co-authored-by: panphil <74254708+panphil@users.noreply.github.com>
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]:
org.apache.logging.log4j:log4j-api:X.XX.X(external dependency, currently:log4j-1.2-api-2.13.3.jar)org.apache.logging.log4j:log4j-core:X.XX.X(external dep., currently:log4j-api-2.13.3.jar)org.apache.logging.log4j:log4j-1.2-api:X.XX.X(external dep., currently:log4j-core-2.13.3.jar)picocontainer-2.11.2-patched_relocated.jar(local dep.)[core, eclipse]:
weupnp.jar(local dep.)smackx-3.4.1.jar(local dep.)[core, eclipse, stf, sever]:
smack-3.4.1.jar(local dep.)The motivation is also described here.