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

Enable QML debugging and profiling in debug builds #3999

Merged
merged 1 commit into from
Jun 21, 2021

Conversation

Swiftb0y
Copy link
Member

Allows builds to be profiled and debugged by external applications
such as VS and Qt Creator.
See https://doc.qt.io/qt-5/qtquick-debugging.html

@github-actions github-actions bot added the build label Jun 15, 2021
@Swiftb0y
Copy link
Member Author

The doc says that enabling QML debugging is a security risk so we might want to reflect that in the build output.

@daschuer
Copy link
Member

What is on a risk? It is probably the same when adding debug information to the binary.
We ship them along with our release.

But anyway, how about adding a cmake status message, that mentions the risk.

QML debugging is enabled. Only use this in a safe environment.

This is printed a qDebug in 5.12 and from 5.13 to street.

@Swiftb0y
Copy link
Member Author

What is on a risk? It is probably the same when adding debug information to the binary.

I'm not sure. AFAIK you can puppeteer the entire application through an unprotected TCP socket. So you can change everything in the QML engine from the outside. So I guess the risk is comparable to being able to attach a gdb to a remote process?

But anyway, how about adding a cmake status message, that mentions the risk.

Yes, thats what I'm advocating for.

QML debugging is enabled. Only use this in a safe environment.

This is printed a qDebug in 5.12 and from 5.13 to street.

I fear this is not enough. Its a single line behind lots of other log messages and maintainers might only compile the application and won't get to see it running. So I'd like to add a message during compile time that mentions the risk there as well.

@daschuer
Copy link
Member

Ah ok, this opens a network port, that's the risk.

@daschuer
Copy link
Member

Yes, that makes sense. The message should mention the network port.

Allows builds to be profiled and debugged by external applications
such as VS and Qt Creator.
See https://doc.qt.io/qt-5/qtquick-debugging.html
@Swiftb0y
Copy link
Member Author

Included a message. What do you think?

Copy link
Member

@daschuer daschuer left a comment

Choose a reason for hiding this comment

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

LGTM, thank you.
@Holzhaus does it work for you?

@daschuer
Copy link
Member

It still work btw.

@Holzhaus
Copy link
Member

Holzhaus commented Jun 15, 2021

Thanks. Unfortunately, this doesn't work for me:

$ ./mixxx -qmljsdebugger=port:9000
QML debugging is enabled. Only use this in a safe environment.
"Unknown option 'qmljsdebugger'."

https://doc.qt.io/qt-5/qtquick-debugging.html#starting-applications

Or how is this supposed to be used?

@Holzhaus
Copy link
Member

Holzhaus commented Jun 15, 2021

Ah, it actually works (I guess? At least I can see some variables in Qt Creator after attaching to that port) . But the message is confusing. I think this needs to be fixed in the command line arg parser.

@Swiftb0y
Copy link
Member Author

I think this needs to be fixed in the command line arg parser.

Seems to be the case. It occurs to me as well, but attaching with Qt Creator works. I can inspect the Component tree and do some profiling. If you don't have QtCreator installed, afaik there should be a qmlprofiler tool that you can use as well to create profile you can then inspect with gprof and similar tools.

@Swiftb0y
Copy link
Member Author

Swiftb0y commented Jun 16, 2021

Merge? Or should I take care of the Commandline parser first?

@Swiftb0y
Copy link
Member Author

"Unknown option 'qmljsdebugger'."

The issue seems to be that we're parsing the commandline args at the wrong time. We're parsing them in main while we should rather parse them in the QApplication subclass (so MixxxApplication I guess).
https://doc.qt.io/qt-5/qapplication.html#QApplication

@Holzhaus Holzhaus merged commit e2e1d94 into mixxxdj:main Jun 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants