Skip to content

Check std::system for -[alert|block|wallet]notify #15457

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.

Sign up for GitHub

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
Merged
merged 3 commits into from
Jul 5, 2019
Merged

Check std::system for -[alert|block|wallet]notify #15457

merged 3 commits into from
Jul 5, 2019

Conversation

Sjors
Copy link
Member

@Sjors Sjors commented Feb 21, 2019 •
edited
Loading

Platforms such as iOs and Universal Windows Platform do not support launching a process through system().

@Sjors
Copy link
Member Author

Sjors commented Feb 21, 2019 •
edited
Loading

One way to test this is by following the iOs cross-compilation instructions in #12557:

make clean

./configure
...
checking for std::system... yes
...

make clean

./configure --prefix=`pwd`/depends/aarch64-apple-darwin14
...
checking for std::system... no
...

Note that ./configure can be a bit stubborn about caching things.

@Sjors Sjors mentioned this pull request Feb 21, 2019
Closed
promag
promag reviewed Feb 21, 2019
View reviewed changes
Copy link
Contributor

@promag promag left a comment

Choose a reason for hiding this comment

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

Concept ACK. Could make runCommand noop instead of multiple guards?

@maflcko
Copy link
Member

maflcko commented Feb 21, 2019

ACK c0e9ac7 (checked that compiling without HAVE_STD__SYSTEM and running the tests results in failure of

feature_notifications.py              | ✖ Failed  | 1 s
feature_versionbits_warning.py        | ✖ Failed  | 0 s

This should be fine, since I don't expect anyone running the functional tests on ios)

@DrahtBot
Copy link
Contributor

DrahtBot commented Feb 21, 2019 •
edited
Loading

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #13339 (wallet: Replace %w by wallet name in -walletnotify script by promag)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

@promag
Copy link
Contributor

promag commented Feb 21, 2019

@MarcoFalke looks like HAVE_STD__SYSTEM could be added to test/config.ini.in so that BitcoinTestFramework.skip_if_no_std_system or something could be added.

@ken2812221
Copy link
Contributor

Concept ACK. Maybe those arguments should be added into hidden_args to keep config file portability.

@maflcko
Copy link
Member

maflcko commented Feb 21, 2019

  • If they are added to hidden args, might as well just make runCommand a noop (since that is doing effectively the same)
  • BitcoinTestFramework.skip_if_no_std_system meh. Not sure if this is ever needed unless someone wants to run the functional tests on ios (with a native compile)

@promag
Copy link
Contributor

promag commented Feb 21, 2019

@MarcoFalke agree :trollface: I was curious how it currently detects if wallet is enabled.

@Sjors
Copy link
Member Author

Sjors commented Feb 21, 2019 •
edited
Loading

I can imagine someone running the functional test suite on their machine against bitcoind on the sandboxed device. But that's likely going to require additional changes to the test framework, so perhaps that's a better time to design something like BitcoinTestFramework.skip_if_no_std_system.

As for running the functional tests themselves from the device, that would also require modifications since the test framework relies on launching processes.

I'm not a fan of turning runCommand into a noop. I still need multiple guards to remove it from the help, and I want functions that call runCommand to explicitly handle its absence.

@promag
Copy link
Contributor

promag commented Feb 22, 2019

I still need multiple guards to remove it from the help, and I want functions that call runCommand to explicitly handle its absence.

👍

@jonasschnelli
Copy link
Contributor

Concept ACK

@laanwj
Copy link
Member

laanwj commented Feb 22, 2019

I can certainly see his being useful in sandboxed environments of various kinds. Vaguely remember a similar thing when trying to get bitcoind to work in CloudABI.
utACK

fanquake
fanquake reviewed Feb 22, 2019
View reviewed changes
@Sjors Sjors force-pushed the 2019/02/std__system branch 3 times, most recently from 3591467 to d9255a1 Compare February 22, 2019 12:59
@DrahtBot
Copy link
Contributor

Gitian builds for commit a094b54 (master):

Gitian builds for commit b2eb95b (master and this pull):

laanwj
laanwj reviewed Mar 8, 2019
View reviewed changes
src/init.cpp Outdated
@@ -576,6 +580,7 @@ std::string LicenseInfo()
"\n";
}

#if defined(HAVE_STD__SYSTEM) || defined(WIN32)
Copy link
Member

Choose a reason for hiding this comment

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

I think repeating this condition everywhere is error-prone. Can't we make sure HAVE_STD__SYSTEM is always set when WIN32 instead?

Copy link
Member Author

Choose a reason for hiding this comment

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

I changed it to check for either std::system or ::wsystem, so we can now use HAVE_SYSTEM.

@Sjors Sjors force-pushed the 2019/02/std__system branch from d9255a1 to de0e729 Compare March 8, 2019 17:12
Empact
Empact reviewed Mar 9, 2019
View reviewed changes
Empact
Empact reviewed Mar 9, 2019
View reviewed changes
@Sjors Sjors force-pushed the 2019/02/std__system branch 3 times, most recently from d7d72a9 to 620c77e Compare March 14, 2019 10:30
@Sjors
Copy link
Member Author

Sjors commented Mar 14, 2019

I tested the Gitian build on Windows and -walletnotify still works. I also managed to build using Visual Studio and it also works there.

I'm confused why feature_notifications.py is failing on AppVeyor. I'm still figuring out how to run the functional tests from the Visual Studio build (something with test/config.ini it seems) to reproduce...

My guess is that AppVeyor builds in a way that doesn't have ::wsystem available (e.g. Universal Windows Platform), which would cause -walletnotify to not get compiled in, but also cause the test to fail unless I add some detection to it.

@Sjors
Copy link
Member Author

Sjors commented Mar 14, 2019

@sipsorcery do you know how to run the functional tests on Windows?

@maflcko
Copy link
Member

maflcko commented Mar 14, 2019

@Sjors See e.g. https://github.com/MarcoFalke/btc_nightly/blob/1501ae84f3f935bcc3889e1232fa462b0ecbb131/.travis.yml#L49

@sipsorcery
Copy link
Contributor

@Sjors yes I can run the functional tests on Windows. If you're alluding to why the appveyor check failed it's almost certainly due to the length of time it's taking rather than the functional test. That's assuming the test hasn't changed since I last checked it a week or so ago.

Maybe AppVeyor would bump up the time limit from 60 to 90 minutes for Bitcoin Core if asked.

@Sjors
Copy link
Member Author

Sjors commented Mar 14, 2019 •
edited
Loading

Ok, it involved some manual work, but I'm able to run to functional tests now.

[environment]
SRCDIR=C:\Users\sjors\bitcoin
BUILDDIR=C:\Users\sjors\bitcoin
EXEEXT=.exe
RPCAUTH=C:\Users\sjors\bitcoin\share\rpcauth\rpcauth.py

[components]
# Which components are enabled. These are commented out by `configure` if they were disabled when running config.
ENABLE_WALLET=true
ENABLE_CLI=true
ENABLE_BITCOIND=true
ENABLE_ZMQ=true�

And you have copy the exe files:

mv .\build_msvc\x64\Debug\*.exe .\src

.\test\functional\feature_notifications.py� passes.

By the way, it's complaining about the bitcoin unicode symbol: UnicodeEncodeError: 'charmap' codec can't encode character '\u20bf'�.

@sipsorcery
Copy link
Contributor

By the way, it's complaining about the bitcoin unicode symbol: UnicodeEncodeError: 'charmap' codec can't encode character '\u20bf'�.

Powershell: $env:PYTHONUTF8=1
or
cmd: set PYTHONUTF8=1

@Sjors
Copy link
Member Author

Sjors commented Mar 14, 2019

If you're alluding to why the appveyor check failed it's almost certainly due to the length of time it's taking rather than the functional test. That's assuming the test hasn't changed since I last checked it a week or so ago.

The test that fails is directly related to this PR and I've reproduced it on two separate AppVeyor builds (trying the 3rd one now). Bumping to 90 minutes would be great indeed, because whenever the cache is cleared, it only makes it halfway through the function tests. That happened here as well (but the error I'm seeing here was after a rebuild).

@Sjors
Copy link
Member Author

Sjors commented Mar 14, 2019 •
edited
Loading

Powershell: $env:PYTHONUTF8=1

That worked, thanks. It seems like sometimes I have to hit a key before the dots start / continue appearing for the test runner.

I'm also missing some dependency for .find_library ('ssl') or 'libeay32')� (OpenSSL?).

@Sjors
Copy link
Member Author

Sjors commented Mar 15, 2019

I managed to reproduce on my own MSVC setup; I forgot to checkout this branch :-)

It turns out configure.ac is ignored, so instead I added HAVE_SYSTEM to build_msvc/bitcoin_config.h.

@Sjors Sjors force-pushed the 2019/02/std__system branch from 07dd553 to 77a5eb3 Compare March 15, 2019 10:15
@Sjors Sjors mentioned this pull request Mar 15, 2019
Closed
@practicalswift
Copy link
Contributor

practicalswift commented Mar 21, 2019 •
edited
Loading

FWIW the disassembly of the bitcoind binary is identical before and after applying this patch on my Ubuntu 18.04 system (as expected).

@Sjors Sjors force-pushed the 2019/02/std__system branch from 77a5eb3 to f874e14 Compare June 6, 2019 09:59
@laanwj
Copy link
Member

laanwj commented Jul 5, 2019

code review ACK f874e14

@laanwj laanwj merged commit f874e14 into bitcoin:master Jul 5, 2019
laanwj added a commit that referenced this pull request Jul 5, 2019
@laanwj
Merge #15457: Check std::system for -[alert|block|wallet]notify
8c69fae 
f874e14 [build]: check std::system for -[alert|block|wallet]notify (Sjors Provoost)
cc3ad56 [build] MSVC: set HAVE_SYSTEM for desktop apps (Sjors Provoost)
c1c91bb [build] detect std::system or ::wsystem (Sjors Provoost)

Pull request description:

  Platforms such as iOs and Universal Windows Platform do not support launching a process through system().

ACKs for top commit:
  laanwj:
    code review ACK f874e14

Tree-SHA512: 16bb4a8fa1896046ccb22a46c8985e1aa45f5b11ecf5539eb2299e9a58f1a5b085c0c12cb6939c7493d93abce7e84fadcbfc73374c887db63da6d00c08aa476d
@Sjors Sjors deleted the 2019/02/std__system branch July 5, 2019 15:35
@Sjors Sjors mentioned this pull request Jul 5, 2019
Merged
@Sjors
Copy link
Member Author

Sjors commented Jul 5, 2019 •
edited
Loading

During my rebase of #12557 I noticed this doesn't actually work as advertised. Created a followup #16344. I think it broke when I refactored from if defined(HAVE_STD__SYSTEM) || defined(WIN32) to using AC_DEFINE([HAVE_SYSTEM], [HAVE_STD__SYSTEM || HAVE_WSYSTEM]. The latter seems to always define HAVE_SYSTEM, so I need to use #IF instead.

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Jul 6, 2019
@laanwj @sidhujag
Merge bitcoin#15457: Check std::system for -[alert|block|wallet]notify
a851e93 
f874e14 [build]: check std::system for -[alert|block|wallet]notify (Sjors Provoost)
cc3ad56 [build] MSVC: set HAVE_SYSTEM for desktop apps (Sjors Provoost)
c1c91bb [build] detect std::system or ::wsystem (Sjors Provoost)

Pull request description:

  Platforms such as iOs and Universal Windows Platform do not support launching a process through system().

ACKs for top commit:
  laanwj:
    code review ACK f874e14

Tree-SHA512: 16bb4a8fa1896046ccb22a46c8985e1aa45f5b11ecf5539eb2299e9a58f1a5b085c0c12cb6939c7493d93abce7e84fadcbfc73374c887db63da6d00c08aa476d
fanquake added a commit that referenced this pull request Jul 7, 2019
@fanquake
Merge #16344: build: use #if HAVE_SYSTEM instead of defined(HAVE_SYSTEM)
f373bee 
976b034 [build]: use #if HAVE_SYSTEM instead of defined(HAVE_SYSTEM) (Sjors Provoost)

Pull request description:

  It seems that `AC_DEFINE([HAVE_SYSTEM], [HAVE_STD__SYSTEM || HAVE_WSYSTEM]` causes `HAVE_SYSTEM` to always be defined, so we need to use `#if HAVE_SYSTEM` instead of `#if defined(HAVE_SYSTEM)`.

  Followup for #15457, can be tested with #12557.

ACKs for top commit:
  dongcarl:
    ACK 976b034.
  promag:
    ACK 976b034.
  fanquake:
    ACK 976b034

Tree-SHA512: b8cdd04c2ec399fd15638aef5d75ea0886ec1572d3cf4fcea27c193e1e6390344315908262cad8981a9b0a905ab9520619ce2ffe9a717f4ee6bfa8b028ebbdc6
jasonbcox pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Aug 4, 2020
@laanwj @majcosta
[backport#15457] Check std::system for -[alert|block|wallet]notify
8e243c9 
Summary:
f874e14cd3c84cd412bd3fb42b3ee1706ca6a267 [build]: check std::system for -[alert|block|wallet]notify (Sjors Provoost)
cc3ad56ff2bc2583fe68c4a9e0b41072a47c0b07 [build] MSVC: set HAVE_SYSTEM for desktop apps (Sjors Provoost)
c1c91bb78d7267f01ee3a3c156c218b46a92cd39 [build] detect std::system or ::wsystem (Sjors Provoost)

Pull request description:

  Platforms such as iOs and Universal Windows Platform do not support launching a process through system().

https://github.com/bitcoin/bitcoin/pull/15457/files

---

Backport of Core [[bitcoin/bitcoin#15457 | PR15457]]

Test Plan:
  ninja check

don't have a windows machine atm to test this on

Reviewers: #bitcoin_abc, deadalnix

Reviewed By: #bitcoin_abc, deadalnix

Subscribers: deadalnix

Differential Revision: https://reviews.bitcoinabc.org/D7031
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Oct 22, 2021
@laanwj @PastaPastaPasta
Merge bitcoin#15457: Check std::system for -[alert|block|wallet]notify
56251ec 
f874e14 [build]: check std::system for -[alert|block|wallet]notify (Sjors Provoost)
cc3ad56 [build] MSVC: set HAVE_SYSTEM for desktop apps (Sjors Provoost)
c1c91bb [build] detect std::system or ::wsystem (Sjors Provoost)

Pull request description:

  Platforms such as iOs and Universal Windows Platform do not support launching a process through system().

ACKs for top commit:
  laanwj:
    code review ACK f874e14

Tree-SHA512: 16bb4a8fa1896046ccb22a46c8985e1aa45f5b11ecf5539eb2299e9a58f1a5b085c0c12cb6939c7493d93abce7e84fadcbfc73374c887db63da6d00c08aa476d

# Conflicts:
#	build_msvc/bitcoin_config.h
#	src/init.cpp
#	src/wallet/init.cpp
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Oct 22, 2021
@fanquake @PastaPastaPasta
Merge bitcoin#16344: build: use #if HAVE_SYSTEM instead of defined(HA…
79e5eb8 
…VE_SYSTEM)

976b034 [build]: use #if HAVE_SYSTEM instead of defined(HAVE_SYSTEM) (Sjors Provoost)

Pull request description:

  It seems that `AC_DEFINE([HAVE_SYSTEM], [HAVE_STD__SYSTEM || HAVE_WSYSTEM]` causes `HAVE_SYSTEM` to always be defined, so we need to use `#if HAVE_SYSTEM` instead of `#if defined(HAVE_SYSTEM)`.

  Followup for bitcoin#15457, can be tested with bitcoin#12557.

ACKs for top commit:
  dongcarl:
    ACK bitcoin@976b034.
  promag:
    ACK 976b034.
  fanquake:
    ACK 976b034

Tree-SHA512: b8cdd04c2ec399fd15638aef5d75ea0886ec1572d3cf4fcea27c193e1e6390344315908262cad8981a9b0a905ab9520619ce2ffe9a717f4ee6bfa8b028ebbdc6

# Conflicts:
#	src/init.cpp
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@laanwj laanwj laanwj left review comments

@fanquake fanquake fanquake left review comments

+2 more reviewers

@Empact Empact Empact left review comments

@promag promag promag left review comments

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
Build system
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.