Skip to content

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

Merged
vstinner merged 4 commits intopython:masterfrom
erikjanss:bpo-36965
May 21, 2019
Merged

bpo-36965: include windows.h to be able to use STATUS_CONTROL_C_EXIT#13421
vstinner merged 4 commits intopython:masterfrom
erikjanss:bpo-36965

Conversation

@erikjanss
Copy link
Contributor

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

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
vstinner reviewed May 20, 2019
View reviewed 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 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 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 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 •
edited by bedevere-bot
Loading

include statements were changed as discussed in bpo-36965

zooba
zooba approved these changes May 20, 2019
View reviewed changes
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

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

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 vstinner mentioned this pull request May 21, 2019
Closed
@vstinner
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?

@miss-islington
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.
🐍🍒⛏🤖

@miss-islington
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

@vstinner
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?

@erikjanss
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 pull request May 21, 2019
@erikjanss
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 pull request May 21, 2019
@erikjanss
[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>
@bedevere-bot
Copy link

bedevere-bot commented May 22, 2019

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

@erikjanss erikjanss mentioned this pull request May 22, 2019
Merged
@erikjanss
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 pushed a commit that referenced this pull request May 22, 2019
@erikjanss @vstinner
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 vstinner vstinner left review comments

@zooba zooba zooba approved these changes

@gpshead gpshead Awaiting requested review from gpshead

Assignees

@vstinner vstinner

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@erikjanss @vstinner @miss-islington @bedevere-bot @zooba @the-knights-who-say-ni