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

Eclipse 4.29 Prerequisites: Orbit #1199

Closed
MohananRahul opened this issue Jul 4, 2023 · 18 comments
Closed

Eclipse 4.29 Prerequisites: Orbit #1199

MohananRahul opened this issue Jul 4, 2023 · 18 comments

Comments

@MohananRahul
Copy link
Contributor

Tracking issue for Orbit. M1 is scheduled for July 7th.

@MohananRahul
Copy link
Contributor Author

@jonahgraham

@jonahgraham
Copy link
Contributor

No new orbit for M1

merks added a commit to merks/eclipse.platform.releng.aggregator that referenced this issue Jul 25, 2023
@merks
Copy link
Contributor

merks commented Jul 25, 2023

Ideally we would use bundle org.hamcrest 2.2:

https://repo1.maven.org/maven2/org/hamcrest/hamcrest/2.2

which is available now but there is some use of org.hamcrest.core that needs to be cleaned up:

image

merks added a commit to merks/eclipse.platform.releng.aggregator that referenced this issue Jul 25, 2023
merks added a commit to merks/eclipse.platform.releng.aggregator that referenced this issue Jul 25, 2023
@HannesWell
Copy link
Member

Ideally we would use bundle org.hamcrest 2.2:

Yes please, that would be great.
We have to do some bnd-instruction magic in our TP to redirect org.hamcrest.core to org.hamcrest.
IIRC also org.junit requires it too, but it is probably from Orbit.
I can check it when back home.

@merks
Copy link
Contributor

merks commented Jul 25, 2023

@HannesWell

I ran into a problem and had to update the BND instructions. JDT and likely many others assume that one can just require bundle org.junit and be done. But Orbit was doing this:

Require-Bundle: \
 org.hamcrest.core;bundle-version="${hamcrest-version-range}";visibility:=reexport

So I added this such that one could still use org.hamcrest 2.2 instead...

Require-Bundle:         org.hamcrest.core;bundle-version="1.3.0";resolution:=optional;visibility:=reexport;x-installation:=greedy

Ideally I'd not need that, but asking everyone downstream to add a package import or a bundle require is probably wishful thinking. And I'm not sure it makes sense to also add this thing for the org.hamcrest bundle...

You can see the second failing build here:

https://ci.eclipse.org/platform/job/eclipse.platform.releng.aggregator/job/PR-1233/

Note I still need to update this PR to use a stable repository URL...

I'll finalize that tomorrow morning...

FYI @jonahgraham

@merks
Copy link
Contributor

merks commented Jul 25, 2023

FYI, see for related issues:

eclipse-orbit/orbit-simrel#4 (comment)

@merks
Copy link
Contributor

merks commented Jul 25, 2023

@HannesWell

Note that this makes me wonder why a consumer of a bundle must duplicate the package imports of that bundle. Or is this a Tycho problem? I haven't tested if PDE gets confused too, but I do recall "the package in directly required" errors in the past that always tempted me to reexport my required bundles...

@HannesWell
Copy link
Member

HannesWell commented Jul 25, 2023

Note that this makes me wonder why a consumer of a bundle must duplicate the package imports of that bundle. Or is this a Tycho problem? I haven't tested if PDE gets confused too, but I do recall "the package in directly required" errors in the past that always tempted me to reexport my required bundles...

I think the root of this problem is that problem is that the class org.junit.Assert has methods that have for example org.hamcrest.Matcher as argument public static <T> void assertThat(T actual, org.hamcrest.Matcher<? super T> matcher);
But these methods are deprecated in favor of the MatcherAssert class of the hamcrest bundle.

But actually when the Assert class is loaded the org.junit bundle class loader should be used and should find hamcrest.Matcher so that no NoClassDefFoundError should be thrown. So as long as one does not use Hamcrest my first guess would be that a test-bundle does not need to import it. And those that use it, it is actually ok to import the corresponding package. This is actually normal if only Import-Package is used: A bundle must list all packages it accesses, regardless of if methods from classes of other bundles return it.
But from my experience this is an area of many surprises, so it should be tested well when something is changed.

Ideally I'd not need that, but asking everyone downstream to add a package import or a bundle require is probably wishful thinking. And I'm not sure it makes sense to also add this thing for the org.hamcrest bundle...

Hamcrest claims that the class API has not changed with version 2: https://github.com/hamcrest/JavaHamcrest/releases/tag/v2.1
Therefore the simplest solution might be that newer versions of the re-packaged org.junit plugin just require and re-export the org.hamcrest 2.2 bundle and then we can wait for the problem to fade out at the day everyone has migrate to JUnit-5 (or whatever will be available then).

Btw. I've written a StackOverflow answer to this topic a while ago: https://stackoverflow.com/a/65611538/14542697

@HannesWell
Copy link
Member

Maybe @vogella can also share his opnion since he has a Hamcrest tutorial and might know how it is used out there in the wild.

@jonahgraham
Copy link
Contributor

@MohananRahul to be clear there is no new "old style EBR" Orbit for M2 so nothing for you to update @merks is doing amazing stuff getting the p2 sites to consume working. As you'll see when the PR from Ed is ready that the new URL will start with https://download.eclipse.org/tools/orbit/simrel/orbit-aggregation instead of https://download.eclipse.org/tools/orbit/downloads/drops

merks added a commit to merks/eclipse.platform.releng.aggregator that referenced this issue Jul 26, 2023
@merks
Copy link
Contributor

merks commented Jul 26, 2023

@HannesWell

I think before I can further change the org.junit bundle that the platform and JDT need to stop relying the org.hamcrest.core bundle. Perhaps package imports and bundle requires need to be added...

merks added a commit to merks/eclipse.platform.releng.aggregator that referenced this issue Jul 26, 2023
merks added a commit to merks/eclipse.platform.releng.aggregator that referenced this issue Jul 26, 2023
@laeubi
Copy link
Contributor

laeubi commented Jul 27, 2023

I haven't tested if PDE gets confused too, but I do recall "the package in directly required" errors in the past that always tempted me to reexport my required bundles...

@merks please don't reexport required bundles, this creates a lot of headache for the resolver as its leading to others using require bundle (and possibly reexporting) and then produces a whole large chain of dependencies unnecessarily.

visibility:=reexport is really only meant as a way to migrate from one bundle to another see for example here:
https://eclipsesource.com/blogs/2008/08/22/tip-split-packages-and-visibility/

The best would be to drop require-bundle at all and replace it by import package (with proper version range!) instead because otherwhise you are exposed to many problems you recently seen noticed here.

I recently added two new features to PDE that make migration/working with package imports more convenient:

  1. Conversion of Require Bundle > Import package in the Manifest cleanup wizard
  2. Automatic manifest generation

If there are still issues / missing features we should solve them, using visibility:=reexport just for convenience is not the solution!

@merks
Copy link
Contributor

merks commented Jul 27, 2023

@laeubi

Unfortunately the reality is:

  • The old version does this and everyone is relying on that so I would need everyone to start doing something different instead of doing what they've always been doing; you know how much people don't like "different".
  • Your comment comes too late because m2 is done and this is already in the Platform's target platform for m2.

So yes, I know it's far, far less than ideal. I did remove it, but that made it not usable in the Platform's build which suggests that unfortunately the ideal and reality are often disjoint. As such, I added it back, but I watered it down to make it optional greedy so that folks can actually use org.hamcrest 2.0.

So in the ideal world, the platform would set the example and the tend by migrating fully to org.hamcrest 2.0 such that we can eventually produce a org.junit without exported bundle requires without the very first project to use it falling flat on its face.

merks added a commit to merks/p2 that referenced this issue Jul 27, 2023
This is to enable migration to use org.hamcrest 2.0, though it's not
clear if this bundle specifically actually need to import anything else.

eclipse-platform/eclipse.platform.releng.aggregator#1199
@laeubi
Copy link
Contributor

laeubi commented Jul 27, 2023

Unfortunately the reality is:

Reality is also if you say "because of this I do that" without any further explanation it:

  1. Could be read as an recommendation for others
  2. It is not clear if it is something you would do in the future

Therefore I think its still good to explain that this is not recommended and should be avoided for new designs.

merks added a commit to eclipse-equinox/p2 that referenced this issue Jul 27, 2023
This is to enable migration to use org.hamcrest 2.0, though it's not
clear if this bundle specifically actually need to import anything else.

eclipse-platform/eclipse.platform.releng.aggregator#1199
@merks
Copy link
Contributor

merks commented Jul 27, 2023

It really, really annoys me that I had to do this. I was so eager to clean this thing up once and for all. It is really a terrible practice. To do that in such a fundamental core bundle that is used in pretty much every project is especially appalling and galling.

Now the Platform needs to set an example such that it no longer requires the org.hamcrest.core bundle anywhere. It will need to start by removing that from the features as already described above; I already fixed p2. Probably it would be good for JDT to lead the charge... Both hamcrest versions are available in the updated target platform:

image

@HannesWell
Copy link
Member

Btw. PDE also has org.hamcrest.core hard-coded for the set up of test-runtimes.
I'll try to provide a PR this evening that (ideally) makes this more flexible, just based on the metadata.

@HannesWell
Copy link
Member

With eclipse-pde/eclipse.pde#684 PDE is updated to not rely on a specific hamcrest bundle name respectively a re-export for its own test-plugins as well as in its PDE Classpath container that adds Junit5 runtime bundles to a projects classpath.

merks added a commit to merks/eclipse.platform.releng.aggregator that referenced this issue Aug 15, 2023
eclipse-platform#1197
eclipse-platform#1199

Use version of com.sun.jna with signed libraries:

eclipse-orbit/orbit-simrel#7

Update net.bytebuddy to 1.14.6
merks added a commit to merks/eclipse.platform.releng.aggregator that referenced this issue Aug 15, 2023
eclipse-platform#1197
eclipse-platform#1199

Use version of com.sun.jna with signed libraries:

eclipse-orbit/orbit-simrel#7

Update net.bytebuddy to 1.14.6
merks added a commit that referenced this issue Aug 15, 2023
#1197
#1199

Use version of com.sun.jna with signed libraries:

eclipse-orbit/orbit-simrel#7

Update net.bytebuddy to 1.14.6
@merks
Copy link
Contributor

merks commented Aug 30, 2023

The the release contribution this effort is done for 4.29.

@merks merks closed this as completed Aug 30, 2023
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

No branches or pull requests

5 participants