run hotspot without GUI for any command-line only option

lievenhey commented 1 year ago

You will need to rebase you change once #550 is landed. It will fix the clazy compaint, but you will need to fix the clang-tidy complaint yourself

lievenhey commented 1 year ago

same with your other PRs

GitMensch commented 1 year ago

Done. @lievenhey as the origin code was added by you, please review.

GitMensch commented 1 year ago

Thanks for the comments, that's all very reasonable (you likely recognize that I'm a C guy...).

I've adjusted this (see first commit, works fine) but then recognized that we duplicate part of the parser. After checking the parser only takes the application args from app, so it occurred to me that we may can swap that.

I personally like the result (second commit) - the only down-side is that the parser only uses a QStringList and the QCoreApplication only takes argc+argv so we need to temporary create the list for the parser (there may be better options for getApplicationArgs() and then let the QCoreApplication create it again... but that moves any app init after the QCommandLineParser is done (so inbuilt help/version options and bad arguments are handled without those) and for "own" command line options we can check the setting via the parser instead.

I've did several tests including filenames that contain non-ansi characters, all passed (should we add some of those to the integrationtests, like DISPLAY=banana bin/hotspot --version (which before this PR returned a QT display error, now returns the version) and DISPLAY=banana bin/hotspot --apple (which returned a QT display error before; using the second commit returns an error about the bad arg)?

milianw commented 1 year ago

I don't like the second patch, can we drop that? I don't think we are should be doing anything too fancy for that purpose. if you are working without a GUI, simply don't use hotspot - just use perf record directly. You can grab the command line parameters from a record session in hotspot on a secondary machine and copy it over.

GitMensch commented 1 year ago

Please reconsider as the second patch mostly moves around existing code and removes existing "fancy double parsing". If you want to, I can also drop the generated doxygen comment for createApplication, reducing the patch by 20%.

If you don't want the second commit... just drop a note if you want to cherry-pick it manually (and close this PR) or want this PR to be reduced.

milianw commented 1 year ago

I don't think it's sound to create the qcommandlineparser before the qapp - a lot of Qt code assumes the qapp to be around already, and all examples do that too, see e.g.: https://doc.qt.io/qt-6/qcommandlineparser.html

I'll talk to some colleagues about this, but again - I believe the whole point of this patch is debatable. I'm fine with the first patch since it's so minor. the second one though - it's very subjective, no? it doesn't actually change any behavior, or what am I missing? to me it looks like you are shoehorning a functionality into an app that is simply not very worthwhile to have for me - it just increases code noise for no real gain.

why do you insist on using hotspot without a display driver? maybe all you need to do instead is run hotspot -platform offscreen ... or QT_QPA_PLATFORM=offscreen hotspot ... instead?

GitMensch commented 1 year ago

why do you insist on using hotspot without a display driver? maybe all you need to do instead is run hotspot -platform offscreen ... or QT_QPA_PLATFORM=offscreen hotspot ... instead?

I have to check those options, but the first test did not worked out

qt.qpa.plugin: Could not find the Qt platform plugin "offscreen" in ""

The main point is not to use it without a display driver, but to not create a full gui application to directly let it be teared down if there is an issue with the command line arguments detected.

it doesn't actually change any behavior, or what am I missing?

it removes the application initialization if there's version or help output, after the first patch this creates the QCoreApplication before (looking up XDG_DATA_HOME and friends)
it removes the application initialization if there's a parser error, until now this creates the full QApplication GUI before (including the display driver)
it removes the additional check for command line options by "self-parsing before parsing via parser.process()"

I'll talk to some colleagues about this

Thank you. In the end I'll have learned something new :-)

milianw commented 1 year ago

why do you insist on using hotspot without a display driver? maybe all you need to do instead is run hotspot -platform offscreen ... or QT_QPA_PLATFORM=offscreen hotspot ... instead?

I have to check those options, but the first test did not worked out

qt.qpa.plugin: Could not find the Qt platform plugin "offscreen" in ""

This is with the appimage I guess? Packaging problem - could be solved there.

The main point is not to use it without a display driver, but to not create a full gui application to directly let it be teared down if there is an issue with the command line arguments detected.

This is not a reason. Creating a full gui app is cheap and not an issue on its own. Complicating the code just for the sake of it is not acceptable for me.

it doesn't actually change any behavior, or what am I missing?

* it removes the application initialization if there's version or help output, after the first patch this creates the QCoreApplication before (looking up XDG_DATA_HOME and friends)

This is not an explanation: Why is this needed or desired?

* it removes the application initialization if there's a `parser` error, until now this creates the full QApplication GUI before (including the display driver)

Again, why would we care?

* it removes the additional check for command line options by "self-parsing before parsing via parser.process()"

My understanding so far is that we only introduced this to allow users to run recording through hotspot when there's no GUI available. Since the patch was easy I was fine with this, but further complicating the code base isn't desired for me - this is a fringe use case. Just copy the perf record line and be done with it would be so much easier.

I'll talk to some colleagues about this

Thank you. In the end I'll have learned something new :-)

GitMensch commented 1 year ago

My understanding so far is that we only introduced this to allow users to run recording through hotspot when there's no GUI available.

That would be super cool, but I don't think that's possible, because we have no command line options for recording. Would you support a FR to record via command line?

What is possible already is to export a perf.data to .perfparser, using --export-to. The first commit of this PR adds the option to output the version and help.

it removes the application initialization if there's version or help output, there's version or help output, after the first patch this creates the QCoreApplication before (looking up XDG_DATA_HOME and friends)

This is not an explanation: Why is this needed or desired?

It is always desired to save cpu cycles. When I "just want the version" I don't desire to get warnings about drivers, platforms or anything and I'd like to see that output "in an instant". If I get an error about some QT settings on start (this aborts the creation of the Q[Core]Application) then I desire to get --help-all output, not another error that there's a QT issue. But the first commit covers most of this already, and the difference in the cpu cycle are not that much (only 3% saved).

... I've pushed without the second commit, just for reference - this was the difference in its output:

only commit 1

$ DISPLAY=banana ./bin/hotspot -H
qt.qpa.xcb: could not connect to display banana
qt.qpa.plugin: Could not load the Qt platform plugin "xcb" in "" even though it was found.
This application failed to start because no Qt platform plugin could be initialized. Reinstalling the application may fix this problem.

Available platform plugins are: eglfs, linuxfb, minimal, minimalegl, offscreen, vnc, wayland-egl, wayland, wayland-xcomposite-egl, wayland-xcomposite-glx, xcb.

Aborted
$ DISPLAY=banana ./bin/hotspot -platform offscreen -H
QStandardPaths: XDG_RUNTIME_DIR not set, defaulting to '/tmp/runtime-gitpod'
QStandardPaths: XDG_RUNTIME_DIR not set, defaulting to '/tmp/runtime-gitpod'
hotspot: Unknown option 'H'.
$ ./bin/hotspot --version
QStandardPaths: XDG_RUNTIME_DIR not set, defaulting to '/tmp/runtime-gitpod'
hotspot 1.4.80

with previous commit 2

$ DISPLAY=banana ./bin/hotspot -H
hotspot: Unknown option 'H'.
$ DISPLAY=banana ./bin/hotspot -platform offscreen -H
hotspot: Unknown option 'H'.
$ ./bin/hotspot --version
hotspot 1.4.80

milianw commented 1 year ago

My understanding so far is that we only introduced this to allow users to run recording through hotspot when there's no GUI available.

That would be super cool, but I don't think that's possible, because we have no command line options for recording. Would you support a FR to record via command line?

What is possible already is to export a perf.data to .perfparser, using --export-to. The first commit of this PR adds the option to output the version and help.

Sorry, I indeed meant exporting, not recording. And I'm fine with adding the first commit to get version and help output too.

it removes the application initialization if there's version or help output, there's version or help output, after the first patch this creates the QCoreApplication before (looking up XDG_DATA_HOME and friends)

This is not an explanation: Why is this needed or desired?

It is always desired to save cpu cycles.

No, this is blatantly false. Saving CPU cycles is only worthwhile when it matters - I doubt you'll manage to measure the difference in time for initializing a QCoreApp or a QApp - especially considering that most users won't ever use these fringe features.

The code you want to add here increases maintainability costs as it's more complicated. I'm not going to get this in just to save a few thousand cpu cycles that noone will ever notice.

When I "just want the version" I don't desire to get warnings about drivers, platforms or anything and I'd like to see that output "in an instant". If I get an error about some QT settings on start (this aborts the creation of the Q[Core]Application) then I desire to get --help-all output, not another error that there's a QT issue. But the first commit covers most of this already, and the difference in the cpu cycle are not that much (only 3% saved).

Exactly.

... I've pushed without the second commit, just for reference - this was the difference in its output:

only commit 1

$ DISPLAY=banana ./bin/hotspot -H
qt.qpa.xcb: could not connect to display banana
qt.qpa.plugin: Could not load the Qt platform plugin "xcb" in "" even though it was found.
This application failed to start because no Qt platform plugin could be initialized. Reinstalling the application may fix this problem.

Available platform plugins are: eglfs, linuxfb, minimal, minimalegl, offscreen, vnc, wayland-egl, wayland, wayland-xcomposite-egl, wayland-xcomposite-glx, xcb.

Aborted
$ DISPLAY=banana ./bin/hotspot -platform offscreen -H
QStandardPaths: XDG_RUNTIME_DIR not set, defaulting to '/tmp/runtime-gitpod'
QStandardPaths: XDG_RUNTIME_DIR not set, defaulting to '/tmp/runtime-gitpod'
hotspot: Unknown option 'H'.
$ ./bin/hotspot --version
QStandardPaths: XDG_RUNTIME_DIR not set, defaulting to '/tmp/runtime-gitpod'
hotspot 1.4.80

with previous commit 2

$ DISPLAY=banana ./bin/hotspot -H
hotspot: Unknown option 'H'.
$ DISPLAY=banana ./bin/hotspot -platform offscreen -H
hotspot: Unknown option 'H'.
$ ./bin/hotspot --version
hotspot 1.4.80

I'm fine with the behavior of patch #1 only.

KDAB / hotspot