Closed nikhilnanivadekar closed 2 months ago
nikhilnanivadekar
commented
1 year ago @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
commented
1 year ago @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
commented
1 year ago Thanks Gunnar! So, what would be the best approach to take?
fipro78
commented
5 months ago @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:
To make Eclipse Collections usable in an Eclipse project, it is necessary to:
This is not necessary anymore.
You could therefore discuss:
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
commented
5 months ago @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
commented
5 months ago Is something currently being published to Maven Central? If so, where exactly?
fipro78
commented
5 months ago 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
commented
5 months ago 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
commented
5 months ago 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
commented
5 months ago As of the most recent Eclipse Platform release, the platform already uses SPI Fly:
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
commented
5 months ago FYI, the only uses in SimRel is this one by VIATRA:
fipro78
commented
5 months ago 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=4The 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
commented
5 months ago 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.,
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:
fipro78
commented
4 months ago As of the most recent Eclipse Platform release, the platform already uses SPI Fly:
@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
commented
4 months ago I think @HannesWell and perhaps @laeubi can best explain the thoughts behind this.
fipro78
commented
4 months ago @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:
For checkstyle or unit tests, the p2 repository does not need to be build from my understanding. What would be your recommendation?
HannesWell
commented
4 months ago Why was it chosen to use the
org.apache.aries.spifly.dynamic.bundlewhich needs the auto-start configuration, rather than theorg.apache.aries.spifly.dynamic.framework.extensionwhich is a fragment to thesystem.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 https://github.com/eclipse-equinox/equinox/pull/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
commented
4 months ago 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
commented
4 months ago @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
commented
4 months ago Sorry for late reply I was ill the last few days.
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
@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.
If you have a "pure" maven setup otherwise there are several options:
fipro78
commented
4 months ago @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
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
commented
4 months ago 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
commented
4 months ago @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.
merks
commented
2 months ago @fipro78
Please open an issue for https://github.com/eclipse-orbit/orbit-simrel if/when there is a new version published to Maven Central that you would like to be included in an Orbit p2 repository.
fipro78
commented
2 months ago @nikhilnanivadekar With my PR merged, the build of the Eclipse Collections p2 update site is separated from the main build. That means
You can ping me in case you have further questions on this. Or, as @merks mentioned, ping him to include Eclipse Collections from Maven Central to Eclipse Orbit.
From my point of view it is a project decision, whether you want to publish and host a Eclipse Collections p2 repository yourself, or if you want to shift that to Orbit. For consumers it should not make a difference, as it is just another URL.
nikhilnanivadekar
commented
2 months ago @donraab what do you suggest?
On Tue, Jun 11, 2024 at 12:23 AM Dirk Fauth @.***> wrote:
@nikhilnanivadekar https://github.com/nikhilnanivadekar With my PR merged, the build of the Eclipse Collections p2 update site is separated from the main build. That means
- first build and publish the Eclipse Collections release to Maven Central
- Update the p2 build to consume the new libraries from Maven Central
- Build and publish the p2 update site
You can ping me in case you have further questions on this. Or, as @merks https://github.com/merks mentioned, ping him to include Eclipse Collections from Maven Central to Eclipse Orbit.
From my point of view it is a project decision, whether you want to publish and host a Eclipse Collections p2 repository yourself, or if you want to shift that to Orbit. For consumers it should not make a difference, as it is just another URL.
— Reply to this email directly, view it on GitHub https://github.com/eclipse/eclipse-collections/issues/1501#issuecomment-2159982817, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACLY4XPZG44WWUX7U7SNMCTZG2QXJAVCNFSM6AAAAAA4BLOBZKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCNJZHE4DEOBRG4 . You are receiving this because you were mentioned.Message ID: @.***>
donraab
commented
2 months ago @donraab what do you suggest?
I suggest the simplest thing that could possibly work. The less complexity we can have in our build the better.
Based on the issue you created here, which was auto closed when I merged @fipro78 PR, along with the lengthy discussion thread, it seems to make sense to me for Eclipse Collections to move to Orbit.
@fipro78 @merks Do we need to create a new orbit-simrel issue with each Eclipse Collections Maven Central release? Would it make sense to test the inclusion of a milestone release of EC in Maven Central / Orbit before the final release of 12.0?
merks
commented
2 months ago No there is no need to announce updates because the process for discovering updates is effectively automatic. The analyzers determine the contents of the various *.target files, look at Maven Central to determine if there is a newer version of that dependency, and then updates the necessary Orbit artifacts to incorporate that new version as an upgrade to the old version, e.g., here is a commit, all the contents of which were generated, which was created when a new version of org.apache.felix.scr was discovered:
https://github.com/eclipse-orbit/orbit-simrel/commit/3c14f01724999f7c4c1f4604eb86ca6196a462f8
To add a Maven artifact available as an OSGi bundle from Maven Central to Orbit would just require adding the coordinates here:
https://github.com/eclipse-orbit/orbit-simrel/blob/main/maven-osgi/tp/other/MavenSupplement.target
There's often a question of how to deal with major version increases. For some projects they regularly produce new Major versions
For other cases we can maintain multiple different major versions, though generally we want to avoid that because with that approach more and more "old crap" accumulates over time.
The generated report does let us know when there are major new versions available:
https://github.com/eclipse-orbit/orbit-simrel/blob/main/report/maven-osgi/merged-target/REPORT.md
So mostly the effort here is the one time effort of adding the coordinates, which of course is literally just a few minutes of work.
Eclipse Tycho's latest version is:
4.0.2: https://projects.eclipse.org/projects/technology.tychoThe EBR plug-in has been merged with Eclipse Orbit (https://github.com/eclipse-orbit/ebr). Hence, we need to replace EBR with Orbit.