-
Notifications
You must be signed in to change notification settings - Fork 16
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
Replace content of o.e.osgi.util by osgi-bundles from Maven-Central #41
Replace content of o.e.osgi.util by osgi-bundles from Maven-Central #41
Conversation
This requires eclipse-platform/eclipse.platform.releng.aggregator#223 to be submitted first. |
That has been submitted, but I guess we need an I-Build that includes that before we can get a good verification build here? |
@akurtakov will features need to be updated to include these bundles or all will be pulled in "automatically" from the require-bundle requirements? |
Should be picked automatically. I've retriggered the verification build as it should work now that target platform is updated and published. |
They won't 'automatically' be included in the features that currently include org.eclipse.osgi.util though will they. I.e., I assume those would need to be updated manually... |
They don't have to be manually added in the features at all. Features include org.eclipse.osgi.util which requires and reexports these bundles. P2 will handle that perfectly fine without touching the features. |
If the bundles are directly listed as content in the feature.xml files, those feature.xml files needs to be rebuilt (and touch prior to that) to reflect the version change as the version is resolved and set at built-time. |
This is a decision that is not without impact. It's always been true that when a bundle requires another bundle that the other bundle will be installed regardless of whether it is included in a feature or not. But including them in a feature ensures that they will be installed as a cohesive set with specific versions; one's that have been tested in combination. In any case, it seems there has been an unstated decision to "include less" which is why I'm asking. Also, I think that means we can expect more cases like this to come up: I have explicitly asked on that issue, "what to do," but got no response. It would be good to respond there rather than leave the issue dangling and leaving me wondering... |
In eclipse-equinox/p2#35 , the fact that 4.23 misses transitive dependencies for feature-based launch is indeed a bug, which has become visible with sat4j being "unincluded", and am glad it's fixed in 4.24. But it's indeed not black or white; there are some advantages and drawbacks to both inclusion and reference. My personal impression is that the advantages are more numerous in the "not include" camp, but maybe there are some important stories I'm missing to balance my opinion a bit. I'm all ears ;) |
@HannesWell I think the application admin tests are failing because the tests suite is not picking up the new bundles in the minimal bundle set in org.eclipse.osgi.tests.appadmin.ApplicationAdminTest.suite() |
Try adding this:
This may impact other session tests in other eclipse project repositories. |
Those osgi-bundles will (or at least should) be automatically included into the Eclipse-Platform/SDK p2-repo because the tycho-p2-repository plugin is set up to include all dependencies into the repo: Since 4.23 feature-based launches from the IDE also consider included and required features+plugin. With 4.24 feature based launches are used by default for launches created from the product editor and you can opt-out to include all requirements (respectively opt-in to include them for plug-in based launches) as I have described here: So from a availability/launching point of view the only problem I can see is the one described initially if downstream consumer create a p2-repo from that feature and does not include dependencies.
Nevertheless this is a valid point that should be considered when discussing if we should require more and include less. One more point I would like to be discussed if developers should be able to use version of the Eclipse-SDK in their TP that is newer than the Eclipse installation they are running? The reason I'm asking is that some of the features containing |
9b0b4c9
to
90fee62
Compare
Together with requiring bundle |
The tests are successful now. The build failure is due to a missing version bump in the launchers. Probably due to #42. I'm about the prepare a fix. |
90fee62
to
8ed1f96
Compare
I think we should not submit this until we have decided on how to handle the inclusion/requirement of the new osgi-bundle. |
@merks Sorry for missing your question (was on vacation) but even now I don't see your question there. Would you please ask it again there? The whole discussion looks complete to me now. P.S. I'm really pleased by the overwhelming amount of discussions going on lately on many topics. It's hard (maybe impossible?) to keep track but it somehow feels like community reborn to me. |
Indeed there is a lot of discussion! A little overwhelming sometime so easy to miss details, but a good thing for sure. I think launch problems were caused by the removal of sat4j includes so I asked about that: eclipse-equinox/p2#35 (comment) Then I suggested that removing it was maybe not such a good idea: eclipse-equinox/p2#35 (comment) But the issue is still open. So either we say, too bad, this won't be such a problem with newer versions of PDE and close the issue, or we decide it wasn't such a good idea and put the include back. But we haven't done either. As Mickael correctly points out, there are probably/maybe other features that have become inconsistent over the years. Nevertheless, we should make such decisions consciously and conscientiously, which is why I noticed a recurrence of this same theme here and ask myself "are we making a decision and what is guiding that decision?" Consider this particular inclusion: I'm pretty sure that unless the *.source bundle is included in a feature (or a source feature is created from a feature that includes the bundle), the source bundle will not end up in the repository, because nothing requires it. But I'm sure people will want and expect the source bundles for these new OSGi bundles to be available in the SDK. So if there are to be source bundles available, then I think it follows that all bundles for which there should be source bundles must be included in at least one feature. Right? |
I think it is quite common that a developer installs Eclipse 4.23 and develops against 4.24 in the target platform; I imagine a great many committers do this at least until M1 is out. In fact, for EMF it's super annoying that the GWT tools don't install/work in the latest IDE so I have to use a really old IDE with the last target platform. That being said, it is of course possible that a newer target platform has structure/content that is unrecognizable to an older IDE, though I think that's not actually happened in a long, long time. It's a bit similar to the warning when you open an older workspace with a newer IDE; it might change the content and structure such that the workspace will no longer work with an older IDE. So I wouldn't go as far as saying you should not care whether a new TP works in an older IDE, but rather that you should take it into consideration and if it's not too much unreasonable effort, it would be nice if that worked because people will do it. I think it's great that you changed how feature-based launching works and if that prevents problems like this one caused by missing includes: Then I think it's okay to say, sorry, please use a newer version of the PDE for development... |
Closed with comment now.
Yes, that's valid thing to consider. The right fix IMHO is moving to autogenerated source features so we do less such manual interventions. |
I won't have time to investigate this one before I'm back from PTO (early may), but it could be that if m2e source lookup is installed, m2e is able to retrieve source for such artifacts by downloading them from Maven (as artifacts have a file describing their maven coordinates). So if the IDE is capable of resolving sources without Platform shipping them, it means we don't need to ship them any more and this issue vanishes. |
Thanks! Indeed auto generated source features a great. I love them. Generating source bundles works well in PDE: And Tycho builds them nicely too. The source bundles will end up in the repository if they are included properly somewhere: So there is nothing you need to investigate. I think it would be a shame to not include sources for such central and important things as the OSGi framework in the repository nor in the SDK itself. Many developers use the running IDE as the target platform, while many others use the Eclipse p2 repository as the source of their TP. Telling them they must figure out the maven coordinates of some things and must specify those too (redundantly) in order to resolve a source bundle would be very unhelpful. |
Please, please include sources in the SDK installation itself and in the repository. |
If you consider how things are intended to be used (eg in Eclipse IDE) and focus on the goal (people do get source on Ctrl+Click or when debugging), then whether they're included or not doesn't matter as long as this source becomes available when needed. |
It is. 😅 But its great, as long everybody is constructive and respectful even if his/her wishes/needs are 100% met, but most people are. :)
Yes that is already happening and I agree its bad. For example you won't get the sources of guava when you only have the SimRel Repos in your TP. Only for this purpose I had to add the Orbit repo to our target at work. Mickael's suggestion sounds very good and feasible to work automatically. But it will only work for Maven artifacts not for Eclipse or Orbit bundles whose sources were not included. To ensure or at least increase the probability that all sources are available it is IMHO the most elegant way to let Tycho generate a source feature for each feature and to include the 'main'+source feature into an sdk feature of the project. This way one has way less If we then agree to include the osgi-bundles from Maven into the features the sources will be available automatically.
Thank you for your clarification. That's a reasonable way to move forward. :) But since this happened to the feature-launching in PDE I think the features in Equinox-bundles and the platform can be simplified to not include plug-ins that are already transitively included. |
I haven't followed discussion here closely, I just hear that we assume it's easy to get sources "later somehow". Please consider to ship sources with the bundles, think about people behind firewalls/in other countries without good internet / rollouts without direct internet access (yep, closed secure networks) etc. |
Please ensure the source bundles are in the repository and are installed in the SDK: Look in the SDK as it is today: You will see more than 250 source plugins. Clearly none of these are needed in order for the SDK to function and they are not there for no reason or some random reason. They're there deliberately because real people use them that way and this most certainly applies to all the OSGi bundles that underlie the platform. Please let's not turn this into a discussion about how sources could/might be found later in some just-in-time other way. In my experience, when the source is missing, I will be spending the next hour locating it and attaching it and then needing to do that again and again in my dozens of development environment... |
Nobody should have to do that and I think your and everybody else time is invested better in other tasks. :) Since there is no other way to include sources at the moment you convinced me to include the replacing osgi-bundles into the features that contain the corresponding plug-in from which the sources are about to be removed. I'm already working on reducing the number of occurrences of However on the long run we should try find better ways to ensure that sources are available, mayby by enhancing Tycho to (optionally) add sources for included requirements (if available). Because as it has been said here, including dependencies into features makes their maintenance harder and somehow contradicts the semantics of inclusion. So we should try to combine the good from both worlds. |
8ed1f96
to
76e8845
Compare
All other PR with related changes are created, the SDK-prerequ-TP is updated and I now also have created corresponding PRs to include the now referenced OSGi-API bundles into those features that contain the plug-ins whose OSGi-sources are replaced by that bundles:
Please express your consent or objections at the corresponding issues. With that being said, I think this PR is ready for submission (think this can be merged without the need to explicitly enforce a build-qualifier update in the including builds since they are all build together). Or did I miss any objections? |
76e8845
to
683063a
Compare
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.
Besides the question on use of Require-Bundle
on osgi.tests
this LGTM.
@@ -8,7 +8,8 @@ Require-Bundle: | |||
org.eclipse.core.runtime, | |||
org.eclipse.core.tests.harness, | |||
org.eclipse.test.performance, | |||
org.junit | |||
org.junit, | |||
org.eclipse.osgi.util;bundle-version="3.7.0";resolution:=optional |
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.
Is there an explanation on why this was needed vs using Import-Package
?
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 just did that to pull in the requirements into Tycho-surefire's test-runtime.
But I assume this is better done by specifying extraRequirements in the pom.xml.
Thanks! One last uncertainty on my side since it also came up in the review of the other changes: |
A minor bump is fine with me. Seems more appropriate given the change is significant, even though under the strict definition we do not view this as an API change. |
683063a
to
83f9f8d
Compare
And update to latest version of - org.osgi.util.function - org.osgi.util.promise Discussed in issue: https://github.com/eclipse-equinox/equinox.framework/issues/40
83f9f8d
to
8b2238f
Compare
Great. Thanks to you and everybody else for your reviews and participation! |
We currently include "unrequired" items with "includeAllDependencies" option, I think one could do similar with something like "includeAllSources" if that's desired. |
I think these are related Tycho issues: |
I have specifically take a look at the
Strike that, it seems the platform repo for 4.24 still contains the "eclipse build" ones, the official maven artifacts contain an |
@iloveeclipse @merks can anyone help me in how to reproduce sources are not found for this particular case for |
As discussed in eclipse-equinox/equinox#18, this replaces the content of
org.eclipse.osgi.util
by corresponding osgi-bundles from Maven-Central.As a side effect this updates the versions of
To comply with the Eclipse deprecation/removal process
org.eclipse.osgi.util
is not removed entirely but it's content is replaced by requiring and re-exporting the provided osgi-bundles from Maven-Central. Bundles that require-bundleorg.eclipse.osgi.util
directly continue to work.Should we add a note or something more alarming that the plug-in is now considered deprecated? And where else should we announce it?
What is happening to the features (i.e.
org.eclipse.platform
andorg.eclipse.e4.rcp
) that containo.e.osgi.util
, should they be adjusted to also include the osgi-bundles? Since the tycho-p2-repository plugin is set up to include all dependencies the osgi-bundles will make it into the eclipse-sdk-updates repository. But if a downstream user has one of those features in its repo and has not configured the repository plugin accordingly the osgi-bundles are not included. Are we willing to accept that?