-
Notifications
You must be signed in to change notification settings - Fork 6.2k
8367913: LIBDL dependency seems to be not needed for some jdk libs #27358
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
base: master
Are you sure you want to change the base?
Conversation
👋 Welcome back mbaesken! A progress list of the required criteria for merging this PR into |
@MBaesken This change now passes all automated pre-integration checks. ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details. After integration, the commit message for the final commit will be:
You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed. At the time when this comment was updated there had been 39 new commits pushed to the
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details. ➡️ To integrate this PR with the above commit message to the |
@MBaesken The following labels will be automatically applied to this pull request:
When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code changes look good but I guess somebody from Oracle has to verify this, whether it does not break unwanted things.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
I have started a verification job with tier1-5. |
Hi Erik, thanks for checking ! |
@@ -162,7 +162,7 @@ $(eval $(call SetupJdkLibrary, BUILD_LIBSYSLOOKUP, \ | |||
LD_SET_ORIGIN := false, \ | |||
LDFLAGS_linux := -Wl$(COMMA)--no-as-needed, \ | |||
LDFLAGS_aix := -brtl -bexpfull, \ | |||
LIBS_linux := $(LIBDL) $(LIBM), \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know libsyslookup is special. It is a dummy library that does not really do anything. I wonder if "pulling in libdl" is one of the things it is there for. Iirc, it is not always used either, so you need to make sure you do testing where it is actually used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know libsyslookup is special. It is a dummy library that does not really do anything. I wonder if "pulling in libdl" is one of the things it is there for. Iirc, it is not always used either, so you need to make sure you do testing where it is actually used.
Is there some test for this special functionality?
The comments in the C sources
https://github.com/openjdk/jdk/blob/master/src/java.base/share/native/libsyslookup/syslookup.c
do not really help me to understand what functions from libdl we want to bring in this way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mcimadamore Can you help us here? libsyslookup has a dependency to libdl, but it is not used. Can we remove that from the linker command line, or would it somehow break libsyslookup? (I have just a very fuzzy idea of the point of this library.) If we try to remove it, what tests do we need to run to make sure we do not break anything?
If that passes, then it is very likely OK. |
I can bring back the LIBDL dependency of BUILD_LIBSYSLOOKUP if we are unsure about this single removal. |
I've pinged Maurizio about libsyslookup; let's hear what he says first. |
Job has completed. There were 2 failures, one had lots of recent history of failing, so seems unrelated. The other was a crash that seemed unusual, but it passed on a rerun. So I think we can say this passed. |
Thanks for the testing ! So let's wait for some feedback on libsyslookup . |
A couple of JDK native libs link to $(LIBDL) , but some do not use symbols from it.
Progress
Issue
Reviewers
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/27358/head:pull/27358
$ git checkout pull/27358
Update a local copy of the PR:
$ git checkout pull/27358
$ git pull https://git.openjdk.org/jdk.git pull/27358/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 27358
View PR using the GUI difftool:
$ git pr show -t 27358
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/27358.diff
Using Webrev
Link to Webrev Comment