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

Git issue templates #172

Merged
merged 1 commit into from Jan 13, 2019
Merged

Git issue templates #172

merged 1 commit into from Jan 13, 2019

Conversation

friday
Copy link
Contributor

@friday friday commented Jan 13, 2019

Issue template suggestions (can be tested here)

Some of these may not be strictly needed:

  • "Remove private information (tokens, keys or id's for steam, gog or humble bundle)" from the log output - maybe I'm just paranoid here. I know this info can't be used to log in to any of the accounts online, but tokens can be abused in some cases.
  • bt full may not be more useful for you than just bt. You never asked me to use full, but I figured more info is better (set pagination off is to avoid the pager for bt full)
  • Not sure you need everything I asked for under "Environment", or if you need more.

Also, it may be possible to make these instructions simpler. For example:

  • Adding commands to help, like a --version cli option that prints the version (including git hash since the semver release isn't unique).
  • Not logging private information, or logging censored private info which cannot be abused (like "696417640987*****"), rather than users having to remove it later. Though you may want another flag for this.
  • Also, I think the current dev version is significantly more stable than the one in master, so maybe it's about time to release?

@tkashkin
Copy link
Owner

Thanks, I was thinking about issue templates like these.

Not logging private information

I'll probably remove it completely or add some CLI flag to log sensitive info as I think auth works correctly now.

I'll probably do the same thing with downloader and other too verbose logging. Suggest what else should be removed/hidden behind a CLI flag.

Also, I think the current dev version is significantly more stable than the one in master, so maybe it's about time to release?

Yes, I wanted to release it already, but there are some unfinished things every time I think about it. 🙂

@friday
Copy link
Contributor Author

friday commented Jan 13, 2019

I'll probably do the same thing with downloader and other too verbose logging. Suggest what else should be removed/hidden behind a CLI flag.

I'm not sure. I'm used to logs being very verbose, but perhaps it makes it harder to separate signal from noise. You're the one who has to read them though, so you can change what's being logged based on getting too much or too little info.

Yes, I wanted to release it already, but there are some unfinished things every time I think about it. 🙂

You're welcome 😉

@tkashkin
Copy link
Owner

tkashkin commented Jan 13, 2019

--debug logging is now less verbose by default.
It can be reenabled with CLI options (--debug is still required to change log level):

$ LANGUAGE=en src/com.github.tkashkin.gamehub --help

[INFO 19:19:15.378545] Application.vala:155: GameHub version: 0.12.1
[INFO 19:19:15.378571] Application.vala:157: Kernel version: 4.18.8-041808-generic
Usage:
  com.github.tkashkin.gamehub [OPTION…]

Help Options:
  -h, --help            Show help options

Application Options:
  -r, --run             Run game
  -c, --show-compat     Show compatibility options dialog
  -s, --show            Show main window
  --log-auth            Log authentication process and sensitive information like authentication tokens
  --log-downloader      Log download manager

tkashkin added a commit that referenced this pull request Jan 13, 2019
Add `--log-workers` option (#172)
Migrate to `Gtk.Application` from `Granite.Application`
@tkashkin
Copy link
Owner

$ LANGUAGE=en src/com.github.tkashkin.gamehub --help-all

Usage:
  com.github.tkashkin.gamehub [OPTION…]

Help Options:
  -h, --help            Show help options
  --help-all            Show all help options
  --help-log            Show logging options help

Logging Options
  -d, --debug           Enable debug logging
  --log-auth            Log authentication process and sensitive information like authentication tokens
  --log-downloader      Log download manager
  --log-workers         Log background workers start/stop

Application Options:
  -v, --version         Show application version and exit
  -r, --run             Run game
  -c, --show-compat     Show compatibility options dialog
  -s, --show            Show main window

$ src/com.github.tkashkin.gamehub -v

Version: 0.12.1-93f62c1-dev
Branch:  dev
Commit:  93f62c1 (93f62c1eb058f1725baa7f6f95d9e040793096a5)
Distro:  elementary OS 5.0 Juno
DE:      Pantheon

@tkashkin tkashkin merged commit fab595b into tkashkin:master Jan 13, 2019
tkashkin added a commit that referenced this pull request Jan 13, 2019
@friday
Copy link
Contributor Author

friday commented Jan 13, 2019

Nice!

I didn't have retroarch and got a warning (I'm guessing this is true for optdependencies like compatibility layers too?), and the branch detection fails with arch packages (though I think this is fines since you still get the hash)

which: no retroarch in (/opt/android-sdk/platform-tools:/home/albin/.nvm/versions/node/v8.14.0/bin:/usr/local/bin:/usr/bin:/bin:/usr/local/sbin:/opt/android-sdk/platform-tools:/usr/bin/site_perl:/usr/bin/vendor_perl:/usr/bin/core_perl)
Version: 0.12.1-5fc975a-makepkg
Branch:  makepkg
Commit:  5fc975a (5fc975a7521291f83b59e5b236df2db9404b322a)
Distro:  Antergos Linux
DE:      GNOME

@tkashkin
Copy link
Owner

tkashkin commented Jan 13, 2019

So I assume makepkg does git checkout -b makepkg, makes some changes, but does not commit anything so git rev-parse [--short] HEAD is still correct.

Do you know a way to get branch in that case?
Now git symbolic-ref --short -q HEAD is used and it's not correct in this case.

Can you try some solutions from https://stackoverflow.com/questions/6245570 to see if it's possible to get a correct branch name?

@tkashkin
Copy link
Owner

tkashkin commented Jan 13, 2019

There are even more problems with this in AppVeyor and Launchpad builds. Both can be fixed by passing version via meson options.


  • AppImage from AppVeyor:
Version: 0.12.1-8c28072-
Branch:  
Commit:  8c28072 (8c280728cd4a48072e5956f7e9b8ca737b4ca778)
Distro:  appimage
DE:      Pantheon
  • .deb package from Launchpad PPA (expected as it doesn't even checkout git repo):
Version: 0.12.1--
Branch:  
Commit:   ()
Distro:  elementary OS 5.0 Juno
DE:      Pantheon

@tkashkin
Copy link
Owner

meson will now accept -Dgit_branch, -Dgit_commit and -Dgit_commit_short options, so it can be worked around in PKGBUILD if it's not possible to get a correct info from meson script.

@friday
Copy link
Contributor Author

friday commented Jan 13, 2019

So I assume makepkg does git checkout -b makepkg, makes some changes, but does not commit anything so git rev-parse [--short] HEAD is still correct.

Yeah, apparently this is how it's done (didn't have any reason to find out before). It clones the bare repo (no files) first, then checks out the files from there to another directory with the branch makepkg for some reason (with no changes otherwise).

-Dgit_branch=dev worked 👍

@friday
Copy link
Contributor Author

friday commented Jan 13, 2019

Commented on the gamehub-git AUR board with the update.

@tkashkin
Copy link
Owner

Also, it may be possible to make these instructions simpler.

Now there's a --gdb option. It will restart GameHub in a separate xterm instance with GDB attached.

$ src/com.github.tkashkin.gamehub --gdb
[INFO]   Restarting with GDB
[DEBUG]  [Utils.run_async] Running {'xterm' '-geometry' '128x48' '-fa' 'Monospace' '-fs' '12' '-e' 'gdb' '-ex' 'set args --debug' '-ex' 'set pagination off' '-ex' 'run' 'src/com.github.tkashkin.gamehub'} in '/home/tk/Projects/GameHub/build'

I'm not exactly sure about -geometry 128x48 -fa Monospace -fs 12. It should ideally be configured by user, but most probably don't use xterm or don't configure it, and default font size seems to be too small.

@friday
Copy link
Contributor Author

friday commented Jan 15, 2019

I was thinking about something like that too.

However, I think the way it's currently implemented is more complicated than before, and adding settings would make it even more so. Seeing as this is (hopefully) not something users would have to do a lot, I think the "less is more" way is better. I like the --gdb flag, but would prefer if it just took over my current terminal session. That way it'd respect my preferences and work without any config, and for users without xterm (including me as I intentionally uninstalled it).

If it's possible to figure out a way for it to automatically backtrace and exit I think that'd help a lot. Then it could run non-interactively, and users could redirect the output to a file with >. I only briefly tried it. Adding -ex "bt full" to the cli options is supposed to work according to some stack overflow thread (I think) I read then. I'm pretty sure I tried that unsuccessfully, but wouldn't rule it out.

(I couldn't actually reproduce a crash now even with an old GameHub version for some reason, so I'm not able to test crashes currently)

@tkashkin
Copy link
Owner

I like the --gdb flag, but would prefer if it just took over my current terminal session. That way it'd respect my preferences and work without any config, and for users without xterm (including me as I intentionally uninstalled it).

I have tried to restart app in the same terminal session, but there are a few problems that are easily solved by starting a new terminal session.

  • Currently running GameHub instance must be finished before new instance is started, because GTK passes CLI arguments to the running instance via DBus and doesn't start a new instance when there's already a running instance.
  • If app is restarted asynchronously and old instance is finished immediately, new instance doesn't receive stdin (and it's impossible to interact with GDB). It leads to weird situation when control is returned to the shell, input also goes to shell, but output is still displayed.

I'll think how can it be improved.

@friday
Copy link
Contributor Author

friday commented Jan 15, 2019

Currently running GameHub instance must be finished before new instance is started, because GTK passes CLI arguments to the running instance via DBus and doesn't start a new instance when there's already a running instance.

Didn't you add this pretty recently? It was on my list of issues since I was able to launch two instances, but then I noticed that you had implemented it 👍

I think as long as it can detect this and print a helpful message to the user they'll know what to do. I actually think this is better than closing the app. It could be in the middle of something. If it's installing a game or showing the executable prompt after an install for example it could be confusing for users if they run debugging and it's restarted. The next time the game will not be marked as installed afaik.

@tkashkin
Copy link
Owner

tkashkin commented Jan 15, 2019

Didn't you add this pretty recently? It was on my list of issues since I was able to launch two instances, but then I noticed that you had implemented it 👍

It's default GTK command line behavior and it's worked this way in GameHub since beginning or at least since there are any CLI options. 😕

If you have one instance running, you can, for example, run com.github.tkashkin.gamehub --show to show main window in old instance or com.github.tkashkin.gamehub --run <game_id> to run a game in old instance without any additional data loading.

CLI args are passed to and parsed in old instance, new instance just passes the args and dies. This behavior is great, but it's the problem for restart.
Also it's a problem for commands like --version, right now info is printed by the old instance and new instance doesn't output anything.

@friday
Copy link
Contributor Author

friday commented Jan 15, 2019

I see. I guess my usage pattern changed then. I created a new issue for it #175. No hurry for me personally on that one.

tkashkin added a commit that referenced this pull request Jan 15, 2019
Restart with GDB in the same terminal session (#172)
@tkashkin
Copy link
Owner

tkashkin commented Jan 15, 2019

I have reworked CLI options parsing. Now it will parse local options like -h, -v, --gdb locally and pass other options to running instance.

It allows to restart app with GDB in the same terminal session.

I also have found this gist. It should launch GDB automatically, finish it when app closes and display backtrace if app crashes.

It will display bt if started with --gdb option and bt full if started with --gdb-bt-full.

@friday
Copy link
Contributor Author

friday commented Jan 15, 2019

This is perfect imo! 👍 Too bad you fixed the crashes first so I have nothing to test this with now 😉

@tkashkin tkashkin mentioned this pull request Apr 10, 2019
Lucki pushed a commit to Lucki/GameHub that referenced this pull request Oct 30, 2021
Lucki pushed a commit to Lucki/GameHub that referenced this pull request Oct 30, 2021
Add `--log-workers` option (tkashkin#172)
Migrate to `Gtk.Application` from `Granite.Application`


Former-commit-id: 70741b5
Lucki pushed a commit to Lucki/GameHub that referenced this pull request Oct 30, 2021
Lucki pushed a commit to Lucki/GameHub that referenced this pull request Oct 30, 2021
Lucki pushed a commit to Lucki/GameHub that referenced this pull request Oct 30, 2021
Lucki pushed a commit to Lucki/GameHub that referenced this pull request Oct 30, 2021
Lucki pushed a commit to Lucki/GameHub that referenced this pull request Oct 30, 2021
Lucki pushed a commit to Lucki/GameHub that referenced this pull request Oct 30, 2021
Lucki pushed a commit to Lucki/GameHub that referenced this pull request Oct 30, 2021
Restart with GDB in the same terminal session (tkashkin#172)


Former-commit-id: 0bbe7ee
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants