Skip to content
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

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

Merged
merged 4 commits into from
May 21, 2019

Conversation

erikjanss
Copy link
Contributor

@erikjanss erikjanss commented May 19, 2019

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

Modules/main.c Outdated
#ifdef _MSC_VER
# include <crtdbg.h> /* STATUS_CONTROL_C_EXIT */
#ifdef MS_WINDOWS
# include <winternl.h> /* NTSTATUS */
Copy link
Member

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

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 vstinner requested a review from gpshead May 20, 2019 15:23
@erikjanss 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
@erikjanss
Copy link
Contributor Author

erikjanss commented May 20, 2019

include statements were changed as discussed in bpo-36965

Copy link
Member

@zooba zooba left a comment

Choose a reason for hiding this comment

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

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.

@vstinner
Copy link
Member

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 vstinner closed this May 21, 2019
@vstinner vstinner reopened this May 21, 2019
@vstinner vstinner merged commit 925af1d into python:master May 21, 2019
@miss-islington
Copy link
Contributor

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

@vstinner
Copy link
Member

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?

@miss-islington
Copy link
Contributor

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

@miss-islington
Copy link
Contributor

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

@vstinner
Copy link
Member

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?

@erikjanss
Copy link
Contributor Author

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

erikjanss added a commit to erikjanss/cpython that referenced this pull request May 21, 2019
…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 pull request May 21, 2019
…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>
@bedevere-bot
Copy link

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

@erikjanss
Copy link
Contributor Author

@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 pushed a commit that referenced this pull request May 22, 2019
…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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants