init: Use InitError for all errors in bitcoind/qt by maflcko · Pull Request #16366 · bitcoin/bitcoin

maflcko · 2019-07-10T13:25:47Z

Using the same InitError for startup error in the daemon and the gui makes it possible to run the tests with the gui again:

BITCOIND=bitcoin-qt ./test/functional/test_runner.py feature_includeconf feature_config_args

DrahtBot · 2019-07-10T15:59:32Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#16362 (gui: Bilingual translation by hebasto)
#16273 (refactor: Remove unused includes by practicalswift)
#16224 (gui: Bilingual GUI error messages by hebasto)
#15864 (Fix datadir handling by hebasto)
#14866 (Improve property evaluation way in bitcoin.conf by AkioNak)
#13389 (Utils and libraries: Fix -sysperms=false (default) doesn't appear to do anything #13371 - move umask operation earlier in AppInit() by n2yen)
#10102 ([experimental] Multiprocess bitcoin by ryanofsky)

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.

ryanofsky

utACK fa6647a. I wonder if it'd be possible to run tests with BITCOIND=bitcoin-qt QT_QPA_PLATFORM=minimal on travis to catch more regressions.

hebasto · 2019-07-10T16:49:46Z

Concept ACK.

Usually, running self-compiled bitcoin-qt on Linux Mint 19.1 I get a message:

$ ./src/qt/bitcoin-qt
qt5ct: using qt5ct plugin

So local run tests on my environment returns errors:

$ BITCOIND=bitcoin-qt ./test/functional/test_runner.py feature_includeconf feature_config_args
Temporary test directory at /tmp/test_runner_₿_🏃_20190710_194358
Traceback (most recent call last):
  File "/home/hebasto/github/bitcoin/test/functional/create_cache.py", line 28, in <module>
    CreateCache().main()
  File "/home/hebasto/github/bitcoin/test/functional/test_framework/test_framework.py", line 218, in main
    self.stop_nodes()
  File "/home/hebasto/github/bitcoin/test/functional/test_framework/test_framework.py", line 397, in stop_nodes
    node.stop_node(wait=wait)
  File "/home/hebasto/github/bitcoin/test/functional/test_framework/test_node.py", line 276, in stop_node
    raise AssertionError("Unexpected stderr {} != {}".format(stderr, expected_stderr))
AssertionError: Unexpected stderr qt5ct: using qt5ct plugin != 
2019-07-10T16:43:58.707000Z TestFramework (INFO): Initializing test directory /tmp/test_runner_₿_🏃_20190710_194358/cache
2019-07-10T16:43:59.616000Z TestFramework (ERROR): Assertion failed
Traceback (most recent call last):
  File "/home/hebasto/github/bitcoin/test/functional/test_framework/test_framework.py", line 191, in main
    self.setup_chain()
  File "/home/hebasto/github/bitcoin/test/functional/test_framework/test_framework.py", line 274, in setup_chain
    self._initialize_chain()
  File "/home/hebasto/github/bitcoin/test/functional/test_framework/test_framework.py", line 514, in _initialize_chain
    self.stop_nodes()
  File "/home/hebasto/github/bitcoin/test/functional/test_framework/test_framework.py", line 397, in stop_nodes
    node.stop_node(wait=wait)
  File "/home/hebasto/github/bitcoin/test/functional/test_framework/test_node.py", line 276, in stop_node
    raise AssertionError("Unexpected stderr {} != {}".format(stderr, expected_stderr))
AssertionError: Unexpected stderr qt5ct: using qt5ct plugin != 
2019-07-10T16:43:59.667000Z TestFramework (INFO): Stopping nodes
[node 0] Cleaning up leftover process
Traceback (most recent call last):
  File "./test/functional/test_runner.py", line 661, in <module>
    main()
  File "./test/functional/test_runner.py", line 324, in main
    runs_ci=args.ci,
  File "./test/functional/test_runner.py", line 356, in run_tests
    subprocess.check_output([sys.executable, tests_dir + 'create_cache.py'] + flags + ["--tmpdir=%s/cache" % tmpdir])
  File "/usr/lib/python3.6/subprocess.py", line 356, in check_output
    **kwargs).stdout
  File "/usr/lib/python3.6/subprocess.py", line 438, in run
    output=stdout, stderr=stderr)
subprocess.CalledProcessError: Command '['/usr/bin/python3', '/home/hebasto/github/bitcoin/test/functional/create_cache.py', '--cachedir=/home/hebasto/github/bitcoin/test/cache', '--configfile=/home/hebasto/github/bitcoin/test/functional/../config.ini', '--tmpdir=/tmp/test_runner_\u20bf_🏃_20190710_194358/cache']' returned non-zero exit status 1.

src/bitcoind.cpp

laanwj · 2019-07-11T15:30:46Z

InitError was supposed to only be called from the AppInit* functions, to make sure it isn't called before the appropriate listeners are installed. It seems I lost track of how the initialization process works though.

ryanofsky · 2019-07-11T15:41:05Z

to make sure it isn't called before the appropriate listeners are installed

@laanwj what are "listeners" here? I don't understand what this could be referring to.

laanwj · 2019-07-11T15:54:24Z

@laanwj what are "listeners" here? I don't understand what this could be referring to.

uiInterface signal listeners etc

maflcko · 2019-07-11T16:06:54Z

In bitcoind.cpp, I only modify the AppInit function, so the change is obviously correct there.

However, the gui uses a self-brewed AppInit*, I think. One of the first things in GuiMain is to // Subscribe to global signals from core. So it should be fine as well.

I'd presume it would be more difficult to rewrite the Gui init to call the same AppInit function as bitcoind does, so I'd rather not do that.

ryanofsky

ACK fa6647a again. No changes since last review, I just dug into this a little more. I have some suggestions and questions below, but they are not too important, so feel free to ignore them.

src/qt/bitcoin.cpp

Also, remove unused <boost/thread.hpp> include (and others)

Avoids GUI code calling a node function, and having to live in the same process as g_ui_signals and uiInterface global variables.

hebasto · 2019-07-13T12:10:47Z

ACK fa6f402

Tested on Linux Mint 19.1 with system Qt 5.9.5.

The "using qt5ct plugin" message that is printed to stderr by qt5ct can be suppressed by the following environment variable:

QT_LOGGING_RULES="qt5ct.debug=false"

Using this knowledge, feature_includeconf.py passed:

$ BITCOIND=bitcoin-qt QT_LOGGING_RULES="qt5ct.debug=false" ./test/functional/test_runner.py feature_includeconf
Temporary test directory at /tmp/test_runner_₿_🏃_20190713_150236
Remaining jobs: [feature_includeconf.py]
1/1 - feature_includeconf.py passed, Duration: 10 s

TEST                   | STATUS    | DURATION

feature_includeconf.py | ✓ Passed  | 10 s

ALL                    | ✓ Passed  | 10 s (accumulated) 
Runtime: 10 s

And feature_config_args.py passed too:

$ BITCOIND=bitcoin-qt QT_LOGGING_RULES="qt5ct.debug=false" ./test/functional/test_runner.py feature_config_args
Temporary test directory at /tmp/test_runner_₿_🏃_20190713_150609
Remaining jobs: [feature_config_args.py]
1/1 - feature_config_args.py passed, Duration: 17 s

TEST                   | STATUS    | DURATION

feature_config_args.py | ✓ Passed  | 17 s

ALL                    | ✓ Passed  | 17 s (accumulated) 
Runtime: 17 s

@MarcoFalke
Could you add QT_LOGGING_RULES="qt5ct.debug=false" to the OP?

maflcko · 2019-07-15T13:24:59Z

Could you add QT_LOGGING_RULES="qt5ct.debug=false" to the OP?

This is specific to your environment. If I add that, I would also have to add XDG_SESSION_TYPE=x11, because someone might be running on Wayland with Gnome and get the Warning: Ignoring XDG_SESSION_TYPE=wayland on Gnome.)

ryanofsky

utACK fa6f402. Only changes since last review are removing more includes and adding Node::initError method to avoid accessing node InitError function and global variables from GUI code.

maflcko · 2019-07-22T12:25:43Z

This pull will be merged some time this week, unless there are objections.

maflcko · 2019-07-23T22:40:21Z

Going to merge this, so that the tests pass with the gui

fa6f402 Call node->initError instead of InitError from GUI code (Russell Yanofsky) fad2502 init: Use InitError for all errors in bitcoind/qt (MarcoFalke) Pull request description: Using the same InitError for startup error in the daemon and the gui makes it possible to run the tests with the gui again: ```sh BITCOIND=bitcoin-qt ./test/functional/test_runner.py feature_includeconf feature_config_args ACKs for top commit: hebasto: ACK fa6f402 ryanofsky: utACK fa6f402. Only changes since last review are removing more includes and adding Node::initError method to avoid accessing node `InitError` function and global variables from GUI code. Tree-SHA512: bd19e08dcea4019dfe40356bc5c63cb583cefed54b6c9dcfb82f1b5b00308d8e2b363549afcaea5e93bf83864dbe0917400c3b70f43a8a5bdff45c9cd34cc294

Summary: **Pull Request#16366** fa6f402bde146f92ed131e0c9c8e15a55e723307 Call node->initError instead of InitError from GUI code (Russell Yanofsky) fad2502240a1c440ef03ac3f880475702e418275 init: Use InitError for all errors in bitcoind/qt (MarcoFalke) Description: Using the same InitError for startup error in the daemon and the gui makes it possible to run the tests with the gui again: (N.f.B: Depends on D5794) ```sh BITCOIND=bitcoin-qt ./test/functional/test_runner.py feature_includeconf feature_config_args ``` --- **Pull Request#15864** ffea41f5301d5582665cf10ba5c2b9547a1443de Enable all tests in feature_config_args.py (Hennadii Stepanov) Description: Fix broken `feature_config_args.py` tests (disabled by MarcoFalke@fabe28a0cdcfa13e0e595a0905e3642a960d3077). All test are enabled now. --- This is a backport of Core [[bitcoin/bitcoin#16366 | PR16366]] with a partial backport of core [[bitcoin/bitcoin#15864 | PR15864]] Test Plan: ninja check check-functional Reviewers: deadalnix, #bitcoin_abc Reviewed By: deadalnix, #bitcoin_abc Differential Revision: https://reviews.bitcoinabc.org/D5807

fa6f402 Call node->initError instead of InitError from GUI code (Russell Yanofsky) fad2502 init: Use InitError for all errors in bitcoind/qt (MarcoFalke) Pull request description: Using the same InitError for startup error in the daemon and the gui makes it possible to run the tests with the gui again: ```sh BITCOIND=bitcoin-qt ./test/functional/test_runner.py feature_includeconf feature_config_args ACKs for top commit: hebasto: ACK fa6f402 ryanofsky: utACK fa6f402. Only changes since last review are removing more includes and adding Node::initError method to avoid accessing node `InitError` function and global variables from GUI code. Tree-SHA512: bd19e08dcea4019dfe40356bc5c63cb583cefed54b6c9dcfb82f1b5b00308d8e2b363549afcaea5e93bf83864dbe0917400c3b70f43a8a5bdff45c9cd34cc294

maflcko force-pushed the 1907-initErrorGui branch 2 times, most recently from 22229f3 to fa6647a Compare July 10, 2019 13:55

DrahtBot added GUI Tests labels Jul 10, 2019

ryanofsky approved these changes Jul 10, 2019

View reviewed changes

hebasto reviewed Jul 10, 2019

View reviewed changes

src/bitcoind.cpp Show resolved Hide resolved

laanwj reviewed Jul 11, 2019

View reviewed changes

src/bitcoind.cpp Show resolved Hide resolved

ryanofsky approved these changes Jul 11, 2019

View reviewed changes

src/qt/bitcoin.cpp Show resolved Hide resolved

src/qt/bitcoin.cpp Outdated Show resolved Hide resolved

src/qt/bitcoin.cpp Show resolved Hide resolved

MarcoFalke and others added 2 commits July 11, 2019 19:39

init: Use InitError for all errors in bitcoind/qt

fad2502

Also, remove unused <boost/thread.hpp> include (and others)

Call node->initError instead of InitError from GUI code

fa6f402

Avoids GUI code calling a node function, and having to live in the same process as g_ui_signals and uiInterface global variables.

maflcko force-pushed the 1907-initErrorGui branch from fa6647a to fa6f402 Compare July 11, 2019 23:40

maflcko requested a review from laanwj July 15, 2019 13:22

maflcko added this to the 0.19.0 milestone Jul 15, 2019

ryanofsky approved these changes Jul 15, 2019

View reviewed changes

maflcko merged commit fa6f402 into bitcoin:master Jul 23, 2019

maflcko deleted the 1907-initErrorGui branch July 23, 2019 22:48

This was referenced Jul 24, 2019

Refactor: Add Flags enum to ArgsManager class #16097

Merged

gui: Bilingual GUI error messages #16224

Merged

jtimon mentioned this pull request Aug 8, 2019

Testchains: Introduce custom chain whose constructor... #8994

Closed

12 tasks

hebasto mentioned this pull request Jan 21, 2021

make check failure #20869

Closed

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

Conversation

maflcko commented Jul 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Jul 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

hebasto commented Jul 10, 2019

Uh oh!

Uh oh!

Uh oh!

laanwj commented Jul 11, 2019

Uh oh!

ryanofsky commented Jul 11, 2019

Uh oh!

laanwj commented Jul 11, 2019

Uh oh!

maflcko commented Jul 11, 2019

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

hebasto commented Jul 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maflcko commented Jul 15, 2019

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko commented Jul 22, 2019

Uh oh!

maflcko commented Jul 23, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

maflcko commented Jul 10, 2019 •

edited

Loading

DrahtBot commented Jul 10, 2019 •

edited

Loading

hebasto commented Jul 13, 2019 •

edited

Loading