Enable PID file creation on WIN #15456

riordant · 2019-02-21T11:47:27Z

Introduction

As discussed with @laanwj on IRC:

PID file creation was never enabled for Windows, as the pid_t filetype is not available for it. However, the WIN32 API contains the header Processthreadsapi.h which in turn contains the function GetCurrentProcessId(). ~~This function is called at a higher level by _getpid()~~ EDIT: _getpid() is not available to the MSVC compiler used in the AppVeyor build. As a result, I have changed the function call toGetCurrentProcessId(), which performs the same function and is available to both MinGW & MSVC.
This allows one to capture the PID in Windows, without any additional includes - the above function is already available.
Within this PR, I have added a separate line that calls GetCurrentProcessId() in the case of a WIN compilation, and the usual getpid() otherwise. All code blocks processing PID file logic that avoid WIN32 have been changed to consider it. I have also updated the preprocessor definitions in libbitcoin_server.vcxproj.in to suppress a warning related to std::strerror for the MSVC build, that was causing the AppVeyor build to fail (see @fanquake comment below).

Rationale

Consistency between OS's running Bitcoin
- Applications which build off of bitcoind, such as novel front-end clients, often need access to the PID in order to control the daemon. Instead of designing some alternate way of doing this for one system, it should be consistent between all of them.

In collaboration with @joernroeder

laanwj · 2019-02-21T12:02:00Z

+4 -10 and adds a feature, this is a great first contribution, thanks!
utACK ca33ac10d36199395bd6553db77e5ceacfdb925e (given that this passes Travis and Appveyor)

riordant · 2019-02-21T12:29:15Z

Force pushed in order to add co-author.

ken2812221

~~utACK c4372b8b18d2fab2101d904289b78f87703bb6b9~~
You might have to include process.h?

fanquake · 2019-02-22T01:21:41Z

Appveyor build is failing with:

c:\projects\bitcoin\src\init.cpp(112): error C3861: '_getpid': identifier not found [C:\projects\bitcoin\build_msvc\libbitcoin_server\libbitcoin_server.vcxproj]

c:\projects\bitcoin\src\init.cpp(119): error C4996: 'strerror': This function or variable may be unsafe. Consider using strerror_s instead. To disable deprecation, use _CRT_SECURE_NO_WARNINGS. See online help for details. [C:\projects\bitcoin\build_msvc\libbitcoin_server\libbitcoin_server.vcxproj]

fanquake · 2019-02-22T01:38:29Z

I compiled this PR inside WSL and ran bitcoin-qt on a Windows 10 system.
Looks like bitcoin.pid is created correctly, and contains the correct PID.

riordant · 2019-02-22T07:51:22Z

Update: AppVeyor build should now pass. I have updated the description with details

ken2812221 · 2019-02-22T12:44:46Z

src/init.cpp

nit: Use fsbridge::get_filesystem_error_message(e) instead of e.what()

Ok, can be added. Just note this was not added/modified by me and was merged in #15278 a few days ago

That was OK to use e.what() in other OS except Windows. So you have to use fsbridge::get_filesystem_error_message(e) instead.

practicalswift · 2019-02-24T19:34:08Z

Concept ACK

Excellent first-time contribution! Thanks! Hope to see more PR:s from you! :-)

hebasto · 2019-02-24T20:47:37Z

Concept ACK.

laanwj · 2019-02-25T08:35:34Z

utACK after squash, I think this is better as one commit to have an atomic change

- Add available WIN PID function - Consider WIN32 in each relevant case - Add new preprocessor definitions to suppress warning - Update error message for generic OS Co-authored-by: Jörn Röder <[email protected]>

riordant · 2019-02-25T09:10:58Z

Yep agree, done.

laanwj · 2019-02-25T12:07:04Z

thanks!
utACK 3f5ad62

@laanwj

3f5ad62 Enable PID file creation on Windows - Add available WIN PID function - Consider WIN32 in each relevant case - Add new preprocessor definitions to suppress warning - Update error message for generic OS (riordant) Pull request description: # Introduction As discussed with @laanwj on IRC: - PID file creation was never enabled for Windows, as the `pid_t` filetype is not available for it. However, the WIN32 API contains the header [`Processthreadsapi.h`](https://github.com/CodeShark/x86_64-w64-mingw32/blob/master/include/processthreadsapi.h) which in turn contains the function [`GetCurrentProcessId()`](https://docs.microsoft.com/en-gb/windows/desktop/api/processthreadsapi/nf-processthreadsapi-getcurrentprocessid). ~~This function is called at a higher level by [`_getpid()`](https://docs.microsoft.com/en-us/cpp/c-runtime-library/reference/getpid?view=vs-2017)~~ EDIT: `_getpid()` is not available to the MSVC compiler used in the AppVeyor build. As a result, I have changed the function call to`GetCurrentProcessId()`, which performs the same function and is available to both MinGW & MSVC. This allows one to capture the PID in Windows, without any additional includes - the above function is already available. - Within this PR, I have added a separate line that calls `GetCurrentProcessId()` in the case of a WIN compilation, and the usual `getpid()` otherwise. All code blocks processing PID file logic that avoid WIN32 have been changed to consider it. I have also updated the preprocessor definitions in `libbitcoin_server.vcxproj.in` to suppress a warning related to `std::strerror` for the MSVC build, that was causing the AppVeyor build to fail (see @fanquake comment below). # Rationale - Consistency between OS's running Bitcoin - Applications which build off of `bitcoind`, such as novel front-end clients, often need access to the PID in order to control the daemon. Instead of designing some alternate way of doing this for one system, it should be consistent between all of them. In collaboration with @joernroeder Tree-SHA512: 22fcbf866e99115d12ed29716e68d200d4c118ae2f7b188b7705dc0cf5f0cd0ce5fb18f772744c6238eecd9e6d0922c615e2f0e12a7fe7c810062a79d97aa6a2

Summary: Enable PID file creation on Windows - Add available WIN PID function - Consider WIN32 in each relevant case - Add new preprocessor definitions to suppress warning - Update error message for generic OS Co-authored-by: Jörn Röder <[email protected]> This is a backport of Core [[bitcoin/bitcoin#15456 | PR15456]] Depends on D5775 Test Plan: make check Run the win64 build and run bitcon-qt Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D5776

@laanwj

3f5ad62 Enable PID file creation on Windows - Add available WIN PID function - Consider WIN32 in each relevant case - Add new preprocessor definitions to suppress warning - Update error message for generic OS (riordant) Pull request description: # Introduction As discussed with @laanwj on IRC: - PID file creation was never enabled for Windows, as the `pid_t` filetype is not available for it. However, the WIN32 API contains the header [`Processthreadsapi.h`](https://github.com/CodeShark/x86_64-w64-mingw32/blob/master/include/processthreadsapi.h) which in turn contains the function [`GetCurrentProcessId()`](https://docs.microsoft.com/en-gb/windows/desktop/api/processthreadsapi/nf-processthreadsapi-getcurrentprocessid). ~~This function is called at a higher level by [`_getpid()`](https://docs.microsoft.com/en-us/cpp/c-runtime-library/reference/getpid?view=vs-2017)~~ EDIT: `_getpid()` is not available to the MSVC compiler used in the AppVeyor build. As a result, I have changed the function call to`GetCurrentProcessId()`, which performs the same function and is available to both MinGW & MSVC. This allows one to capture the PID in Windows, without any additional includes - the above function is already available. - Within this PR, I have added a separate line that calls `GetCurrentProcessId()` in the case of a WIN compilation, and the usual `getpid()` otherwise. All code blocks processing PID file logic that avoid WIN32 have been changed to consider it. I have also updated the preprocessor definitions in `libbitcoin_server.vcxproj.in` to suppress a warning related to `std::strerror` for the MSVC build, that was causing the AppVeyor build to fail (see @fanquake comment below). # Rationale - Consistency between OS's running Bitcoin - Applications which build off of `bitcoind`, such as novel front-end clients, often need access to the PID in order to control the daemon. Instead of designing some alternate way of doing this for one system, it should be consistent between all of them. In collaboration with @joernroeder Tree-SHA512: 22fcbf866e99115d12ed29716e68d200d4c118ae2f7b188b7705dc0cf5f0cd0ce5fb18f772744c6238eecd9e6d0922c615e2f0e12a7fe7c810062a79d97aa6a2

fanquake added the Windows label Feb 21, 2019

fanquake requested a review from ken2812221 February 21, 2019 12:54

ken2812221 approved these changes Feb 21, 2019

View reviewed changes

ken2812221 reviewed Feb 22, 2019

View reviewed changes

Enable PID file creation on Windows

3f5ad62

- Add available WIN PID function - Consider WIN32 in each relevant case - Add new preprocessor definitions to suppress warning - Update error message for generic OS Co-authored-by: Jörn Röder <[email protected]>

laanwj merged commit 3f5ad62 into bitcoin:master Feb 25, 2019

bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021

Enable PID file creation on WIN #15456

Enable PID file creation on WIN #15456

Uh oh!

Conversation

riordant commented Feb 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Introduction

Rationale

Uh oh!

laanwj commented Feb 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

riordant commented Feb 21, 2019

Uh oh!

ken2812221 left a comment • 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!

fanquake commented Feb 22, 2019

Uh oh!

fanquake commented Feb 22, 2019

Uh oh!

riordant commented Feb 22, 2019

Uh oh!

ken2812221 Feb 22, 2019

Choose a reason for hiding this comment

Uh oh!

riordant Feb 23, 2019

Choose a reason for hiding this comment

Uh oh!

ken2812221 Feb 24, 2019

Choose a reason for hiding this comment

Uh oh!

practicalswift commented Feb 24, 2019

Uh oh!

hebasto commented Feb 24, 2019

Uh oh!

laanwj commented Feb 25, 2019

Uh oh!

riordant commented Feb 25, 2019

Uh oh!

laanwj commented Feb 25, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

riordant commented Feb 21, 2019 •

edited

Loading

laanwj commented Feb 21, 2019 •

edited

Loading

ken2812221 left a comment •

edited

Loading