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

pull Sjors wants to merge 3 commits into bitcoin:master from Sjors:2019/02/std__system changing 8 files +51 −0
  1. Sjors commented at 1:15 pm on February 21, 2019: member
    Platforms such as iOs and Universal Windows Platform do not support launching a process through system().
  2. fanquake added the label Build system on Feb 21, 2019
  3. Sjors commented at 1:17 pm on February 21, 2019: member

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

     0make clean
 1
 2./configure
 3...
 4checking for std::system... yes
 5...
 6
 7make clean
 8
 9./configure --prefix=`pwd`/depends/aarch64-apple-darwin14
10...
11checking for std::system... no
12...

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

  4. promag commented at 1:52 pm on February 21, 2019: member
    Concept ACK. Could make runCommand noop instead of multiple guards?
  5. MarcoFalke commented at 2:26 pm on February 21, 2019: member

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

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

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

  6. DrahtBot commented at 2:36 pm on February 21, 2019: member

    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.

  7. promag commented at 2:41 pm on February 21, 2019: member
    @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.
  8. ken2812221 commented at 3:06 pm on February 21, 2019: contributor
    Concept ACK. Maybe those arguments should be added into hidden_args to keep config file portability.
  9. MarcoFalke commented at 3:09 pm on February 21, 2019: member
    • 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)
  10. promag commented at 3:22 pm on February 21, 2019: member
    @MarcoFalke agree :trollface: I was curious how it currently detects if wallet is enabled.
  11. Sjors commented at 5:00 pm on February 21, 2019: member

    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.

  12. promag commented at 0:31 am on February 22, 2019: member

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

    :+1:

  13. jonasschnelli commented at 6:08 am on February 22, 2019: contributor
    Concept ACK
  14. laanwj commented at 8:28 am on February 22, 2019: member
    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
  15. in configure.ac:888 in c0e9ac7ac1 outdated 
    883@@ -884,6 +884,15 @@ if test x$use_reduce_exports = xyes; then
884   [AC_MSG_ERROR([Cannot set default symbol visibility. Use --disable-reduce-exports.])])
885 fi
886 
887+AC_MSG_CHECKING([for std::system])
888+AC_TRY_LINK( [ #include <cstdlib> ],
    fanquake commented at 8:40 am on February 22, 2019:

    I see these warnings when running ./autogen.sh:

    0configure.ac:894: warning: The macro `AC_TRY_LINK' is obsolete.
1configure.ac:894: You should run autoupdate.
2../../lib/autoconf/general.m4:2688: AC_TRY_LINK is expanded from...
3configure.ac:894: the top level
4glibtoolize: putting auxiliary files in AC_CONFIG_AUX_DIR, 'build-aux'.
5...
    Sjors commented at 10:57 am on February 22, 2019:
    I switched to the modern AC_LINK_IFELSE( [ AC_LANG_PROGRAM( syntax. For some reason I don’t think I got these warnings on macOS.
  16. Sjors force-pushed on Feb 22, 2019
  17. Sjors force-pushed on Feb 22, 2019
  18. Sjors force-pushed on Feb 22, 2019
  19. MarcoFalke added the label Needs gitian build on Feb 22, 2019
  20. DrahtBot commented at 8:06 am on February 23, 2019: member

    Gitian builds for commit a094b543324267e6d956bb0df2ede990b90118cb (master):

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

  21. DrahtBot removed the label Needs gitian build on Feb 23, 2019
  22. in src/init.cpp:583 in d9255a1b5e outdated 
    579@@ -576,6 +580,7 @@ std::string LicenseInfo()
580            "\n";
581 }
582 
583+#if defined(HAVE_STD__SYSTEM) || defined(WIN32)
    laanwj commented at 2:38 pm on March 8, 2019:
    I think repeating this condition everywhere is error-prone. Can’t we make sure HAVE_STD__SYSTEM is always set when WIN32 instead?
    Sjors commented at 5:13 pm on March 8, 2019:
    I changed it to check for either std::system or ::wsystem, so we can now use HAVE_SYSTEM.
  23. Sjors force-pushed on Mar 8, 2019
  24. in src/util/system.h:18 in de0e7298d9 outdated 
    13@@ -14,6 +14,9 @@
14 #include <config/bitcoin-config.h>
15 #endif
16 
17+/* Define to 1 if std::system or (for Windows) ::wsystem is available */
18+#define HAVE_SYSTEM HAVE_STD__SYSTEM || HAVE_WSYSTEM
    Empact commented at 6:55 pm on March 9, 2019:
    Should this be happening in configure alone, or here alone?
    Sjors commented at 8:21 pm on March 9, 2019:
    I’ve kept HAVE_STD__SYSTEM and HAVE_WSYSTEM separate, because it allows for separate handling of Windows vs. non-Windows. HAVE_SYSTEM combines the two, for code that doesn’t care.
    Empact commented at 8:27 pm on March 9, 2019:
    All for keeping them separate. I’m more calling out that the HAVE_SYSTEM definition seems to be happening in configure.ac as well: AC_DEFINE([HAVE_SYSTEM], [HAVE_STD__SYSTEM || HAVE_WSYSTEM], [HAVE_WSYSTEM]), and that it’s preferable to have one or the other to avoid potential divergence / confusion.
    Sjors commented at 9:47 pm on March 9, 2019:
    Ah oops, I’ll get rid the duplicate.
  25. in configure.ac:908 in de0e7298d9 outdated 
    903+    [ AC_MSG_RESULT(yes); AC_DEFINE(HAVE_WSYSTEM, 1, Define to 1 if you have the `::wsystem' function.)],
904+    [ AC_MSG_RESULT(no) ]
905+)
906+
907+# Define to 1 if std::system or ::wsystem (Windows) is available
908+AC_DEFINE([HAVE_SYSTEM], [HAVE_STD__SYSTEM || HAVE_WSYSTEM], [HAVE_WSYSTEM])
    Empact commented at 8:31 pm on March 9, 2019:
    HAVE_WSYSTEM seems unintended as a “description” argument. https://www.gnu.org/software/autoconf/manual/autoconf-2.69/html_node/Defining-Symbols.html
  26. Sjors force-pushed on Mar 14, 2019
  27. Sjors force-pushed on Mar 14, 2019
  28. Sjors force-pushed on Mar 14, 2019
  29. Sjors commented at 5:36 pm on March 14, 2019: member

    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.

  30. Sjors commented at 6:08 pm on March 14, 2019: member
    @sipsorcery do you know how to run the functional tests on Windows?
  32. sipsorcery commented at 6:42 pm on March 14, 2019: member

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

  33. Sjors commented at 8:11 pm on March 14, 2019: member

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

     0[environment]
 1SRCDIR=C:\Users\sjors\bitcoin
 2BUILDDIR=C:\Users\sjors\bitcoin
 3EXEEXT=.exe
 4RPCAUTH=C:\Users\sjors\bitcoin\share\rpcauth\rpcauth.py
 5
 6[components]
 7# Which components are enabled. These are commented out by `configure` if they were disabled when running config.
 8ENABLE_WALLET=true
 9ENABLE_CLI=true
10ENABLE_BITCOIND=true
11ENABLE_ZMQ=true�

    And you have copy the exe files:

    0mv .\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'�.

  34. sipsorcery commented at 8:17 pm on March 14, 2019: member

    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

  35. Sjors commented at 8:17 pm on March 14, 2019: member

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

  36. Sjors commented at 8:23 pm on March 14, 2019: member

    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?).

  37. Sjors commented at 10:15 am on March 15, 2019: member

    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.

  38. Sjors force-pushed on Mar 15, 2019
  39. practicalswift commented at 8:57 am on March 21, 2019: contributor
    FWIW the disassembly of the bitcoind binary is identical before and after applying this patch on my Ubuntu 18.04 system (as expected).
  40. DrahtBot added the label Needs rebase on May 13, 2019
  41. [build] detect std::system or ::wsystem c1c91bb78d
  42. [build] MSVC: set HAVE_SYSTEM for desktop apps cc3ad56ff2
  43. [build]: check std::system for -[alert|block|wallet]notify 
    Platforms such as iOs do not support launching a process
through system().
    f874e14cd3
  44. Sjors force-pushed on Jun 6, 2019
  45. DrahtBot removed the label Needs rebase on Jun 6, 2019
  46. laanwj commented at 3:29 pm on July 5, 2019: member
    code review ACK f874e14cd3c84cd412bd3fb42b3ee1706ca6a267
  47. laanwj merged this on Jul 5, 2019
  48. laanwj closed this on Jul 5, 2019
  49. laanwj referenced this in commit 8c69fae944 on Jul 5, 2019
  50. Sjors deleted the branch on Jul 5, 2019
  51. Sjors commented at 4:39 pm on July 5, 2019: member
    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.
  52. sidhujag referenced this in commit a851e931b9 on Jul 6, 2019
  53. fanquake referenced this in commit f373beebbc on Jul 7, 2019
  54. jasonbcox referenced this in commit 8e243c9eb5 on Aug 4, 2020
  55. PastaPastaPasta referenced this in commit 56251ec35d on Oct 22, 2021
  56. PastaPastaPasta referenced this in commit 79e5eb8a63 on Oct 22, 2021
  57. DrahtBot locked this on Dec 16, 2021


Sjors promag MarcoFalke DrahtBot ken2812221 jonasschnelli laanwj fanquake Empact sipsorcery practicalswift

Labels
Build system

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-11-17 15:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me