-
Notifications
You must be signed in to change notification settings - Fork 321
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Use Maven targets for OSGi compliant artifacts from Maven-Central #2207
base: main
Are you sure you want to change the base?
Conversation
@HannesWell the current plan is to consume these from the new aggregated maven and orbit thing ed is currently working on merks/simrel-maven#3 (comment) So that from our perspective it is transparent if a 3rd party comes from orbit or from maven central regarding the junit5 part I have currentlz this wip branch https://github.com/eclipse/xtext/tree/cd_junit592intarget but here the setup with mixing junit 5 and 4 still makes problems to have it nice in eclipse and maven |
<type>jar</type> | ||
</dependency> | ||
<dependency> | ||
<groupId>org.junit.platform</groupId> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i asume you did not stumbe over the junit5 container problem as you migrated latest only
@HannesWell @cdietrich, I did not follow the discussion about the new p2 repo replacing orbit. If that repo solves the problem of consuming Maven artifacts, I'd stay with that solution, which looks simpler. Or is using Maven artifacts in the target platform providing some benefits? Concerning the other parts:
|
May I ask you why you made that descision?
The suggested change in the setup would probably help even without using Maven Targets to keep the workspace and CI in sync without the need to duplicate/maintain the required IUs and the repo URLs. If you are interested I can do that in a separat PR first. I have not yet looked into or encountered the junit container issue but I can look into it this evening. |
cause in the past i made bad performance experience with the maven feature => using maven orbit works as it worked with old orbit with no extra effort |
Thank you for the initiative Hannes.
I wonder how a change Can you help me understand why it's better to list the maven coordinates in our target file? |
The main advantage of Maven-Targets is that you can consume artifacts from Maven-Central directly. All of that without the need to do any change in any other repo. Even if that would just be a simple version bump like it is for many deps with the service Ed just created, you still have to go to anther repo, create a PR there, wait until it is reviewed, merged and deployed to a P2 repo. For some artifacts you may be lucky and somebody else updates it, but for some you are not. For example Guice is still at the old version although the update was requested a year ago. Another advantage of using maven targets is that you can try out snapshot version of libraries by simply setting the version to that snapshot. Consequently you can for example try it out in a draft PR, make necessary code adjustments and report issues early to the library maintainers.
Maven-Targets are not directly supported by Oomph, that is correct, but indirectly if you include a target-file in a Targlet or set it directly using a
IIRC there was a bug in m2e.pde a while ago that wrapped artifacts were downloaded again on each reload or restart, but that is fixed for a while and I'm not aware of any other performance issue.
The simplification was not regarding the lines of code, which is often a valid measure, but in this case regarding the procedure how to update to a new version or add a completely new artifact. And if we consider #2213 as at least motivated by this change you still have a net save of ~200 lines. :)
Sorry if my remark was missleading, but that statement is not correct. Maven-targets work for every artifact available in an accessible Maven repository. The only 'problem' with generating the OSGi metadata is that it can happen that different projects generate different Metadata for the same Maven artifact and for example contribute the same artifact under different Nevertheless, for the sake of having the same metadata generated it would probably be good to have them generated by a 'central' service like Orbit or Ed's SimRel Orbit. |
1fdf4ab
to
5d8ab5e
Compare
Additionally I just added a license compliance check workflow based on the reusable GitHub
This checks that the license of all dependencies are vetted and if some are not, allows to request a license review by the Eclipse IP team using a simple comment on a PR. This only requires a EclipseFDN Gitlab API token: |
@HannesWell we need a repo to consume in oomph from anyway. what i have understood is
so for bumps i would bump gav version there and can consume it from target platform and oomph what is unclear to me: how to solve the oomph part of the problem |
xtext-latest.target
Outdated
<dependency> | ||
<groupId>org.junit.platform</groupId> | ||
<artifactId>junit-platform-suite-commons</artifactId> | ||
<version>1.9.1</version> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
its not possbile to leverage bom here i assume
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not yet, but there is a proposal to support properties in target-definitions, which could be used to only define the version once: eclipse-pde/eclipse.pde#204
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thinking about it a second time, Maven targets actually also support pom typed artifacts, that work as bom.
So it is probably already working. I'll check it and report back.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, using boms is not yet possible, but it should not be too complicated to support it:
eclipse-m2e/m2e-core#625
But it then will probably only be available in Tycho 4.
5d8ab5e
to
e414950
Compare
That's right or you can just bump the version in this repo target and don't have to go to any other repo, wait for any build to complete or don't have to monitor any other repo. :)
Yes that was the proposal and with #2213, I think we can consider this problem solved. :) |
Another advantage of using maven targets is that you can try out snapshot version of libraries by simply setting the version to that snapshot. |
@cdietrich, @LorenzoBettini, @szarnekow How should we proceed here? |
i propose to consume from eds site for now |
I agree. We can leave this PR open anyway. |
Can you please elaborate, what you think is missing at the moment in order to complete this? |
my main concerns:
|
ad09bb0
to
44ed927
Compare
That's unexpected and I don't have the same experience. Maybe this was a bug in a an old version. Looking at the runtime now they are relatively similar like the mater branch.
In other discussions about this topic, the conclusion was to use simply the latest version, unless there is a specific reason not to use the latest version (e.g. for the If you configure your metadata properly, i.e. specify suitable version range it does not harm if somebody else contributes a more recent version to SimRel. The resolve then tries to minimize the number of artifacts. But in order to be able to minimize the number of artifacts you should not include third-party deps in features because they are always bound to a specific version.
This is already done in this PR and the PGP keyring is already set up in the deploy Jenkins pipelines. So everthing is in place. :) Regarding the not OSGi-ified error-prone dependency, I filed a PR to add OSGi metadata (google/error-prone#3903). |
@HannesWell what, from your perspective, besides turnaround times, speaks against using "eds place" |
It is not per-se wrong to use 'Eds place', so there are not harmful reasons that speak against using it, besides that it is redundant to build another mini-maven-central-mirror (aka. SimRel-Orbit) for artifacts that are already OSGi complaint. The disadvantages of not using Maven-Targets are simply that you don't get the benefits of them. As you said faster turn-around times, you can experiment with pre-release versions (like you did in #2056, I'm glad you like it for experimenting, but why not use it all the time?), you can update dependencies and required code changes (if any) one by one and don't have to do a bulk update because multiple version changed in Ed's place. You often tell that you have difficulties to keep up in Xtext with Eclipse platform's pace for new dependencies. I'm convinced that using Maven-Targets will help you in that regard and will make your life easier. Therefore I created this PR and therefore I helped using Tycho 3 in Xtext. For Guice 6/7 the error-prone dependency is now optional (in Maven and OSGi, google/guice#1739). In case you accept this PR before the upgrade to Guice 6/7 you just have to bump the guice version and no wrapping will be necessary.
Btw. looking at the 2023-03 release repo I see for example three versions of guava, according to the qualifier, two of them come from Orbit. I also want to say again that OSGi already gives us everything we need to let the tools deal with potential multiple version in the SimRel 'reactor', mainly use Import-Package with suitable version-Ranges for third-party dependencies and don't include them in features to not 'lock their version. Additionally this would also have the benefit that this would also lead to higher quality metadata from which also down-stream users would benefit. |
is m2e pde integration still not default in the packages eclipse ships? |
It is in the "Eclipse for Committers": https://www.eclipse.org/downloads/packages/release/2023-03/r/eclipse-ide-eclipse-committers |
see also problems i see in #2671 |
And add license compliance check workflow based on the reusable GitHub workflow provided by eclipse/dash-licenses.
Can we come to a conclusion here? I'm currently working on support for BOM dependencies in Maven-targets and hopefully will complete this before Tycho 4 is released end of this month, so that it could then be used here.
If you decide to use Maven targets of course I can help if any such usage problems arise, although I assume once everything is settled, most of the time not much will change. |
i am still unsure how to deal with this in domain model example or wizard where we have target platforms users maybe set as target plaform and how to communicate it to users. |
Wouldn't it be possible to just use the SimRel release repo (at |
Looks like this is already the case: Therefore I think this would not be a problem, since the third-party deps are contributed to SimRel, either via Xtext's p2-repo or indirectly via the Maven-Target mirrors from SimRel-Maven. |
In order to simplify consuming third-party artifacts from Maven-Central this PR aims to use a Maven-type location in the Target-Platform for all artifacts from Maven-Central that are already OSGi compliant by default.
The version of the artifacts is not updated, this is left for a follow up PR.
Furthermore Guice is not yet included because it has non-OSGi-compliant dependencies (see also merks/simrel-maven#4).
The gpg KEYRING is already set up in the deploy workflow
xtext/jenkins/nightly-deploy/Jenkinsfile
Lines 36 to 46 in 7ec6b4f
and therefore only the gpg-sgining for the p2 repo has to be activated.
The only thing that is currently missing in this PR is the inclusion of the maven-target IUs in the Oomph-setup respectively in the workspace target location set with it.
At the moment my idea is to move all locations shared between all selectable target-platforms into a
xtext-shared.target
, which is included in all other xtext-targets as nested target and included into the Oomph targlet as composed-targets.Additionally the Targlet-Task in the setup could be changed to conditionally include the target-file of the target-platform selected in the workspace (see https://wiki.eclipse.org/Eclipse_Oomph_Authoring#Conditional_Tasks). Actually a task for each target-file would be created, but filtered for the corresponding value of the
eclipse.target.platform
variable.This would allow to further reduce the number of URLs defined in the setup (eventually maybe even to zero) and only specify the repo-locations once in a target-file.
This would avoid the need to declare many URLs multiple times and prevents divergence of definitions (I had the impression that in the target-files sometimes a different repository URL is used than in the Oomph-Targlet for the same project).
@cdietrich, @LorenzoBettini @szarnekow what do you think?