bpo-34412: strsignal(3) does not exist on HP-UX #8786

michael-o · 2018-08-16T15:06:27Z

Introduce a configure check for strsignal(3) and define HAVE_STRSIGNAL for
signalmodule.c. This change applies for Windows and HP-UX.

https://bugs.python.org/issue34412

pppery · 2018-08-16T15:20:51Z

Modules/signalmodule.c

@@ -530,7 +530,7 @@ signal_strsignal_impl(PyObject *module, int signalnum)
        return NULL;
    }

-#ifdef MS_WINDOWS
+#ifndef HAVE_STRSIGNAL
    /* Custom redefinition of POSIX signals allowed on Windows */


This comment is now somewhat inaccurate

Shall I drop it?

I'd keep it and add HP-UX instead.

berkerpeksag

Please add a NEWS entry. See https://devguide.python.org/committing/#what-s-new-and-news-entries for details.
Please regenerate configure.
Does the test_strsignal test in Lib/test/test_signal.py passed on HP-UX when you applied this patch?

berkerpeksag · 2018-08-16T15:24:20Z

configure.ac

@@ -5541,6 +5541,15 @@ AC_MSG_RESULT(python)
 AC_DEFINE(PY_SSL_DEFAULT_CIPHERS, 1)
 ])

+# Windows and HP-UX do not support strsignal(3)
+AC_MSG_CHECKING(for strsignal)
+AC_COMPILE_IFELSE([AC_LANG_PROGRAM([[#include <string.h>]], [[


Can't we add strsignal to the list at https://github.com/python/cpython/blob/7980d771c76d55aca55e1e619bada7c4707305b2/configure.ac#L3433 ?

I have considered this, but it is nowhere documented that it will include string.h. I went for the safe bet.

Many of the functions listed there require a header file to be included and none of them are documented in the autoconf documentation. For example, alarm() requires unistd.h and Modules/signalmodule.c already uses it:

https://github.com/python/cpython/blob/7980d771c76d55aca55e1e619bada7c4707305b2/Modules/signalmodule.c#L344

Does that check works on HP-UX? If it works, I think we can try adding signalstr to that list.

Currently, configure.ac is a mess, so it would nice if we can keep it shorter :)

I will check that too.

Just check on FreeBSD and HP-UX. It does its job in both config.log and config.status for both platforms. Will change accordingly.

bedevere-bot · 2018-08-16T15:29:19Z

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

michael-o · 2018-08-16T15:59:41Z

@berkerpeksag

Will do
Will do, though I do not understand why you are committing a generated file.
Yes, this test passes.

Output:

$ make test TESTOPTS="-v test_signal"
 CC='/opt/aCC/bin/cc' LDSHARED='/opt/aCC/bin/cc -b -L/usr/local/lib/hpux32 -L/usr/local/lib/hpux32' OPT='-O'    _TCLTK_INCLUDES='' _TCLTK_LIBS=''       ./python -E ./setup.py  build
running build
running build_ext
...
Python build finished successfully!
The necessary bits to build these optional modules were not found:
_gdbm                 _lzma                 _tkinter
_uuid                 ossaudiodev           spwd
To find the necessary bits, look in setup.py in detect_modules() for the module's name.


The following modules found by detect_modules() in setup.py, have been
built by the Makefile instead, as configured by the Setup files:
_abc                  atexit                pwd
time


Failed to build these modules:
_curses_panel


Following modules built successfully but were removed because they could not be imported:
_curses

running build_scripts
copying and adjusting /var/osipovmi/cpython/Tools/scripts/pydoc3 -> build/scripts-3.8
copying and adjusting /var/osipovmi/cpython/Tools/scripts/idle3 -> build/scripts-3.8
copying and adjusting /var/osipovmi/cpython/Tools/scripts/2to3 -> build/scripts-3.8
changing mode of build/scripts-3.8/pydoc3 from 640 to 755
changing mode of build/scripts-3.8/idle3 from 640 to 755
changing mode of build/scripts-3.8/2to3 from 640 to 755
renaming build/scripts-3.8/pydoc3 to build/scripts-3.8/pydoc3.8
renaming build/scripts-3.8/idle3 to build/scripts-3.8/idle3.8
renaming build/scripts-3.8/2to3 to build/scripts-3.8/2to3-3.8
         ./python  ./Tools/scripts/run_tests.py -v test_signal
/var/osipovmi/cpython/python -u -W default -bb -E -m test -r -w -j 0 -u all,-largefile,-audio,-gui -v test_signal
== CPython 3.8.0a0 (heads/[bpo-34412](https://www.bugs.python.org/issue34412)-dirty:7980d771c7, Aug 16 2018, 17:43:10) [C]
== HP-UX-B.11.31-ia64-32bit-ELF big-endian
== cwd: /var/osipovmi/cpython/build/test_python_6640
== CPU count: 4
== encodings: locale=utf8, FS=utf-8
Using random seed 9950569
Run tests in parallel using 6 child processes
0:00:25 [1/1/1] test_signal failed
test_enums (test.test_signal.GenericTests) ... ok
test_itimer_exc (test.test_signal.ItimerTest) ... ok
test_itimer_prof (test.test_signal.ItimerTest) ... ok
test_itimer_real (test.test_signal.ItimerTest) ... ok
test_itimer_virtual (test.test_signal.ItimerTest) ... ok
test_setitimer_tiny (test.test_signal.ItimerTest) ... ok
test_pthread_kill (test.test_signal.PendingSignalsTests) ... ok
test_pthread_kill_main_thread (test.test_signal.PendingSignalsTests) ... ok
test_pthread_sigmask (test.test_signal.PendingSignalsTests) ... ok
test_pthread_sigmask_arguments (test.test_signal.PendingSignalsTests) ... ok
test_pthread_sigmask_valid_signals (test.test_signal.PendingSignalsTests) ... ok
test_sigpending (test.test_signal.PendingSignalsTests) ... ok
test_sigpending_empty (test.test_signal.PendingSignalsTests) ... ok
test_sigtimedwait (test.test_signal.PendingSignalsTests) ... ok
test_sigtimedwait_negative_timeout (test.test_signal.PendingSignalsTests) ... ok
test_sigtimedwait_poll (test.test_signal.PendingSignalsTests) ... ok
test_sigtimedwait_timeout (test.test_signal.PendingSignalsTests) ... ok
test_sigwait (test.test_signal.PendingSignalsTests) ... ok
test_sigwait_thread (test.test_signal.PendingSignalsTests) ... ok
test_sigwaitinfo (test.test_signal.PendingSignalsTests) ... ok
test_getsignal (test.test_signal.PosixTests) ... ok
test_interprocess_signal (test.test_signal.PosixTests) ... ok
...

michael-o · 2018-08-16T17:08:52Z

I have made the requested changes; please review again

bedevere-bot · 2018-08-16T17:08:55Z

Thanks for making the requested changes!

@berkerpeksag: please review the changes made to this pull request.

tiran

You have a typo in your test:

======================================================================
ERROR: test_strsignal (test.test_signal.PosixTests)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/travis/build/python/cpython/Lib/test/test_signal.py", line 66, in test_strsignal
    self.assertIn("Hangup", signal.strsignal(singal.SIGHUP))
NameError: name 'singal' is not defined

bedevere-bot · 2018-08-16T17:26:20Z

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

berkerpeksag · 2018-08-16T17:32:26Z

Lib/test/test_signal.py

@@ -60,6 +60,10 @@ def test_getsignal(self):
    def test_strsignal(self):
        self.assertIn("Interrupt", signal.strsignal(signal.SIGINT))
        self.assertIn("Terminated", signal.strsignal(signal.SIGTERM))
+        if sys.platform.startswith("hp-ux"):


I think SIGHUP is a pretty common signal, so it can be added to the list in Modules/signalmodule.c:

#ifndef MS_WINDOWS /* TODO: Add a comment to explain why we need this on HP-UX. */ case SIGHUP: res = "Hangup"; break; #endif

Then we can test self.assertIn("Hangup", signal.strsignal(singal.SIGHUP)) unconditionally.

berkerpeksag · 2018-08-16T17:35:02Z

Misc/NEWS.d/next/Library/2018-08-16-19-07-05.bpo-34412.NF5Jm2.rst

@@ -0,0 +1 @@
+Add HAVE_STRSIGNAL to avoid build failure on HP-UX with strsignal(3)


Can we say something like "make :func:`signal.strsignal` work on HP-UX"?

Please add "Patch by Your Name.".

michael-o · 2018-08-17T07:41:37Z

I have made the requested changes; please review again

bedevere-bot · 2018-08-17T07:41:39Z

Thanks for making the requested changes!

@tiran, @berkerpeksag: please review the changes made to this pull request.

michael-o · 2018-08-17T07:48:27Z

There are test failures on Windows because SIGHUP needs to be declared at several spots. I think this is out of scope for this PR, I will drop that spot.

berkerpeksag · 2018-08-17T08:10:51Z

There are test failures on Windows because SIGHUP needs to be declared at several spots.

Did you cover it with #ifndef MS_WINDOWS ... #endif? I think if you add that #ifndef, it should cover the Windows case since it would only added to the list if HAVE_STRSIGNAL and MS_WINDOWS aren't defined.

michael-o · 2018-08-17T08:23:09Z

@berkerpeksag

So you are looking for this:

diff --git a/Modules/signalmodule.c b/Modules/signalmodule.c
index 056645c124..84095a24c0 100644
--- a/Modules/signalmodule.c
+++ b/Modules/signalmodule.c
@@ -533,6 +533,11 @@ signal_strsignal_impl(PyObject *module, int signalnum)
 #ifndef HAVE_STRSIGNAL
     switch (signalnum) {
         /* Though being a UNIX, HP-UX does not provide strsignal(3) */
+#ifndef MS_WINDOWS
+        case SIGHUP:
+            res = "Hangup";
+            break;
+#endif
         case SIGINT:
             res = "Interrupt";
             break;

This isn't there, I can add this. Shall I?

michael-o · 2018-08-17T08:32:26Z

@berkerpeksag Change pushed.

michael-o · 2018-08-17T09:43:43Z

I have made the requested changes; please review again

bedevere-bot · 2018-08-17T09:43:45Z

Thanks for making the requested changes!

@tiran, @berkerpeksag: please review the changes made to this pull request.

berkerpeksag · 2018-08-19T00:09:53Z

Misc/NEWS.d/next/Library/2018-08-16-19-07-05.bpo-34412.NF5Jm2.rst

@@ -0,0 +1 @@
+make :func:`signal.strsignal` work on HP-UX. Patch by Michael Osipov.


Nit: make -> Make

berkerpeksag · 2018-08-19T00:15:07Z

Modules/signalmodule.c

    switch (signalnum) {
+        /* Though being a UNIX, HP-UX does not provide strsignal(3) */
+#ifndef MS_WINDOWS
+        case SIGHUP:


This list was just a redefinition of POSIX signals supported by Windows. I think we need to check whether there is more than one signal to add to this list for other Unix systems.

Well, I can go through signal.h on HP-UX and provide those, but this is out of scope of this PR because the motivation for this one is just to make this module properly compile on HP-UX. Is that OK for you?

@berkerpeksag I went through signal.h. There is a bus load of signals, some wrapped with ifdefs. I don't think that HP-UX is worth the effort for now just to have a nice string. If so, this has to be a separate PR.

We definitely can't maintain a full list of signals for every platform we support, so I agree with you. I was wondering whether there are other signals to include to make strsignal() useful for most common cases.

I just checked http://nixdoc.net/man-pages/hp-ux/man5/signal.5.html and I think the most common unlisted signals are SIGALRM, SIGPIPE, SIGQUIT, and SIGCHLD.

Since SIGINT and SIGTERM is already in the list, I'd expect SIGQUIT to be there as well.

So, I added all those. checked FreeBSD and glibc implementation + manpages and took the strings (_sys_siglist[]) out of them. They are identical. I am sure whether the tests should be extented because the Open Group says about strsignal(3) that the return value is implementation-defined.

@berkerpeksag WDYT?

Awesome, thanks! No need to update the tests.

I will take another look at this and merge tonight.

bedevere-bot · 2018-08-19T00:15:19Z

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

michael-o · 2018-08-20T07:28:38Z

I have made the requested changes; please review again

bedevere-bot · 2018-08-20T07:28:40Z

Thanks for making the requested changes!

@tiran, @berkerpeksag: please review the changes made to this pull request.

michael-o · 2018-08-22T07:47:49Z

@berkerpeksag are we good to merge?

berkerpeksag · 2018-08-22T16:58:40Z

@michael-o could you rerun autoreconf and commit the changes in pyconfig.h.in? It should look like:

diff --git a/pyconfig.h.in b/pyconfig.h.in
index d80ddc0..a82c373 100644
--- a/pyconfig.h.in
+++ b/pyconfig.h.in
@@ -984,6 +984,9 @@
 /* Define to 1 if you have the <stropts.h> header file. */
 #undef HAVE_STROPTS_H
 
+/* Define to 1 if you have the `strsignal' function. */
+#undef HAVE_STRSIGNAL
+
 /* Define to 1 if `pw_gecos' is a member of `struct passwd'. */
 #undef HAVE_STRUCT_PASSWD_PW_GECOS

Addressed in the latest review. Thanks!

Introduce a configure check for strsignal(3) which defines HAVE_STRSIGNAL for signalmodule.c. This change applies for Windows and HP-UX. Patch by Michael Osipov.

michael-o · 2018-08-23T10:08:18Z

I have made the requested changes; please review again

@berkerpeksag Done. Should be fine now. Have a look please.

bedevere-bot · 2018-08-23T10:08:20Z

Thanks for making the requested changes!

@berkerpeksag: please review the changes made to this pull request.

berkerpeksag · 2018-08-23T13:27:28Z

Thanks!

michael-o · 2018-08-23T14:36:18Z

@berkerpeksag Thanks for merging. Would you mind to take a look at my other HP-UX related PRs? I'd like to continue contributing.

berkerpeksag · 2018-08-23T14:50:45Z

@michael-o I'm looking at #8846 and #8828, but the PRs about setup.py are out of my knowledge.

the-knights-who-say-ni added the CLA signed label Aug 16, 2018

bedevere-bot added the awaiting review label Aug 16, 2018

pppery reviewed Aug 16, 2018

View reviewed changes

berkerpeksag requested changes Aug 16, 2018

View reviewed changes

bedevere-bot added awaiting changes and removed awaiting review labels Aug 16, 2018

michael-o force-pushed the bpo-34412 branch 2 times, most recently from 26627f0 to 20df665 Compare August 16, 2018 17:08

bedevere-bot added awaiting change review and removed awaiting changes labels Aug 16, 2018

tiran previously requested changes Aug 16, 2018

View reviewed changes

bedevere-bot removed the awaiting change review label Aug 16, 2018

bedevere-bot added the awaiting changes label Aug 16, 2018

berkerpeksag requested changes Aug 16, 2018

View reviewed changes

michael-o force-pushed the bpo-34412 branch 2 times, most recently from 391f445 to bdc1a80 Compare August 17, 2018 07:38

bedevere-bot added awaiting change review and removed awaiting changes labels Aug 17, 2018

michael-o force-pushed the bpo-34412 branch from bdc1a80 to af7e6fd Compare August 17, 2018 07:49

berkerpeksag requested changes Aug 19, 2018

View reviewed changes

bedevere-bot removed the awaiting change review label Aug 19, 2018

bedevere-bot added the awaiting changes label Aug 19, 2018

michael-o force-pushed the bpo-34412 branch from 77eaecd to f1331c0 Compare August 20, 2018 07:28

bedevere-bot added awaiting change review and removed awaiting changes labels Aug 20, 2018

michael-o force-pushed the bpo-34412 branch from f1331c0 to 7dc1f02 Compare August 22, 2018 15:44

bpo-34412: strsignal(3) does not exist on HP-UX

e35e799

Introduce a configure check for strsignal(3) which defines HAVE_STRSIGNAL for signalmodule.c. This change applies for Windows and HP-UX. Patch by Michael Osipov.

michael-o force-pushed the bpo-34412 branch from 1292428 to e35e799 Compare August 23, 2018 10:07

berkerpeksag approved these changes Aug 23, 2018

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting change review labels Aug 23, 2018

berkerpeksag merged commit 48ce489 into python:master Aug 23, 2018

bedevere-bot removed the awaiting merge label Aug 23, 2018

michael-o deleted the bpo-34412 branch August 23, 2018 14:35

		@@ -0,0 +1 @@
		Add HAVE_STRSIGNAL to avoid build failure on HP-UX with strsignal(3)

		@@ -0,0 +1 @@
		make :func:`signal.strsignal` work on HP-UX. Patch by Michael Osipov.

Uh oh!

bpo-34412: strsignal(3) does not exist on HP-UX #8786

bpo-34412: strsignal(3) does not exist on HP-UX #8786

Uh oh!

Conversation

michael-o commented Aug 16, 2018 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

berkerpeksag left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

berkerpeksag Aug 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

michael-o Aug 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bedevere-bot commented Aug 16, 2018

Uh oh!

michael-o commented Aug 16, 2018 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

michael-o commented Aug 16, 2018

Uh oh!

bedevere-bot commented Aug 16, 2018

Uh oh!

tiran left a comment

Choose a reason for hiding this comment

Uh oh!

bedevere-bot commented Aug 16, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

michael-o commented Aug 17, 2018

Uh oh!

bedevere-bot commented Aug 17, 2018

Uh oh!

michael-o commented Aug 17, 2018

Uh oh!

berkerpeksag commented Aug 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

michael-o commented Aug 17, 2018

Uh oh!

michael-o commented Aug 17, 2018

Uh oh!

michael-o commented Aug 17, 2018

Uh oh!

bedevere-bot commented Aug 17, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

michael-o commented Aug 16, 2018 •

edited by bedevere-bot

Loading

berkerpeksag Aug 16, 2018 •

edited

Loading

michael-o Aug 16, 2018 •

edited

Loading

michael-o commented Aug 16, 2018 •

edited by bedevere-bot

Loading

berkerpeksag commented Aug 17, 2018 •

edited

Loading