search

ibmruntimes / openj9-openjdk-jdk11

Extensions for OpenJDK 11 for Eclipse OpenJ9
GNU General Public License v2.0
32 stars 111 forks source link

Ensure jars are processed in classpath order #787

Closed srutjay closed 2 months ago

srutjay commented 3 months ago

This pull request updates the code to ensure that jars are processed in the exact order they appear in the classpath. By maintaining this sequence, we can prevent potential conflicts and ensure that dependencies are loaded correctly. This change addresses issues where jar order was previously inconsistent, leading to unexpected behavior.

This is a backport of : -https://github.com/openjdk/jdk11u-dev/pull/2749

srutjay commented 3 months ago

@keithc-ca/ @joransiu Can you please review this PR

srutjay commented 2 months ago

@keithc-ca I have addressed the review comments. Please review

srutjay commented 2 months ago

Although the OpenJDK PR has been merged, the fix will only be available in version 11.0.25, scheduled for release in October. Since the customer is seeking an earlier release, this PR needs to be merged sooner.

keithc-ca commented 2 months ago

The commit summary line should describe what this change will do if merged, not what the problem is. This was originally offered as a fix to avoid stack overflow, but the comments upstream suggest this is more about the order jars are considered. Suggest:

Consider jars in the order they appear in the classpath

This is a backport of: https://github.com/openjdk/jdk11u-dev/pull/2749.

Signed-off-by: Sruthy Jayan <srutjay1@in.ibm.com>
srutjay commented 2 months ago

The commit summary line should describe what this change will do if merged, not what the problem is. This was originally offered as a fix to avoid stack overflow, but the comments upstream suggest this is more about the order jars are considered. Suggest:

Consider jars in the order they appear in the classpath

This is a backport of: https://github.com/openjdk/jdk11u-dev/pull/2749.

Signed-off-by: Sruthy Jayan <srutjay1@in.ibm.com>

@keithc-ca I have addressed this.

keithc-ca commented 2 months ago

The commit summary ...

@keithc-ca I have addressed this.

Sorry, I should have also asked you to update the title of this pull request and update the description to more accurately reflect what this change is about.

pshipton commented 2 months ago

Although the OpenJDK PR has been merged

This seems overly complicated for something already released in the openjdk jdk11 head stream. Ideally we would just cherry pick the openjdk change to create the PR.

keithc-ca commented 2 months ago

The back-port to jdk11u was not as close to the change in the head stream as I would have liked. If we were to just cherry-pick the change from the head stream, we'll have a merge conflict when the change in jdk11u flows downstream to here.

My latest ask is just that the description here be consistent with the commit message which should be consistent with the change and its motivation.

pshipton commented 2 months ago

I don't understand, the change in this PR looks identical to the OpenJDK change. By "openjdk head stream" I meant the jdk11 head stream where https://github.com/openjdk/jdk11u-dev/pull/2749 is merged. We should be able to just cherry pick this change.

keithc-ca commented 2 months ago

The change is in https://github.com/openjdk/jdk11u-dev, but yet not in https://github.com/openjdk/jdk11u where content for this repository originates, making cherry-picking more challenging.

pshipton commented 2 months ago

Doesn't seem like a problem to add another remote.

keithc-ca commented 2 months ago

As far as I know, we don't have a repository based on jdk11u-dev that's been filtered (via filter-repo) like the master branch here, and I wouldn't expect cherry-picking to work from jdk11u-dev directly.

srutjay commented 2 months ago

@keithc-ca Updated the title and description

keithc-ca commented 2 months ago

Jenkins test sanity amac jdk11

keithc-ca commented 2 months ago

Grinder for jdk_lang passed: https://openj9-jenkins.osuosl.org/job/Grinder/3689.