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

Enable PID file creation on WIN #15456

Merged
merged 1 commit into from Feb 25, 2019

Conversation

Projects
None yet
6 participants
@riordant
Copy link
Contributor

commented Feb 21, 2019

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

@fanquake fanquake added the Windows label Feb 21, 2019

@laanwj

This comment has been minimized.

Copy link
Member

commented Feb 21, 2019

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

@riordant riordant force-pushed the riordant:master branch 2 times, most recently from 0e6dc30 to c4372b8 Feb 21, 2019

@riordant

This comment has been minimized.

Copy link
Contributor Author

commented Feb 21, 2019

Force pushed in order to add co-author.

@fanquake fanquake requested a review from ken2812221 Feb 21, 2019

@ken2812221
Copy link
Member

left a comment

utACK c4372b8
You might have to include process.h?

@fanquake

This comment has been minimized.

Copy link
Member

commented Feb 22, 2019

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

This comment has been minimized.

Copy link
Member

commented Feb 22, 2019

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.

windows

@riordant

This comment has been minimized.

Copy link
Contributor Author

commented Feb 22, 2019

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

@@ -286,15 +288,13 @@ void Shutdown(InitInterfaces& interfaces)
}
#endif

#ifndef WIN32
try {
if (!fs::remove(GetPidFile())) {
LogPrintf("%s: Unable to remove PID file: File does not exist\n", __func__);
}
} catch (const fs::filesystem_error& e) {
LogPrintf("%s: Unable to remove PID file: %s\n", __func__, e.what());

This comment has been minimized.

Copy link
@ken2812221

ken2812221 Feb 22, 2019

Member

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

This comment has been minimized.

Copy link
@riordant

riordant Feb 23, 2019

Author Contributor

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

This comment has been minimized.

Copy link
@ken2812221

ken2812221 Feb 24, 2019

Member

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

This comment has been minimized.

Copy link
Member

commented Feb 24, 2019

Concept ACK

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

@hebasto

This comment has been minimized.

Copy link
Member

commented Feb 24, 2019

Concept ACK.

@laanwj

This comment has been minimized.

Copy link
Member

commented Feb 25, 2019

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

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 <kontakt@joernroeder.de>

@riordant riordant force-pushed the riordant:master branch from 9a4c7b7 to 3f5ad62 Feb 25, 2019

@riordant

This comment has been minimized.

Copy link
Contributor Author

commented Feb 25, 2019

Yep agree, done.

@laanwj

This comment has been minimized.

Copy link
Member

commented Feb 25, 2019

thanks!
utACK 3f5ad62

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

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

laanwj added a commit that referenced this pull request Feb 25, 2019

Merge #15456: Enable PID file creation on WIN
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.