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

Build the tests with release=8 whenever it is possible #489

Merged
merged 1 commit into from
Nov 18, 2024

Conversation

fridrich
Copy link
Contributor

Fixes eclipse-openj9/openj9#19888

The tests have to be built with release=8 in order to avoid calling the new overridden methods with covariant return types in java.nio.ByteBuffer/CharBuffer/LongBuffer, like position(int), rewind, flip...
The only source file that has to be built with source=1.8 and target=1.8 is the gnu/testlet/java/lang/Class/classInfo/getDeclaredMethod.java, because it requires sun.reflect.annotation.AnnotationType that is not visible using release=8 from jdk9+
The specifying of release="8", source="1.8" and target="1.8" allows ant 1.10.x to use either release if supported or the source/target if release is not supported.

@fridrich fridrich force-pushed the master branch 2 times, most recently from eeca573 to 99327e9 Compare September 23, 2024 18:46
@karianna
Copy link
Contributor

@fridrich Are you able to sign the Eclipse CLA?

@llxia
Copy link
Contributor

llxia commented Sep 24, 2024

Thanks @fridrich for providing the fix. Could you try to sign ECA? https://github.com/adoptium/aqa-tests/blob/master/Contributing.md#eclipse-contributor-agreement

The process should be simple, but unfortunately, we cannot proceed with the change without a signed ECA.

@llxia
Copy link
Contributor

llxia commented Sep 24, 2024

@annaibm Please help to test this PR. Thanks

@pshipton
Copy link
Contributor

FYI the comment which I'll copy here.

fridrich commented Sep 23, 2024

So, that pull request basically fixes the build of the mauve.jar, so that it works with the java 8. It is hanging in eclipsefdn/eca. I am not sure whether we (SUSE) have ever signed something like this. I also don't care to be credited, so anybody who has the right paperwork on file can submit that patch. I transfer automatically the copyright to anybody brave enough to get it integrated.

@fridrich
Copy link
Contributor Author

At the end, I signed it in my personal capacity. So it should be good I think.

@karianna karianna requested a review from llxia September 25, 2024 10:30
@llxia
Copy link
Contributor

llxia commented Oct 3, 2024

Quick update: We encountered other issues while upgrading to JDK 17 for compilation for all dependent jars (though mauve.jar is fine). This PR can't be merged until those issues are resolved.

Copy link
Contributor

@smlambert smlambert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given the comment that it can't be merged, putting a request changes 'hold' on this one so that it is not accidentally merged ahead of the other issues are resolved (more details on the other issues referenced would be good to link to this @llxia )

@karianna karianna marked this pull request as draft October 3, 2024 19:47
Copy link
Contributor

@llxia llxia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@llxia
Copy link
Contributor

llxia commented Nov 18, 2024

@smlambert we are ready to merge this PR.

@llxia llxia marked this pull request as ready for review November 18, 2024 18:35
@llxia llxia merged commit 901f940 into adoptium:master Nov 18, 2024
1 check passed
llxia added a commit that referenced this pull request Nov 22, 2024
LongyuZhang pushed a commit that referenced this pull request Nov 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants