Skip to content

Fix compile error and add iconv_init() call #20070

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

Merged
merged 2 commits into from
Sep 5, 2024

Conversation

hangshao0
Copy link
Contributor

@hangshao0 hangshao0 commented Aug 28, 2024

  1. Revert #Backup and restore orignal LIBPATH on z/OS Java 21+ #20014.
  2. Fix compile error that origLibpath is not defined on z/OS Java 21.
  3. Call iconv_init() in JNI_CreateJavaVM_impl() before the first use of
    getenv() on z/OS.

Internal issue

@hangshao0 hangshao0 marked this pull request as draft August 28, 2024 14:21
@hangshao0 hangshao0 marked this pull request as ready for review August 28, 2024 15:27
@hangshao0 hangshao0 requested a review from keithc-ca August 28, 2024 15:28
@keithc-ca keithc-ca self-assigned this Aug 28, 2024
@keithc-ca
Copy link
Contributor

Why should we not also revert #20007? That would avoid the compile error relating to origLibpath and eliminate the need for the new call to iconv_init().

@hangshao0
Copy link
Contributor Author

Why should we not also revert #20007?

I think the change in #20007 is still needed (append /lib to LIBPATH) for the cases that are not called from launcher.

@keithc-ca
Copy link
Contributor

For situations where the JVM is not started from a standard launcher, I think adding to LIBPATH as the JVM is started is either irrelevant or too late.

@hangshao0
Copy link
Contributor Author

hangshao0 commented Aug 28, 2024

For situations where the JVM is not started from a standard launcher, I think adding to LIBPATH as the JVM is started is either irrelevant or too late.

That means users have to take care of the LIBPATH in these situations ?

@keithc-ca
Copy link
Contributor

Yes, but hopefully, "users" in this case means the authors of non-standard launcher code, not end users of an application.

@hangshao0
Copy link
Contributor Author

hangshao0 commented Aug 28, 2024

Yes, but hopefully, "users" in this case means the authors of non-standard launcher code, not end users of an application.

FYI @joransiu
I remembered @joransiu mentioned there are some users on z/OS starting up the JVM via JNI_CreateJavaVM().
If we decide to revert #20007, we may need to notice them that they need to take care of LIBPATH.

@keithc-ca
Copy link
Contributor

Yes, but if those users need shared libraries before starting the JVM, they can't expect the JVM to help (because by definition it hasn't started yet).

@hangshao0
Copy link
Contributor Author

but if those users need shared libraries before starting the JVM, they can't expect the JVM to help (because by definition it hasn't started yet).

Yes, I agree. They could see issue caused by LIBPATH before calling into our code. We have no control over that.

I should mention in my above comment I am fine reverting #20007.

@joransiu
Copy link
Member

FYI @joransiu I remembered @joransiu mentioned there are some users on z/OS starting up the JVM via JNI_CreateJavaVM(). If we decide to revert #20007, we may need to notice them that they need to take care of LIBPATH.

Yeah, we hit the problem when using Liberty's launcher, which had not updated LIBPATH with /lib. I think having the JVM ensure /lib is appended if necessary makes sense for such custom launchers.

@pshipton
Copy link
Member

The question is, does it work? We've seen before that things such as LIBPATH need to be set before the process is started. Setting it in the process after it's started doesn't take effect. The java launcher sets it and then forks the JVM process.

@hangshao0
Copy link
Contributor Author

If we have the requirement of /lib being in the LIBPATH on z/OS Java 21, it should be documented.

@keithc-ca
Copy link
Contributor

If we have the requirement of /lib being in the LIBPATH on z/OS Java 21, it should be documented.

I don't think it's a requirement for z/OS: the need for the library is a feature of the launcher code on z/OS. Other JNI applications may not need it.

@hangshao0 hangshao0 changed the title Fix compile error and add iconv_init() call Revert #20007 and #20014 Aug 29, 2024
@hangshao0
Copy link
Contributor Author

Changed this to revert ##20014 and ##20007

@joransiu
Copy link
Member

I don't think it's a requirement for z/OS: the need for the library is a feature of the launcher code on z/OS. Other JNI applications may not need it.

An application using java/util/zip/GZip APIs will invoke classlib implementation that calls into system zlib library. How would /lib be handled in that case? I think the JLI launcher issue we handled recently was case where the launcher itself had a dependency on system zlib.

@hangshao0
Copy link
Contributor Author

@keithc-ca Any comments on #20070 (comment) ?

@joransiu
Copy link
Member

joransiu commented Sep 3, 2024

We just tested Liberty launcher, and can confirm that we are able to bring up the app server properly without /lib in the LIBPATH now. The changes from #20007 are needed.

@keithc-ca
Copy link
Contributor

The changes from #20007 are needed.

After an offline discussion with @joransiu, I agree we don't want to revert #20007.

This reverts commit b5844da.

Change-Id: Ie4ae9cedbb3d420e784aaaa275487626b2fde214
@hangshao0 hangshao0 changed the title Revert #20007 and #20014 Fix compile error and add iconv_init() call Sep 3, 2024
@hangshao0
Copy link
Contributor Author

After an offline discussion with @joransiu, I agree we don't want to revert #20007.

I've changed this PR back to the original form: revert #20014 + fix compile error and add a iconv_init() call.

@hangshao0 hangshao0 force-pushed the master branch 2 times, most recently from 57a0973 to 5e295cc Compare September 4, 2024 18:50
1. Fix compile error that origLibpath is not defined on z/OS Java 21+.

2. Call iconv_init() in JNI_CreateJavaVM_impl() before the first use of
getenv() on z/OS java 21+.

Signed-off-by: Hang Shao <[email protected]>
@keithc-ca
Copy link
Contributor

Jenkins test sanity aix,zlinux jdk11,jdk21

@keithc-ca keithc-ca merged commit 66909d5 into eclipse-openj9:master Sep 5, 2024
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants