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

Backport c++ compatible mangling support to 22.3 #445

Merged
merged 7 commits into from
Oct 18, 2022

Conversation

zakkak
Copy link
Collaborator

@zakkak zakkak commented Oct 18, 2022

Backports: oracle#4950
Closes: #432

@zakkak zakkak added enhancement New feature or request backport labels Oct 18, 2022
@zakkak zakkak added this to the 22.3.0.1-Final milestone Oct 18, 2022
@zakkak zakkak requested review from jerboaa and adinn October 18, 2022 09:10
@oracle-contributor-agreement oracle-contributor-agreement bot added the OCA Verified All contributors have signed the Oracle Contributor Agreement. label Oct 18, 2022
@jerboaa
Copy link
Collaborator

jerboaa commented Oct 18, 2022

The fix being present should also be verifiable by recently merged integration test here: Karm/mandrel-integration-tests#122 That is the existing 22.3 branch should fail prior and pass after.

@jerboaa
Copy link
Collaborator

jerboaa commented Oct 18, 2022

In fact, the 22.3 failure here from the nightly CI is because of this missing backport: https://github.com/graalvm/mandrel/actions/runs/3270318066/jobs/5378993901#step:10:18540

@jerboaa
Copy link
Collaborator

jerboaa commented Oct 18, 2022

This mandrel IT test failure is fixed with #444:

Error:  Failures: 
Error:    AppReproducersTest.imageioAWTTest:429->imageioAWT:546 A different set of static libraries was expected. 
Expected: [libawt.a, libawt_headless.a, libfdlibm.a, libfontmanager.a, libjava.a, libjavajpeg.a, libjvm.a, liblcms.a, liblibchelper.a, libnet.a, libnio.a, libzip.a]
Actual:   [libawt.a, libawt_headless.a, libfdlibm.a, libfontmanager.a, libjava.a, libjavajpeg.a, libjvm.a, liblcms.a, liblibchelper.a, libmanagement_ext.a, libnet.a, libnio.a, libzip.a] ==> expected: <true> but was: <false>
[INFO] 
Error:  Tests run: 18, Failures: 1, Errors: 0, Skipped: 5

@jerboaa jerboaa linked an issue Oct 18, 2022 that may be closed by this pull request
@jerboaa
Copy link
Collaborator

jerboaa commented Oct 18, 2022

There were two failing test. One looking like a network glitch, the second as mentioned in #445 (comment).

Copy link
Collaborator

@jerboaa jerboaa left a comment

Choose a reason for hiding this comment

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

Running mandrel integration tests with this patch and #444 pass for me locally. 2 failures without those patches. LGTM.

Copy link
Collaborator

@adinn adinn left a comment

Choose a reason for hiding this comment

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

This looks good to me. I applied the patch to Mandrel 22.3 spun it up and checked the results and it seems to work ok.

One question before I ack it though. Did the upstream patches apply cleanly or did you make some tweaks. If the latter then what precisely?

@zakkak
Copy link
Collaborator Author

zakkak commented Oct 18, 2022

This looks good to me. I applied the patch to Mandrel 22.3 spun it up and checked the results and it seems to work ok.

One question before I ack it though. Did the upstream patches apply cleanly or did you make some tweaks. If the latter then what precisely?

Thank you for the review @adinn !

The only conflict was in https://github.com/oracle/graal/blob/824967bfa15973848c05e4012798dc245e8f86b5/substratevm/src/com.oracle.svm.hosted/src/com/oracle/svm/hosted/reflect/ReflectionFeature.java#L232-L238

Due to oracle@dc932e5 not being part of 22.3

@adinn
Copy link
Collaborator

adinn commented Oct 18, 2022

@zakkak ^^^

@adinn
Copy link
Collaborator

adinn commented Oct 18, 2022

@zakkak zakkak merged commit b299c10 into graalvm:mandrel/22.3 Oct 18, 2022
@zakkak zakkak deleted the 2022-10-18-backport-4950 branch October 18, 2022 13:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport enhancement New feature or request OCA Verified All contributors have signed the Oracle Contributor Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Consider backport of c++ compatible mangling support for 22.3
3 participants