Skip to content

gh-91962: Fix hstrerror detection issues on Solaris #91963

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 5 commits into from
Oct 30, 2024

Conversation

kulikjak
Copy link
Contributor

@kulikjak kulikjak commented Apr 26, 2022

Configure only checks whether inet_aton needs -lresolv but it also needs to check hstrerror (because on Solaris, inet_aton no longer needs -lresolv, but hstrerror does).

Issue #91962 has more detailed explanation.

encukou
encukou previously approved these changes Feb 12, 2024
Copy link
Member

@encukou encukou left a comment

Choose a reason for hiding this comment

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

LGTM!
Let's merge after alpha 4 is cut, so if something's wrong we don't block the release process.

@erlend-aasland
Copy link
Contributor

I'm not sure if this is the correct fix; please hold on merging it.

@kulikjak
Copy link
Contributor Author

I tested it on Solaris and it seems to work as expected, but I won't pretend that I am an autotools expert and it's perfectly possible that I overlooked something.

@erlend-aasland
Copy link
Contributor

I tested it on Solaris and it seems to work as expected, but I won't pretend that I am an autotools expert and it's perfectly possible that I overlooked something.

There is an existing hstrerror check. If it is incorrect, should we not fix that instead of adding a new check?

@kulikjak kulikjak force-pushed the Solaris-hstrerror-fix branch from 62d527a to f2a9799 Compare February 13, 2024 14:24
@kulikjak
Copy link
Contributor Author

I presume you mean the PY_CHECK_NETDB_FUNC([hstrerror]) line? That one seems to check whether hstrerror is available, not whether it needs additional libraries to link. I guess I can shuffle the code a little and move the -lresolv check near the corresponding PY_CHECK_NETDB_FUNC but that would mean more extensive changes.

That said, there was indeed a slight issue with the detection - it wouldn't work correctly on a system where hstrerror needs -lresolv and inet_aton doesn't exist (although I don't think that is the case anywhere...)

@encukou
Copy link
Member

encukou commented Sep 17, 2024

@erlend-aasland, does this look good?

@kulikjak kulikjak force-pushed the Solaris-hstrerror-fix branch from 040652c to 3cfc710 Compare September 17, 2024 15:52
@kulikjak
Copy link
Contributor Author

I just finished working on an updated version of the check - the old one stopped working at some point, because extensions are no longer using LIBS it seems.

This one keeps AC_CHECK_LIB and just adds another check for hstrerror.

@encukou encukou added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Oct 4, 2024
@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by @encukou for commit 3cfc710 🤖

If you want to schedule another build, you need to add the 🔨 test-with-buildbots label again.

@bedevere-bot bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Oct 4, 2024
@encukou
Copy link
Member

encukou commented Oct 10, 2024

I plan to merge this tomorrow if there are no objections.

Copy link
Contributor

@erlend-aasland erlend-aasland left a comment

Choose a reason for hiding this comment

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

I plan to merge this tomorrow if there are no objections.

With this PR, HAVE_LIBRESOLV will never be defined in pyconfig.h anymore. I don't think that will be a problem; just a heads-up. We don't use HAVE_LIBRESOLV in our code base.

@encukou
Copy link
Member

encukou commented Oct 30, 2024

My gut feeling is that the general idea is OK: HAVE_LIBRESOLV means that we have -lresolv in the flags, so if we drop -lresolv for CPython itself, HAVE_LIBRESOLV should go too.

Alas, if you link socket statically, you get -lresolv but not HAVE_LIBRESOLV. I don't think we can handle that well with current tooling?

@erlend-aasland
Copy link
Contributor

Alas, if you link socket statically, you get -lresolv but not HAVE_LIBRESOLV. I don't think we can handle that well with current tooling?

You can just modify the action-if-true block to explicitly define HAVE_LIBRESOLV (it was implicitly defined before, since the action-if-true block was empty).

@kulikjak
Copy link
Contributor Author

I can do so, but is it really necessary considering that HAVE_LIBRESOLV is currently not used anywhere? It can always be easily added later once it becomes necessary.

@erlend-aasland
Copy link
Contributor

I can do so, but is it really necessary considering that HAVE_LIBRESOLV is currently not used anywhere? It can always be easily added later once it becomes necessary.

It is also exported in Python.h via pyconfig.h, but IMHO it is not needed; I'm fine with the patch as it is (which is also why I approved it).

@encukou encukou merged commit 1064141 into python:main Oct 30, 2024
41 checks passed
@kulikjak
Copy link
Contributor Author

Thank you both very much!

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