Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

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

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
berkerpeksag merged 1 commit intopython:masterfrommichael-o:bpo-34412
Aug 23, 2018

Conversation

@michael-o
Copy link
Contributor

@michael-omichael-o commentedAug 16, 2018•
edited by bedevere-bot
Loading

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

Choose a reason for hiding this comment

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

This comment is now somewhat inaccurate

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

Shall I drop it?

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

@berkerpeksagberkerpeksag left a comment

Choose a reason for hiding this comment

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

  1. Please add a NEWS entry. Seehttps://devguide.python.org/committing/#what-s-new-and-news-entries for details.

  2. Please regenerateconfigure.

  3. Does thetest_strsignal test inLib/test/test_signal.py passed on HP-UX when you applied this patch?

configure.ac Outdated
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

I have considered this, but it isnowhere documented that it will includestring.h. I went for the safe bet.

Copy link
Member

@berkerpeksagberkerpeksagAug 16, 2018•
edited
Loading

Choose a reason for hiding this comment

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

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() requiresunistd.h andModules/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 addingsignalstr to that list.

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

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

I will check that too.

Copy link
ContributorAuthor

@michael-omichael-oAug 16, 2018•
edited
Loading

Choose a reason for hiding this comment

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

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

@bedevere-bot
Copy link

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 phraseI 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
Copy link
ContributorAuthor

michael-o commentedAug 16, 2018•
edited by bedevere-bot
Loading

@berkerpeksag

  1. Will do
  2. Will do, though I do not understand why you are committing a generated file.
  3. 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  buildrunning buildrunning build_ext...Python build finished successfully!The necessary bits to build these optional modules were not found:_gdbm                 _lzma                 _tkinter_uuid                 ossaudiodev           spwdTo 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 beenbuilt by the Makefile instead, as configured by the Setup files:_abc                  atexit                pwdtimeFailed to build these modules:_curses_panelFollowing modules built successfully but were removed because they could not be imported:_cursesrunning build_scriptscopying and adjusting /var/osipovmi/cpython/Tools/scripts/pydoc3 -> build/scripts-3.8copying and adjusting /var/osipovmi/cpython/Tools/scripts/idle3 -> build/scripts-3.8copying and adjusting /var/osipovmi/cpython/Tools/scripts/2to3 -> build/scripts-3.8changing mode of build/scripts-3.8/pydoc3 from 640 to 755changing mode of build/scripts-3.8/idle3 from 640 to 755changing mode of build/scripts-3.8/2to3 from 640 to 755renaming build/scripts-3.8/pydoc3 to build/scripts-3.8/pydoc3.8renaming build/scripts-3.8/idle3 to build/scripts-3.8/idle3.8renaming 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-8Using random seed 9950569Run tests in parallel using 6 child processes0:00:25 [1/1/1] test_signal failedtest_enums (test.test_signal.GenericTests) ... oktest_itimer_exc (test.test_signal.ItimerTest) ... oktest_itimer_prof (test.test_signal.ItimerTest) ... oktest_itimer_real (test.test_signal.ItimerTest) ... oktest_itimer_virtual (test.test_signal.ItimerTest) ... oktest_setitimer_tiny (test.test_signal.ItimerTest) ... oktest_pthread_kill (test.test_signal.PendingSignalsTests) ... oktest_pthread_kill_main_thread (test.test_signal.PendingSignalsTests) ... oktest_pthread_sigmask (test.test_signal.PendingSignalsTests) ... oktest_pthread_sigmask_arguments (test.test_signal.PendingSignalsTests) ... oktest_pthread_sigmask_valid_signals (test.test_signal.PendingSignalsTests) ... oktest_sigpending (test.test_signal.PendingSignalsTests) ... oktest_sigpending_empty (test.test_signal.PendingSignalsTests) ... oktest_sigtimedwait (test.test_signal.PendingSignalsTests) ... oktest_sigtimedwait_negative_timeout (test.test_signal.PendingSignalsTests) ... oktest_sigtimedwait_poll (test.test_signal.PendingSignalsTests) ... oktest_sigtimedwait_timeout (test.test_signal.PendingSignalsTests) ... oktest_sigwait (test.test_signal.PendingSignalsTests) ... oktest_sigwait_thread (test.test_signal.PendingSignalsTests) ... oktest_sigwaitinfo (test.test_signal.PendingSignalsTests) ... oktest_getsignal (test.test_signal.PosixTests) ... oktest_interprocess_signal (test.test_signal.PosixTests) ... ok...

@michael-omichael-oforce-pushed thebpo-34412 branch 2 times, most recently from26627f0 to20df665CompareAugust 16, 2018 17:08
@michael-o
Copy link
ContributorAuthor

I have made the requested changes; please review again

@bedevere-bot
Copy link

Thanks for making the requested changes!

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

tiran
tiran previously requested changesAug 16, 2018
Copy link
Member

@tirantiran left a comment

Choose a reason for hiding this comment

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

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
Copy link

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 phraseI 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.

Copy link
Member

Choose a reason for hiding this comment

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

I thinkSIGHUP is a pretty common signal, so it can be added to the list inModules/signalmodule.c:

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

Please add "Patch by Your Name.".

@michael-omichael-oforce-pushed thebpo-34412 branch 2 times, most recently from391f445 tobdc1a80CompareAugust 17, 2018 07:38
@michael-o
Copy link
ContributorAuthor

I have made the requested changes; please review again

@bedevere-bot
Copy link

Thanks for making the requested changes!

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

@michael-o
Copy link
ContributorAuthor

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

@berkerpeksag
Copy link
Member

berkerpeksag commentedAug 17, 2018•
edited
Loading

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 ifHAVE_STRSIGNAL andMS_WINDOWS aren't defined.

@michael-o
Copy link
ContributorAuthor

@berkerpeksag

So you are looking for this:

diff --git a/Modules/signalmodule.c b/Modules/signalmodule.cindex 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
Copy link
ContributorAuthor

@berkerpeksag Change pushed.

@michael-o
Copy link
ContributorAuthor

I have made the requested changes; please review again

@bedevere-bot
Copy link

Thanks for making the requested changes!

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

Copy link
Member

Choose a reason for hiding this comment

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

Nit: make -> Make

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

Will do.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

Well, I can go throughsignal.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?

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

@berkerpeksag I went throughsignal.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.

Copy link
Member

Choose a reason for hiding this comment

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

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 makestrsignal() useful for most common cases.

I just checkedhttp://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.

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

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 aboutstrsignal(3) that the return value is implementation-defined.

@berkerpeksag WDYT?

Copy link
Member

Choose a reason for hiding this comment

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

Awesome, thanks! No need to update the tests.

I will take another look at this and merge tonight.

@bedevere-bot
Copy link

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 phraseI 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
Copy link
ContributorAuthor

I have made the requested changes; please review again

@bedevere-bot
Copy link

Thanks for making the requested changes!

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

@michael-o
Copy link
ContributorAuthor

@berkerpeksag are we good to merge?

@berkerpeksag
Copy link
Member

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

diff --git a/pyconfig.h.in b/pyconfig.h.inindex 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

@berkerpeksagberkerpeksag dismissedtiran’sstale reviewAugust 22, 2018 16:59

Addressed in the latest review. Thanks!

Introduce a configure check for strsignal(3) which defines HAVE_STRSIGNAL forsignalmodule.c. This change applies for Windows and HP-UX.Patch by Michael Osipov.
@michael-o
Copy link
ContributorAuthor

I have made the requested changes; please review again

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

@bedevere-bot
Copy link

Thanks for making the requested changes!

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

@berkerpeksag
Copy link
Member

Thanks!

@michael-omichael-o deleted the bpo-34412 branchAugust 23, 2018 14:35
@michael-o
Copy link
ContributorAuthor

@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
Copy link
Member

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

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@berkerpeksagberkerpeksagberkerpeksag approved these changes

@tirantirantiran left review comments

+1 more reviewer

@ppperyppperypppery left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@michael-o@bedevere-bot@berkerpeksag@tiran@pppery@the-knights-who-say-ni
[8]ページ先頭
©2009-2025 Movatter.jp