[proxy] web.archive.org — ← back | site home | direct (HTTPS) ↗ | proxy home | ◑ dark◐ light

/ cpython Public

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

Jump to bottom

bpo-36965: include windows.h to be able to use STATUS_CONTROL_C_EXIT #13421

Merged
merged 4 commits into from May 21, 2019
Merged

bpo-36965: include windows.h to be able to use STATUS_CONTROL_C_EXIT #13421

merged 4 commits into from May 21, 2019

Conversation

Copy link
Contributor

erikjanss commented May 19, 2019 •
edited by bedevere-bot

According to the Microsoft documentation at

https://docs.microsoft.com/en-us/windows-hardware/drivers/kernel/using-ntstatus-values

system-supplied status codes are defined in ntstatus.h. and the NTSTATUS type is defined in ntdef.h

this PR includes both ntstatus.h and ntdef.h to be able to use STATUS_CONTROL_C_EXIT when compiling for windows.

https://bugs.python.org/issue36965

vstinner reviewed May 20, 2019
View changes
Modules/main.c Outdated
#ifdef _MSC_VER
# include <crtdbg.h> /* STATUS_CONTROL_C_EXIT */
#ifdef MS_WINDOWS
# include <winternl.h> /* NTSTATUS */
Copy link
Member

vstinner May 20, 2019

Choose a reason for hiding this comment

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

I don't understand what this include is needed, the file doesn't use NTSTATUS.

Copy link
Contributor Author

erikjanss May 20, 2019

Choose a reason for hiding this comment

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

ntstatus.h contains the value of STATUS_CONTROL_C_EXIT, and that value is of type NTSTATUS.

however ntstatus.h does not contain the definition of NTSTATUS and neither does it include that definition. So, only including ntstatus.h will not compile. I found the documentation linked above that specifies that one should include ntdef.h to have the definition of NTSTATUS.

but then again, that seems to have been deprecated, and I found comments in the headers that winternl.h should be used instead of ntdef.h

(I'm not much of a windows developer, so I only tried to parse the available documentation)

vstinner requested a review from gpshead May 20, 2019
erikjanss changed the title bpo-36965: include ntstatus.h when compiling for windows bpo-36965: include windows.h to be able to use STATUS_CONTROL_C_EXIT May 20, 2019
Copy link
Contributor Author

erikjanss commented May 20, 2019 •
edited by bedevere-bot

include statements were changed as discussed in bpo-36965

zooba approved these changes May 20, 2019
View changes
Copy link
Member

zooba left a comment

I approve this. Since @gpshead was called in too I'll let him weigh in or merge, in case he has some special knowledge about this area.

Copy link
Member

vstinner commented May 21, 2019

Azure Pipelines PR: Windows PR Tests win64 failed with timeout:

534 0:11:08 load avg: 3.52 [420/421] test_multiprocessing_spawn passed (2 min 24 sec) -- running: test_import (1 min 31 sec)
535running: test_import (2 min 1 sec)
536running: test_import (2 min 31 sec)
(...)
620running: test_import (44 min 32 sec)
621running: test_import (45 min 2 sec)
622Terminate batch job (Y/N)?

I close/reopen the PR to schedule a new job.

vstinner closed this May 21, 2019
vstinner reopened this May 21, 2019
vstinner merged commit 925af1d into python:master May 21, 2019
5 checks passed
Copy link
Contributor

miss-islington commented May 21, 2019

Thanks @erikjanss for the PR, and @vstinner for merging it 🌮🎉.. I'm working now to backport this PR to: 3.7.
🐍🍒⛏🤖

vstinner mentioned this pull request May 21, 2019
Closed
Copy link
Member

vstinner commented May 21, 2019

Hum, our bot creating backport is broken: python/miss-islington#228

@erikjanss: Can you please try to create manually a PR to backport your fix?

Copy link
Contributor

miss-islington commented May 21, 2019

Thanks @erikjanss for the PR, and @vstinner for merging it 🌮🎉.. I'm working now to backport this PR to: 3.7.
🐍🍒⛏🤖

Copy link
Contributor

miss-islington commented May 21, 2019

Sorry, @erikjanss and @vstinner, I could not cleanly backport this to 3.7 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker 925af1d99b69bf3e229411022ad840c5a0cfdcf8 3.7

Copy link
Member

vstinner commented May 21, 2019

Sorry, @erikjanss and @vstinner, I could not cleanly backport this to 3.7 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker 925af1d 3.7

The bot first failed with a timeout, but then run correctly, except that there is a merge conflict.

@erikjanss: Agai, can you please backport manually the fix?

Copy link
Contributor Author

erikjanss commented May 21, 2019

@vstinner, I'll give the manual backport a try ...

erikjanss added a commit to erikjanss/cpython that referenced this issue May 21, 2019
bpo-36965: Fix includes in main.c on Windows with non-MSC compilers (p…
83c14f7 
…ythonGH-13421)

Include windows.h rather than crtdbg.h to get STATUS_CONTROL_C_EXIT constant.
Moreover, include windows.h on Windows, not only when MSC is used.

(cherry picked from commit 925af1d)
erikjanss added a commit to erikjanss/cpython that referenced this issue May 21, 2019
[3.7] bpo-36965: Fix includes in main.c on Windows with non-MSC compi…
220fdec 
…lers (pythonGH-13421)

Include windows.h rather than crtdbg.h to get STATUS_CONTROL_C_EXIT constant.
Moreover, include windows.h on Windows, not only when MSC is used..
(cherry picked from commit 925af1d)

Co-authored-by: Erik Janssens <erik.janssens@conceptive.be>
Copy link

bedevere-bot commented May 22, 2019

GH-13471 is a backport of this pull request to the 3.7 branch.

erikjanss mentioned this pull request May 22, 2019
Merged
Copy link
Contributor Author

erikjanss commented May 22, 2019

@vstinner, backport ready in GH-13471, what I did not evaluate though was if that include is needed at all, since STATUS_CONTROL_C_EXIT is not used in 3.7

vstinner added a commit that referenced this issue May 22, 2019
bpo-36965: Fix includes in main.c on Windows with non-MSC compilers (G…
791e5fc 
…H-13421) (GH-13471)

Include windows.h rather than crtdbg.h to get STATUS_CONTROL_C_EXIT constant.
Moreover, include windows.h on Windows, not only when MSC is used.

(cherry picked from commit 925af1d)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

vstinner

zooba

gpshead

Assignees

vstinner

Labels
CLA signed
Projects
None yet
Milestone
No milestone
6 participants