Skip to content

Conversation

hebasto
Copy link
Member

@hebasto hebasto commented Dec 29, 2019

This PR adds some info to debug.log I found useful for testing (e.g., on Wayland) and debugging issues like #17153:

$ ./src/qt/bitcoin-qt -printtoconsole | head -n 6
2020-01-04T14:57:40Z [main] Bitcoin Core version v0.19.99.0-0df287f4e (release build)
2020-01-04T14:57:40Z [main] InitParameterInteraction: parameter interaction: -externalip set -> setting -discover=0
2020-01-04T14:57:40Z [main] Qt 5.9.5 (dynamic), plugin=xcb (dynamic)
2020-01-04T14:57:40Z [main] System: Linux Mint 19.3, x86_64-little_endian-lp64
2020-01-04T14:57:40Z [main] Screen: HDMI-1 1600x1200, pixel ratio=1.0
2020-01-04T14:57:40Z [main] Assuming ancestors of block 00000000000000b7ab6ce61eb6d571003fbe5fe892da4c9b740c49a07542462d have valid signatures.

@fanquake fanquake added the GUI label Dec 29, 2019
@cvengler
Copy link
Contributor

-host is to confusing in my opinion. I think something like -debuginfo or -sysinfo would be better

@DrahtBot
Copy link
Contributor

DrahtBot commented Dec 29, 2019

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #15768 (gui: Add close window shortcut by IPGlider)

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.

@laanwj
Copy link
Member

laanwj commented Jan 2, 2020

Concept ACK, but yes, -host is confusing. I would expect it to do something with P2P.

-host is to confusing in my opinion. I think something like -debuginfo or -sysinfo would be better

yes, or -guiinfo or -qtinfo, to make it clear this is GUI specific

@hebasto hebasto force-pushed the 20191229-host-info branch from 6981076 to 5ea7c91 Compare January 3, 2020 11:46
@hebasto hebasto changed the title qt: Add -host command-line option qt: Add -qtinfo command-line option Jan 3, 2020
@hebasto
Copy link
Member Author

hebasto commented Jan 3, 2020

@emilengler @laanwj
Thank you for your reviews. Your comments have been addressed.
-qtinfo is chosen as the shortest string.

The OP and the title have been updated.
Added info about Qt version and link type.

@sipa
Copy link
Member

sipa commented Jan 3, 2020

Wouldn't it be better to add this information to -version? That's where I would expect it.

EDIT: hmm, maybe not as it's not just Qt version information but also X stuff.

@hebasto
Copy link
Member Author

hebasto commented Jan 3, 2020

@sipa

Wouldn't it be better to add this information to -version? That's where I would expect it.

-version also contains a huge piece of license info.
-qtinfo is intended to supply some environment info, that seems a bit unappropriated for -version output, no?

@cvengler
Copy link
Contributor

cvengler commented Jan 3, 2020

@sipa

Wouldn't it be better to add this information to -version? That's where I would expect it.

I think help2man would have issues with it and it would be weird on the GNU userland if it would contain information that is not the version and license stuff.

@hebasto
Copy link
Member Author

hebasto commented Jan 3, 2020

I think help2man would have issues with it...

help2man relies on gen-manpages.sh wrapper which in turn relies on bitcoind -version.

@sipa
Copy link
Member

sipa commented Jan 4, 2020

@hebasto You're right, the environment information does not belong in -version.

@hebasto
Copy link
Member Author

hebasto commented Jan 4, 2020

@sipa

@hebasto You're right, the environment information does not belong in -version.

Does it mean "Concept ACK"? 😄

@cvengler
Copy link
Contributor

cvengler commented Jan 4, 2020

Concept ACK

@laanwj
Copy link
Member

laanwj commented Jan 4, 2020

Wouldn't it be better to add this information to -version? That's where I would expect it.

I personally prefer -version to be a short message that just states the version. And some license info. I think it's currently fine. I wouldn't expect all kind of window system information there.

Thinking of it, this is for troubleshooting right? Let's just always log this in a message to the debug log instead of adding another step?

@sipa
Copy link
Member

sipa commented Jan 4, 2020

Concept ACK

@hebasto hebasto force-pushed the 20191229-host-info branch from 5ea7c91 to 0df287f Compare January 4, 2020 14:58
@hebasto
Copy link
Member Author

hebasto commented Jan 4, 2020

@laanwj

Thinking of it, this is for troubleshooting right?

Yes, it is.

Let's just always log this in a message to the debug log instead of adding another step?

Done in the latest push.

The OP has been updated.

@hebasto hebasto changed the title qt: Add -qtinfo command-line option qt: Log Qt related info Jan 4, 2020
@@ -879,4 +884,22 @@ int TextWidth(const QFontMetrics& fm, const QString& text)
#endif
}

void LogQtInfo()
{
#ifdef QT_STATIC
Copy link
Contributor

Choose a reason for hiding this comment

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

nit, do

#if defined(QT_STATIC) || defined(QT_STATICPLUGIN)

and remove the two conditions below

Copy link
Contributor

Choose a reason for hiding this comment

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

qt_link and plugin_link are orthogonal.

#else
const std::string qt_link{"dynamic"};
#endif
#ifdef QT_STATICPLUGIN
Copy link
Contributor

Choose a reason for hiding this comment

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

^^

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.

Code review ACK 0df287f. I also prefer laanwj suggestion.

@@ -879,4 +884,22 @@ int TextWidth(const QFontMetrics& fm, const QString& text)
#endif
}

void LogQtInfo()
{
#ifdef QT_STATIC
Copy link
Contributor

Choose a reason for hiding this comment

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

qt_link and plugin_link are orthogonal.

@laanwj
Copy link
Member

laanwj commented Jan 4, 2020

FWIW it doesn't seem like it detects multiple screens that form one display area.
output:

2020-01-04T20:58:32Z Qt 5.9.5 (dynamic), plugin=xcb (dynamic)
2020-01-04T20:58:32Z System: Ubuntu 18.04.3 LTS, x86_64-little_endian-lp64
2020-01-04T20:58:32Z Screen: DVI-I-1 1920x1200, pixel ratio=1.0

xrandr output:

Screen 0: minimum 320 x 200, current 3520 x 1200, maximum 16384 x 16384
DVI-I-1 connected primary 1920x1200+1600+0 (normal left inverted right x axis y axis) 519mm x 324mm
DVI-I-2 connected 1600x900+0+0 (normal left inverted right x axis y axis) 442mm x 249mm

@hebasto hebasto force-pushed the 20191229-host-info branch from 0df287f to 092d146 Compare January 5, 2020 08:12
@hebasto
Copy link
Member Author

hebasto commented Jan 5, 2020

@laanwj

FWIW it doesn't seem like it detects multiple screens that form one display area.

Now it detects all the screens associated with the windowing system the application is connected to.
Please test.

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.

ACK 092d146, now outputs all screens details.

LogPrintf("Qt %s (%s), plugin=%s (%s)\n", qVersion(), qt_link, QGuiApplication::platformName().toStdString(), plugin_link);
LogPrintf("System: %s, %s\n", QSysInfo::prettyProductName().toStdString(), QSysInfo::buildAbi().toStdString());
const QList<QScreen*> screens = QGuiApplication::screens();
for (int i = 0; i < screens.size(); ++i) {
Copy link
Contributor

Choose a reason for hiding this comment

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

for (const QScreen* s : QGuiApplication::screens())?

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

It's fine here, these are pointers.

Copy link
Member Author

Choose a reason for hiding this comment

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

Implemented in the latest push.

@hebasto hebasto force-pushed the 20191229-host-info branch from 092d146 to a004673 Compare January 5, 2020 21:41
@laanwj
Copy link
Member

laanwj commented Jan 8, 2020

Thanks, works now!

2020-01-08T13:20:54Z Qt 5.9.5 (dynamic), plugin=xcb (dynamic)
2020-01-08T13:20:54Z System: Ubuntu 18.04.3 LTS, x86_64-little_endian-lp64
2020-01-08T13:20:54Z Screen: DVI-I-1 1920x1200, pixel ratio=1.0
2020-01-08T13:20:54Z Screen: DVI-I-2 1600x900, pixel ratio=1.0

ACK a004673

laanwj added a commit that referenced this pull request Jan 8, 2020
a004673 qt: Add LogQtInfo() function (Hennadii Stepanov)

Pull request description:

  This PR adds some info to `debug.log` I found useful for testing (e.g., on Wayland) and debugging issues like #17153:

  ```
  $ ./src/qt/bitcoin-qt -printtoconsole | head -n 6
  2020-01-04T14:57:40Z [main] Bitcoin Core version v0.19.99.0-0df287f4e (release build)
  2020-01-04T14:57:40Z [main] InitParameterInteraction: parameter interaction: -externalip set -> setting -discover=0
  2020-01-04T14:57:40Z [main] Qt 5.9.5 (dynamic), plugin=xcb (dynamic)
  2020-01-04T14:57:40Z [main] System: Linux Mint 19.3, x86_64-little_endian-lp64
  2020-01-04T14:57:40Z [main] Screen: HDMI-1 1600x1200, pixel ratio=1.0
  2020-01-04T14:57:40Z [main] Assuming ancestors of block 00000000000000b7ab6ce61eb6d571003fbe5fe892da4c9b740c49a07542462d have valid signatures.
  ```

ACKs for top commit:
  laanwj:
    ACK a004673

Tree-SHA512: 496bcfd4870a2730eab92b96b3e74989a7904b21369c372b6d4368f4ca2c141e2fdc1348a1fdd18cb68bb144dcea01d3023bb782f9d030e330c187f6a5a1a082
@laanwj laanwj merged commit a004673 into bitcoin:master Jan 8, 2020
@hebasto hebasto deleted the 20191229-host-info branch January 8, 2020 14:46
MarkLTZ added a commit to litecoinz-core/litecoinz that referenced this pull request Jan 12, 2020
deadalnix pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Nov 14, 2020
Summary:
Add information to debug log about Qt, the OS, the screens.

This is a backport of Core [[bitcoin/bitcoin#17826 | PR17826]]

Test Plan:
```
$ ninja && src/qt/bitcoin-qt -regtest -printtoconsole  | head -n6
[219/219] Linking CXX executable src/qt/bitcoin-qt
2020-11-13T15:28:52Z Bitcoin ABC version v0.22.7-3ba582bc3 (release build)
2020-11-13T15:28:52Z Qt 5.12.8 (dynamic), plugin=xcb (dynamic)
2020-11-13T15:28:52Z System: Ubuntu 20.04.1 LTS, x86_64-little_endian-lp64
2020-11-13T15:28:52Z Screen: DVI-I-1 1920x1080, pixel ratio=1.0
2020-11-13T15:28:52Z Screen: DVI-D-1 1920x1080, pixel ratio=1.0
2020-11-13T15:28:53Z Checkpoints will be verified.
```

Reviewers: #bitcoin_abc, deadalnix

Reviewed By: #bitcoin_abc, deadalnix

Differential Revision: https://reviews.bitcoinabc.org/D8393
jonasschnelli added a commit to bitcoin-core/gui that referenced this pull request Jan 28, 2021
957895c util: Log static plugins meta data and style (Hennadii Stepanov)

Pull request description:

  This PR is a follow-up of bitcoin/bitcoin#17826, and adds additional info about the imported static plugins and the used style to the `debug.log` I found useful for testing (e.g., with `QT_QPA_PLATFORM`, `QT_QPA_PLATFORMTHEME`, `QT_STYLE_OVERRIDE` variables) and debugging issues (e.g., bitcoin/bitcoin#19716 (comment)).

  The excerpt from the log:
  ```
  2020-11-15T18:41:45Z [main] Bitcoin Core version v0.20.99.0-f0b933f78 (release build)
  2020-11-15T18:41:45Z [main] Qt 5.9.8 (static), plugin=xcb (static)
  2020-11-15T18:41:45Z [main] Static plugins:
  2020-11-15T18:41:45Z [main]  QXcbIntegrationPlugin, version 329992
  2020-11-15T18:41:45Z [main] Style: fusion / QFusionStyle
  ...
  ```

ACKs for top commit:
  jarolrod:
    ACK 957895c, Tested on macOS 11.1
  jonasschnelli:
    utACK 957895c

Tree-SHA512: 0e46db7560f380fbda8ce5e53faa5d419a456e90ca595ce46be8e3030c99d3a113586edad1988a97e9bf0279e944f975968ed1156817bc16723ed31c64850239
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Jan 28, 2021
…d style

957895c util: Log static plugins meta data and style (Hennadii Stepanov)

Pull request description:

  This PR is a follow-up of bitcoin#17826, and adds additional info about the imported static plugins and the used style to the `debug.log` I found useful for testing (e.g., with `QT_QPA_PLATFORM`, `QT_QPA_PLATFORMTHEME`, `QT_STYLE_OVERRIDE` variables) and debugging issues (e.g., bitcoin#19716 (comment)).

  The excerpt from the log:
  ```
  2020-11-15T18:41:45Z [main] Bitcoin Core version v0.20.99.0-f0b933f78 (release build)
  2020-11-15T18:41:45Z [main] Qt 5.9.8 (static), plugin=xcb (static)
  2020-11-15T18:41:45Z [main] Static plugins:
  2020-11-15T18:41:45Z [main]  QXcbIntegrationPlugin, version 329992
  2020-11-15T18:41:45Z [main] Style: fusion / QFusionStyle
  ...
  ```

ACKs for top commit:
  jarolrod:
    ACK 957895c, Tested on macOS 11.1
  jonasschnelli:
    utACK 957895c

Tree-SHA512: 0e46db7560f380fbda8ce5e53faa5d419a456e90ca595ce46be8e3030c99d3a113586edad1988a97e9bf0279e944f975968ed1156817bc16723ed31c64850239
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants