Skip to content
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

Merged
merged 1 commit into from
Apr 27, 2022

Conversation

HannesWell
Copy link
Member

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

  • org.osgi.util.function 1.1.0 -> 1.2.0
  • org.osgi.util.promise 1.1.1 -> 1.2.0

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-bundle org.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 and org.eclipse.e4.rcp) that contain o.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?

@HannesWell
Copy link
Member Author

This requires eclipse-platform/eclipse.platform.releng.aggregator#223 to be submitted first.

@tjwatson
Copy link
Contributor

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?

@tjwatson
Copy link
Contributor

@akurtakov will features need to be updated to include these bundles or all will be pulled in "automatically" from the require-bundle requirements?

@akurtakov
Copy link
Member

@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.

@merks
Copy link
Contributor

merks commented Apr 19, 2022

@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.

image

I.e., I assume those would need to be updated manually...

@akurtakov
Copy link
Member

akurtakov commented Apr 19, 2022

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.
We are speaking for new bundles that have never been listed in the features.

@mickaelistria
Copy link
Contributor

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.
If the bundles are softer requirements (require/import in feature.xml, or transitive dependencies), then their is nothing to change as resolution of (newer) version would happen at install-time.

@merks
Copy link
Contributor

merks commented Apr 19, 2022

@akurtakov @mickaelistria

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:

eclipse-equinox/p2#35

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...

@mickaelistria
Copy link
Contributor

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.
Overall, everything that does provision something without computing or validating dependencies is weak, fragile and error-prone. Before Tycho and p2, projects were forced to take great care of their features because otherwise it was hell, and even with great case, it was hell; but since introduction of p2, builds and testing do rely on dependency resolution at provisioning; so many features have stopped being consistent for a long time (I would even argue that feature were meant to resolve a problem that almost doesn't exist anymore). As a result, we see way less "Could not resolve installation" dialogs now than we used to some years ago. Moreover, what a feature describes isn't necessarily what will be run by the application: imagine someone installs 3 features, one including eg sat4j 2.3.5, the other 2.3.6, the other 2.3.7; then the 3 versions are in the OSGi container, OSGi will pick only one if possible, which won't be part of the "cohesive set" that was initially tested and delivered by the 2 first features.
I think the dynamic and modular nature of the runtime needs to be embraced at install-time; and giving more flexibility by "not including" better maps what OSGi is going to do in practice when facing multiple versions of the same bundle.
For the "cohesive set", it would then be the p2 repository; which is what was tested and that includes the tested versions of the dependencies.

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 ;)

@tjwatson
Copy link
Contributor

@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()

@tjwatson
Copy link
Contributor

Try adding this:

		appAdminSessionTest.addBundle("org.osgi.util.function");
		appAdminSessionTest.addBundle("org.osgi.util.measurement");
		appAdminSessionTest.addBundle("org.osgi.util.position");
		appAdminSessionTest.addBundle("org.osgi.util.promise");
		appAdminSessionTest.addBundle("org.osgi.util.xml");

This may impact other session tests in other eclipse project repositories.

@HannesWell
Copy link
Member Author

@akurtakov will features need to be updated to include these bundles or all will be pulled in "automatically" from the require-bundle requirements?

They won't 'automatically' be included in the features that currently include org.eclipse.osgi.util though will they.

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:
eclipse-platform/eclipse.platform.releng.aggregator#211

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:
eclipse-equinox/p2#35 (comment)

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.

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.

Nevertheless this is a valid point that should be considered when discussing if we should require more and include less.
In the end it seems to be a trade of between feature-maintenance effort and possible (but hopefully unlikely) incompatibilities between different versions. In an ideal world where everybody uses proper SemVer (that not only considers API but also behavior) such problems should be fixed if proper version-ranges are set.

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?
I have observed that (most likely) PDE shows a warning dialog to the user and recommends to update the running Eclipse.
Therefore I assumed that old running Eclipse + new Eclipse in TP is nothing I have to care about or is this assumption wrong?

The reason I'm asking is that some of the features containing o.e.osgi.util directly are also contained in each other so it would be sufficient to only include it in the inner most one. P2/Tycho already supports this for a long time only when launching from could then be problematic if one uses a version before 4.23.

@HannesWell
Copy link
Member Author

Try adding this:

		appAdminSessionTest.addBundle("org.osgi.util.function");
		appAdminSessionTest.addBundle("org.osgi.util.measurement");
		appAdminSessionTest.addBundle("org.osgi.util.position");
		appAdminSessionTest.addBundle("org.osgi.util.promise");
		appAdminSessionTest.addBundle("org.osgi.util.xml");

This may impact other session tests in other eclipse project repositories.

Together with requiring bundle org.eclips.osgi.util in the tests plug-in this solved the problem locally. Lets see if this also works on CI.

@HannesWell
Copy link
Member Author

Together with requiring bundle org.eclips.osgi.util in the tests plug-in this solved the problem locally. Lets see if this also works on CI.

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.

@HannesWell
Copy link
Member Author

I think we should not submit this until we have decided on how to handle the inclusion/requirement of the new osgi-bundle.

@akurtakov
Copy link
Member

eclipse-equinox/p2#35

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...

@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.

@merks
Copy link
Contributor

merks commented Apr 20, 2022

@akurtakov

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:

image

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?

@merks
Copy link
Contributor

merks commented Apr 20, 2022

@HannesWell

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?
I have observed that (most likely) PDE shows a warning dialog to the user and recommends to update the running Eclipse.
Therefore I assumed that old running Eclipse + new Eclipse in TP is nothing I have to care about or is this assumption wrong?

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:

eclipse-equinox/p2#35

Then I think it's okay to say, sorry, please use a newer version of the PDE for development...

@akurtakov
Copy link
Member

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.

Closed with comment now.

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:

image

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?

Yes, that's valid thing to consider. The right fix IMHO is moving to autogenerated source features so we do less such manual interventions.

@mickaelistria
Copy link
Contributor

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.

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.

@merks
Copy link
Contributor

merks commented Apr 20, 2022

@akurtakov

Thanks! Indeed auto generated source features a great. I love them.

@mickaelistria

Generating source bundles works well in PDE:

image

And Tycho builds them nicely too. The source bundles will end up in the repository if they are included properly somewhere:

image

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.

@merks
Copy link
Contributor

merks commented Apr 20, 2022

@HannesWell

Please, please include sources in the SDK installation itself and in the repository.

@mickaelistria
Copy link
Contributor

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

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.
So if m2e or some other plugin can provide that (to be verified) without effort on releng side in re-generating and publishing extra bundles, then it seems like the best of both worlds: less releng work, and people access "official" content as provided by the upstream project.

@HannesWell
Copy link
Member Author

Indeed there is a lot of discussion! A little overwhelming sometime so easy to miss details, but a good thing for sure.

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. :)

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.

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 .source elements to handle (and to forget) and the features are less overloaded. Equinox-bundles could benefit from that and I'm planning to provide a PR to apply that.

If we then agree to include the osgi-bundles from Maven into the features the sources will be available automatically.

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:

eclipse-equinox/p2#35

Then I think it's okay to say, sorry, please use a newer version of the PDE for development...

Thank you for your clarification. That's a reasonable way to move forward. :)
But indeed we should wait to eat our own dog-food until it was at least as a milestone or better as GA.

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.

@iloveeclipse
Copy link
Member

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.

@merks
Copy link
Contributor

merks commented Apr 21, 2022

Please ensure the source bundles are in the repository and are installed in the SDK:

Look in the SDK as it is today:

https://download.eclipse.org/eclipse/downloads/drops4/R-4.23-202203080310/download.php?dropFile=eclipse-SDK-4.23-win32-x86_64.zip

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...

@HannesWell
Copy link
Member Author

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 o.e.osgi.util in features where it is not necessary due to transitive inclusion: eclipse-platform/eclipse.platform.releng#28 and eclipse-equinox/equinox.bundles#20 (the latter is hindering the progress at the moment). Including the osg-bundles is then only necessary in as few features possible. The other plug-ins that contain osgi-sources are already only contained in as few features as possible.

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.

@HannesWell
Copy link
Member Author

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?

Copy link
Contributor

@tjwatson tjwatson left a 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
Copy link
Contributor

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?

Copy link
Member Author

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.

@HannesWell
Copy link
Member Author

Besides the question on use of Require-Bundle on osgi.tests this LGTM.

Thanks! One last uncertainty on my side since it also came up in the review of the other changes:
Do you think a minor version bump is the right thing to do here? Since API-tools errors have to be suppressed anyway we can choose what's suitable.
Because the 'effectivly' exposed packages by this bundle do not change actually a micro-version bump could be sufficient from a SemVer point of view.
But since this bundle starts to become obsolete with this change I think a minor version-bump would reflect that better.
What do you think?

@tjwatson
Copy link
Contributor

But since this bundle starts to become obsolete with this change I think a minor version-bump would reflect that better.
What do you think?

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.

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
@HannesWell
Copy link
Member Author

But since this bundle starts to become obsolete with this change I think a minor version-bump would reflect that better.
What do you think?

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.

Great. Thanks to you and everybody else for your reviews and participation!

@laeubi
Copy link
Member

laeubi commented Jul 10, 2022

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.

We currently include "unrequired" items with "includeAllDependencies" option, I think one could do similar with something like "includeAllSources" if that's desired.

@laeubi
Copy link
Member

laeubi commented Jul 10, 2022

@laeubi
Copy link
Member

laeubi commented Jul 11, 2022

I have specifically take a look at the util.function and util.promise and noticed that (in contrast to e.g. osgi.dto) they don't contain an OSGI-OPT folder that carries the sources inside the jar (as in contrast to a separate source bundle).

Regardless of how we solve this in the platform, I think it would be good to have the sources embedded, even if that's optional, maybe with a 1.2.1 version. @bjhargrave what would be the best place to ask for such things?

Because if the would have contained an OSGI-OPT, PDE would have found the sources automatically.

Strike that, it seems the platform repo for 4.24 still contains the "eclipse build" ones, the official maven artifacts contain an OSGI-OPT and thus should be fine!

@laeubi
Copy link
Member

laeubi commented Jul 11, 2022

@iloveeclipse @merks can anyone help me in how to reproduce sources are not found for this particular case for org.osgi.util.function and org.osgi.util.promise? As they carry their sources along in the bundle itself (see http://docs.osgi.org/specification/osgi.core/7.0.0/framework.module.html -> OSGI-OPT directory of the JAR file) I can see the sources of those without a problem without any special need for a separate source bundle.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants