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

Update Eclipse Tycho and replace EBR dependency with Orbit #1501

Open
nikhilnanivadekar opened this issue Aug 28, 2023 · 23 comments · May be fixed by #1569
Open

Update Eclipse Tycho and replace EBR dependency with Orbit #1501

nikhilnanivadekar opened this issue Aug 28, 2023 · 23 comments · May be fixed by #1569

Comments

@nikhilnanivadekar
Copy link
Contributor

Eclipse Tycho's latest version is: 4.0.2 : https://projects.eclipse.org/projects/technology.tycho

The EBR plug-in has been merged with Eclipse Orbit (https://github.com/eclipse-orbit/ebr). Hence, we need to replace EBR with Orbit.

@nikhilnanivadekar
Copy link
Contributor Author

@guw @ujhelyiz Hi Gunnar, Zoltan, is the update to Eclipse Tycho as simple update of the plugins or it is more intrusive?
Secondly, since EBR is now sunset, what is the process to remove the dependency and test it out? Happy to read the documentation, if you can please point me to it.

Thanks for the help!

@guw
Copy link
Contributor

guw commented Aug 28, 2023

@nikhilnanivadekar I am not aware of documentation. But there is a lot happening to replace EBR with a more native Tycho approach. See https://github.com/orgs/eclipse-orbit/discussions/49

@nikhilnanivadekar
Copy link
Contributor Author

Thanks Gunnar! So, what would be the best approach to take?

@fipro78
Copy link
Contributor

fipro78 commented Mar 28, 2024

@nikhilnanivadekar
I just stumbled across this issue and would like to add some information.

From my understanding the Eclipse p2 build step is needed because:

  1. In an Eclipse project it was only possible to add p2 update sites to resolve dependencies via a target platform.
  2. The Java SPI design of Eclipse Collections (separation of API and implementation in separate bundles, where the API uses services from the implementation bundle) does not work out of the box in an OSGi context, because of classloader issues.

To make Eclipse Collections usable in an Eclipse project, it is necessary to:

  • squash Eclipse Collections API and implementation into a single bundle
  • provide the result via a p2 update site

This is not necessary anymore.

  1. You can consume artifacts from Maven Central in a Target Definition, so it is technically possible to consume the Eclipse Collections artifacts from there. So the need for a p2 update site is not mandatory anymore.
  2. Using the OSGi ServiceLoader Mediator, it is possible to solve the classloading issue with the SPI design (see [OSGi] Opting in to Service Loader Mediator #1568)

You could therefore discuss:

  • whether you still want to publish an additional p2 update site
  • whether you still want to publish the squashed artifact for easier usage in an OSGi context

It would be possible to reduce the build efforts on your side, if you only publish the two artifacts to Maven Central, and make them work in an OSGi context without additional effort (despite the fact that the SPI Fly bundle needs to be present in the runtime). I mean, you could skip the p2 update site build for future releases.
You could even think of publishing an additional OSGi bundle (the squashed one that is currently created) and publish it also on Maven Central. Which would make it easier to consume in Eclipse. But you will still have to maintain an additional special OSGi artefact.

I don't give a recommendation here, every option has its pros and cons, depending on the consumer.

@Bananeweizen
As an expert in Eclipse build technologies and setups, maybe you can add some opinion and information here?

@fipro78
Copy link
Contributor

fipro78 commented Mar 28, 2024

@merks
What is your opinion on this topic? With the latest modifications on the PR #1569 I now get an error from EBR about an incorrect manifest header. But the header is generated by bnd, so I would assume the header is correct, but there is a bug in EBR. But EBR should not be used anymore if I understood correctly.

Either we are able to fix the issue and keep the status to create a re-bundled Eclipse Collections bundle in a p2 repository, or we remove the p2 build and the re-bundling, once the Service Loader Mediator opt-in is merged. But the later could have impact on existing Eclipse projects that consume Eclipse Collections and want to update to the new version then.

Would like to hear your opinion.

@merks
Copy link

merks commented Mar 28, 2024

Is something currently being published to Maven Central? If so, where exactly?

@fipro78
Copy link
Contributor

fipro78 commented Mar 28, 2024

https://mvnrepository.com/artifact/org.eclipse.collections/eclipse-collections

This is what is currently published to Maven Central. But the problem is that API and Impl are separated, so they do not work out of the box in OSGi/Eclipse. You need to add SPI Fly additionally, and currently you need to add additional system properties to make SPI Fly aware of the Eclipse Collections bundles.

Therefore currently there is a p2 build that uses EBR to re-bundle API and Impl to a single bundle, which is then published as p2 update site. Question is, should Eclipse Collections keep the re-bundling because it is easier to consume in OSGi/Eclipse, or should the Eclipse based projects switch to the Maven artefacts and include SPI Fly. To make that easier, I create PR #1569.

@merks
Copy link

merks commented Mar 28, 2024

Note that we repackage Jetty 11 and 12 as a p2 repository consuming it directly from Maven Central:

https://github.com/eclipse-orbit/orbit-simrel/tree/main/maven-jetty

We could certainly consume the Eclipse Collections artifacts either via this if they are proper OSGi artifacts already

https://github.com/eclipse-orbit/orbit-simrel/tree/main/maven-osgi

Or via this if a BND recipe is needed:

https://github.com/eclipse-orbit/orbit-simrel/tree/main/maven-bnd

@fipro78
Copy link
Contributor

fipro78 commented Mar 28, 2024

Just to get it right:
Is your suggestion to add Eclipse Collections to Orbit? Then we can discuss if we either want to re-bundle them as a single bundle or keep them?

That would remove the need for the project itself to provide a p2 update site. Should be a decision of the project itself, but IMHO a good option to reduce the technical depts in the project itself.

@merks
Copy link

merks commented Mar 28, 2024

As of the most recent Eclipse Platform release, the platform already uses SPI Fly:

https://github.com/eclipse-platform/eclipse.platform.releng.aggregator/blob/7abd3a2f917361376456a95a5f412f0e1d9c120b/eclipse.platform.releng.tychoeclipsebuilder/eclipse.platform.repository/sdk.product#L191

And yes, Orbit can easily incorporate these like any other 3rd party bundle.

The comments about system properties concern me though. What exactly is involved here so that these things "just work"?

The other concern I have is that I/we don't want to contribute the Orbit repository itself to SimRel. That will encourage people to be lazy and not manage their dependencies properly. They will just rely on what's in Orbit and the moment Orbit updates some library version, then aggregation could stop work because someone depends on an older version but isn't contributing that via their p2 repository contents. This could be circumvented by keeping it in a separate repository like I do for Jetty; but that moves some of the workload from the project to Orbit (me), so good for the project, not so good for me. 😨

@merks
Copy link

merks commented Mar 28, 2024

FYI, the only uses in SimRel is this one by VIATRA:

image

@fipro78
Copy link
Contributor

fipro78 commented Mar 28, 2024

What exactly is involved here so that these things "just work"?

The concrete issue is that the API consumes SPI services from Impl. But currently they do not configure the OSGi capabilities to opt-in to the Service Loader Mediator. That means, SPI Fly does not recognize them automatically. So in the current state, you need to add the system properties to tell SPI Fly to process the Eclipse Collections bundles.

With PR #1569 I want to fix this and add the capabilities.

I have one problem right now with this. After I added the capabilities generated by bnd, I get an error from the ebr plugin:

Error: 2:509 [ERROR] Manifest org.eclipse.collections:org.eclipse.collections:eclipse-bundle-recipe:12.0.0-SNAPSHOT : Header contains name field after attribute or directive: ${#attribute} from osgi.service;objectClass:List<String>="${#value}";uses:='${#uses}';${#attribute};effective:=active,osgi.serviceloader;osgi.serviceloader=${#value};register:=${#register};uses:='${#uses}';${#attribute}. Name fields must be consecutive, separated by a ';' like a;b;c;x=3;y=4

The header is generated by bnd, so I suppose it is correct. I therefore assume an issue with EBR when it tries to re-bundle API and Impl. Do you have an idea how I can fix this? Is there an example how the current EBR based p2 build could be updated to maven-bnd? That could at least keep that status quo for the moment.

As of the most recent Eclipse Platform release, the platform already uses SPI Fly

That's nice! So actually Eclipse Collections can then be included in a Target Platform via Maven Central and a p2 update site is not really needed then anymore. Do I get it right?

FYI, the only uses in SimRel is this one by VIATRA

Well, there are several projects that have an implicit dependency. Not in SimRel, but for example NatTable uses Eclipse Collections. So every project that uses NatTable, implicitly consumes Eclipse Collections. Not a SimRel issue, but for other Eclipse projects it could become an issue.

@merks
Copy link

merks commented Mar 28, 2024

I've never familiarized myself with the details of the EBR implementation. Instead I've relied on driving BND wrapping directly from a *.target file as supported by PDE (via m2e extension) and in Tycho, e.g.,

https://github.com/eclipse-orbit/orbit-simrel/blob/132dfb4507312e4e8636cea76d03069231483203/maven-bnd/tp/MavenBND.target#L1172-L1191

There are even cases where the jars being wrapped do nasty java service loader things or other things that rely on flat class loader visibility and for that I use this Equinox-specific trick:

https://github.com/eclipse-orbit/orbit-simrel/blob/132dfb4507312e4e8636cea76d03069231483203/maven-bnd/tp/MavenBND.target#L1187-L1188

@fipro78
Copy link
Contributor

fipro78 commented Apr 2, 2024

As of the most recent Eclipse Platform release, the platform already uses SPI Fly:

https://github.com/eclipse-platform/eclipse.platform.releng.aggregator/blob/7abd3a2f917361376456a95a5f412f0e1d9c120b/eclipse.platform.releng.tychoeclipsebuilder/eclipse.platform.repository/sdk.product#L191

@merks
Just out of curiosity. Why was it chosen to use the org.apache.aries.spifly.dynamic.bundle which needs the auto-start configuration, rather than the org.apache.aries.spifly.dynamic.framework.extension which is a fragment to the system.bundle? Would the fragment not be easier to consume for an RCP application?

@merks
Copy link

merks commented Apr 2, 2024

I think @HannesWell and perhaps @laeubi can best explain the thoughts behind this.

@fipro78
Copy link
Contributor

fipro78 commented Apr 2, 2024

@nikhilnanivadekar
With the PR to add the support for the ServiceLoaderMediator, I want to update to a newer Tycho version and remove the EBR usage. This seems to be necessary, because the EBR plugins do not correctly recognize the generated manifest headers from bnd, and therefore the creation of the p2 repository fails with EBR.

But Tycho >= 3.0 requires Java 17. And we need a quite current 4.x Tycho version to support Maven locations in the target definition. Java 17 is only required for the build execution time. The question for me is:

  • should I update the github actions to use at least Java 17?
  • or should the p2-feature module (the one that builds the p2 repository with Tycho now) be excluded from several builds?

For checkstyle or unit tests, the p2 repository does not need to be build from my understanding. What would be your recommendation?

@HannesWell
Copy link
Member

Why was it chosen to use the org.apache.aries.spifly.dynamic.bundle which needs the auto-start configuration, rather than the org.apache.aries.spifly.dynamic.framework.extension which is a fragment to the system.bundle? Would the fragment not be easier to consume for an RCP application?

I have tried to use the extension once or twice and failed to make it work. I'm not sure if there is an issue with the extension itself or if it was a problem in PDE, but I also didn't try it hard. But in general, handling extensions is also cumbersome with Equinox, at least if they are developed in the same workspace as the product.

In the meantime I had an idea for an implementation that does not require byte-code transformation, which I drafted in eclipse-equinox/equinox#295. But I didn't had the time to complete yet.

Originally I intended it to be part of the equinox core, but it will probably also become an extension (so probably we should take that as an opportunity to enhance the extension handling in PDE as well).

should I update the github actions to use at least Java 17?

Did you know that the setup-java action automatically creates you a toolchains.xml? And if you define their identifiers to match the OSGi EE naming schema, Tycho can pick them automatically to build a plugins code (if useJDK=BREE is set):
https://github.com/eclipse-equinox/equinox/blob/b327cae6d2b4181fb426a91f9fb7196029a88b71/.github/workflows/build.yml#L25-L35

@fipro78
Copy link
Contributor

fipro78 commented Apr 3, 2024

I have tried to use the extension once or twice and failed to make it work. I'm not sure if there is an issue with the extension itself or if it was a problem in PDE, but I also didn't try it hard. But in general, handling extensions is also cumbersome with Equinox, at least if they are developed in the same workspace as the product.

I tested it only with a plain OSGi project based on bnd with Maven setup. There it just worked. Not sure what the issue might be for an Eclipse application, as it has a different startup mechanism.

Did you know that the setup-java action automatically creates you a toolchains.xml? And if you define their identifiers to match the OSGi EE naming schema, Tycho can pick them automatically to build a plugins code (if useJDK=BREE is set): https://github.com/eclipse-equinox/equinox/blob/b327cae6d2b4181fb426a91f9fb7196029a88b71/.github/workflows/build.yml#L25-L35

I wasn't aware of this. Thanks for reminding me of Maven Toolchains. I always forget about it. Actually the bundles/plugins are not build with Tycho. That is plain Maven. I only try to build the feature and p2 update site with Tycho.

@nikhilnanivadekar
Copy link
Contributor Author

@fipro78 to answer your question, please update the p2-feature module (the one that builds the p2 repository with Tycho now) be excluded from several builds?

It shouldn't be there in any builds per say because it would be a no-op on a GitHub action. The only place this is built is Jenkins to push to the update site. If we can get away from the p2 update build and have the library be directly consumed from Maven Central via Simrel, it will be amazing! We are long overdue a release, and removing any build processes is super helpful 😄
Thanks a ton for looking at this!

@laeubi
Copy link
Member

laeubi commented Apr 3, 2024

Sorry for late reply I was ill the last few days.

About Java version:

Neither maven nor Tycho needs toolchains, one can simply use the release option in maven (Tycho will derive it automatically), this is only relevant if you have very strict requirements and don't trust the compiler do the right things or want to test with a "real" JVM, from my experience this mostly is obsolete from Java 9+ on and you can run your build with any suitable JVM

About extensions

@tjwatson might correct me if I'm wrong, but the main issue is that a framework extension must be co-located to the framework bundle what makes issues especially in development scenarios. So assume I have the equinox bundle in my workspace, I need to have the source for all extensions there as well in the same folder what makes it quite brittle.

It also makes it quite inconvenient if one is not copy jars around (what bnd probably does) and try to consume things from different folders. Beside that extensions work but the development of those is quite cumbersome.

About P2 / Updatesites / Maven

If you have a "pure" maven setup otherwise there are several options:

  1. You can simply deploy to maven (central) as Tycho + PDE has good support for consuming items directly from a maven repository (the so called maven target locations) this is also used in eclipse simrel a lot already
  2. You can produce a "maven p2 site", this is what the jetty project does here, this produces a site that is fully deployed to maven and consumable from there
  3. You can produce a "classic" site and deploy it to http server as usual https://github.com/eclipse-tycho/tycho/tree/main/demo/publish-p2

@fipro78
Copy link
Contributor

fipro78 commented Apr 3, 2024

@nikhilnanivadekar
That makes it easy to solve the build issues. :)

I removed p2-feature and p2-repository from the modules, so the p2 update site is not built anymore as part of the main build. So it does not interfere with the build of the JARs and the GitHub actions.

p2-repository is now actually outdated and should not be used anymore. You could even think about deleting the sources of that module in the future. p2-repository uses EBR, which seems to be not usable anymore with the ServiceLoader Mediator changes.

p2-feature uses Tycho only, consumes the artifacts from Maven Central and builds up an Update Site with the API and Impl artifacts. It does not create an additional eclipse-collections jar that is wrapping API and Impl in a single jar. This will introduce that consumers in Eclipse need to have a ServiceLoader Mediator implementation in the runtime. Which I learned is now already available in current Eclipse versions.
The update site build needs to be executed in Jenkins after the publishing to Maven Central succeeded. It is a separate build not related to the main build. Although it is now possible to consume Eclipse Collections directly from Maven Central (via maven target locations), I suggest to keep the p2 update site for a while, just to make the transition from an update site to Maven Central easier for consumers. But of course that is a project decision. I can tell that there are projects that keep relying on update sites, so giving the opportunity is surely not bad.

Thanks to @merks @HannesWell @laeubi for your feedback. I hope my changes will

  • help the Eclipse Collections project to reduce their build complexity
  • make it possible to consume Eclipse Collections directly in Eclipse/OSGi projects without the need for additional modifications, like additional wrapped jars and/or system properties

I have updated the PR #1569 and also updated the initial comment in that PR to reflect all necessary changes. Please let me know if you have any questions on the PR. My local tests looked fine so far. If you want to merge the PR and publish a milestone release with those changes, I can try to test different setups to consume Eclipse Collections in an plain OSGi application and an Eclipse Application. Which are the main targets for the changes.

@merks
Copy link

merks commented Apr 3, 2024

Is there somewhere we can see the maven artifacts, in the form of a maven repository, before they are finally published to maven central?

@nikhilnanivadekar
Copy link
Contributor Author

@merks there are few options to see the artifacts:

  • In the local run maven command similar to Jenkins release build something like this -pl '!jcstress-tests,!junit-trait-runner,!serialization-tests,!test-coverage,!unit-tests-java8,!unit-tests,!p2-repository,!p2-repository/org.eclipse.collections' -DperformRelease=true -Dsurefire.useFile=false clean install javadoc:jar

  • Run the Jenkins deploy build and push them to Sonatype. We can drop the artifacts if they are useless and choose to skip pushing them to maven central.

In general we have ensured that we can replicate in local environments.

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 a pull request may close this issue.

6 participants