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

gui: Also log and print messages or questions like bitcoind #14162

Merged
merged 2 commits into from Sep 11, 2018

Conversation

Projects
None yet
4 participants
@MarcoFalke
Copy link
Member

commented Sep 6, 2018

Testing and debugging after shutdown are harder if the node was run through the gui, because errors and warnings would not be logged to the debug.log or written to the stderr (as is the case for bitcoind).

MarcoFalke added some commits Sep 6, 2018

@MarcoFalke MarcoFalke added the GUI label Sep 6, 2018

@promag

This comment has been minimized.

Copy link
Member

commented Sep 7, 2018

utACK fa7e969.

I guess calling handleMessageBox and friends is not worth it.

BTW noui.cpp is in libbitcoin_server which is shared between bitcoind and bitcoin-qt — initially thought noui.cpp wasn't included in bitcoin-qt.

@jonasschnelli

This comment has been minimized.

Copy link
Member

commented Sep 7, 2018

utACK fa7e969

@laanwj

This comment has been minimized.

Copy link
Member

commented Sep 11, 2018

I'm fairly sure that this used to be the behavior, the noui handlers were always registered.

But it regressed when moving away from boost::signals in #13961, or earlier, which dropped support for having multiple handlers.

utACK fa7e969 anyhow.

Edit: checked that this does not need backport to 0.17.

@laanwj laanwj merged commit fa7e969 into bitcoin:master Sep 11, 2018

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 Sep 11, 2018

Merge #14162: gui: Also log and print messages or questions like bitc…
…oind

fa7e969 qt: Also log and print messages or questions like bitcoind (MarcoFalke)
dd031e3 noui: Move handlers to header file (MarcoFalke)

Pull request description:

  Testing and debugging after shutdown are harder if the node was run through the gui, because errors and warnings would not be logged to the debug.log or written to the stderr (as is the case for bitcoind).

Tree-SHA512: 1154e2bf02e3c2616c8d28609569d6c3c7344c5877ad5c1303245044cc7aced9eaec9627f1e1258ed087b49c2a2e6f99bc6c1ad0abe0a855b61e737bdf2059bc

@laanwj laanwj added the Bug label Sep 11, 2018

@MarcoFalke MarcoFalke deleted the MarcoFalke:Mf1808-uiSignals2 branch Sep 11, 2018

@MarcoFalke

This comment has been minimized.

Copy link
Member Author

commented Sep 11, 2018

@laanwj I am pretty sure this has always been the case.

Also, it exists on 0.17:

$ ./src/qt/bitcoin-qt -regtest -walletdir=/tmp/aaaab
$

master:

$ ./src/qt/bitcoin-qt -regtest -walletdir=/tmp/aaaab
Error: Specified -walletdir "/tmp/aaaab" does not exist
$
@laanwj

This comment has been minimized.

Copy link
Member

commented Sep 11, 2018

seems you're right, i've checked back as far as 0.10.0 and there's no noui_connect in the GUI

must have imagined it then...

@laanwj

This comment has been minimized.

Copy link
Member

commented Sep 11, 2018

I think I was confused with InitMessage, both qt and the noui implementation have (before this PR):

static void InitMessage(const std::string &message)
{
    LogPrintf("init message: %s\n", message);
}

so they were logged for both the GUI and with bitcoind

the init window also subscribes to this handler, but that's no problem, as multiple handlers for these signals are still supported (in contrast to what I thought)

@MarcoFalke

This comment has been minimized.

Copy link
Member Author

commented Sep 11, 2018

Indeed, init messages are both handled. Though, not the errors, warnings and questions.

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.