Release candidate for FR #1100 (unclear user-callback location)

[aryoda]

THIS IS A PREVIEW FOR DISCUSSIONS - PLEASE DO NOT YET MERGE INTO MASTER

I have created a first PR to make the location and file name of the user-callback script visible.

Applied changes:

1. backintime now shows the same content as before + (new!) the user callback location (I call this: "about" backintime)
2. backintime -v did show only the version number. It now shows the same "about" output as above

The main reason for 2. was that 1. is not obvious to call and 2. does not hurt (having a longer output now).
The version number can still be easily parsed via a regexp (if someone really needs to do so).

Open:

• I did not (yet) implement the syslog output whenever the user-callback is called (could also be a second PR)

Out of scope:

• GUI doesn't inform about errors in callback-script #827 (together with [Unit tests] Popen() warning: subprocess is still running (non-deterministic). #1292): I would work an that in a separate PR (to provide an agile "minimal viable product" here)

New output:

> backintime      # now same as:  backintime -v

Back In Time
Version: 1.3.2
User callback script file: /home/xxx/.config/backintime/user-callback

Back In Time comes with ABSOLUTELY NO WARRANTY.
This is free software, and you are welcome to redistribute it
under certain conditions; type `backintime --license' for details.

Old output:

> backintime -v
backintime 1.2.1

> backintime

Back In Time
Version: 1.2.1

Back In Time comes with ABSOLUTELY NO WARRANTY.
This is free software, and you are welcome to redistribute it
under certain conditions; type `backintime --license' for details.

Possible future extensions (out of scope for this PR/issue):

I think of extending the output later with another PR to show more "diagnostics" that could be copied&pasted
into a new issue, eg. also:

> backintime      # now same as:  backintime -v

Back In Time
Version: 1.3.2
User callback script file: /home/xxx/.config/backintime/user-callback
Distro: Ubuntu 20.04.5 LTS
Desktop Environment: KDE
Display Server protocol: wayland

Back In Time comes with ABSOLUTELY NO WARRANTY.
This is free software, and you are welcome to redistribute it
under certain conditions; type `backintime --license' for details.

[buhtz]

Great! I will take a closer look in the next days.

[emtiu]

I have a feeling that it might be nicer to keep the behavior of -v/--version the same as before, but introduce a new --about that would show all relevant info.

Also, backintime with no arguments might print a notice to users who might have been trying to run backintime-qt instead.

Just thinking out loud. Thanks for tackling this :)

[aryoda]

I have a feeling that it might be nicer to keep the behavior of -v/--version the same as before
but introduce a new --about that would show all relevant info.

I was thinking about this too and decided to introduce not another argument (the list is already quite long)
but technically it would be no problem and a clear separation.
--about would be the recommended "diagnostics" output then for issues...

Also, backintime with no arguments might print a notice to users who might have been trying to run backintime-qt instead.

Good idea, I will add this (esp. because this mistake happened to me too several times ;-)

[buhtz]

I agree with @emtiu that we shouldn't modify the -v behaviour.

Dear @aryoda what is your intention? It is debug information right? In that case I would introduce something like --diagnostic or a "more verbose" option like -vv.

I also would introduce a --json modifier which print out the diagnostic output in json format. No need to parse in that case. E.g. rsync itself offers something like this after I asked. ;)

Because of that I would recommend to introduce an internal method that collects all diagnostic data and return it as a dict(). Then dict can be used in the printAbout() or printDiagnostig() (or how ever you name it). And this is easier to test.

Didn't we had an Issues about that diagnostic output somewhere? Here it was: #1278 (comment)

EDIT: If you want I can implement just a dict-returning function and a unittest for it; just the function. To keep it atomic I would create an extra PR for that. After that you can use and set commandline arguments for it.

[aryoda]

THX all for your input!

I think, verbosity levels like -vv are quite difficult to understand for users so I'd prefer to add a new argument with a clearer name for diagnostics.

For now I would not offer to write a (machine-readable) json file because I see no use case for that (automation of what?).
I know that a version number as json could have helped with the the rsync issue of the changed cmd arg quotations
but in the case of backintime the version number is so easy to parse that a json would not simplify things (backintime -v > version.txt vs backintime --diagnostics --json). And we keep the -v output stable ;-)

So I suggest:

• I will let -v as it was originally since it is easy to parse by callers.
• I will let backintime (without arguments) as it was (but remove the git commit info after the version number and move this part to our new "diagnostics" argument where it belongs - backintime -v does also not show the git commit info!)
• I will introduce a new --show-diagnostics argument (we can find a better name of course).
  Here I will for now only print the version number and perhaps the python and rsync version (if a function in tools exist for this).
  I will add more diagnostics in a separate PR to keep this PR minimal (and to find out how the collect that info best - eg. via a Popen call. @buhtzz If you find time to implement such a collectDiagnosticsInfo() function it would be great!)
• I will also write the diagnostics output to the log when the --debug option is passed.
  This raises the question if the --debug arg could be used instead of a new --show-diagnostics arg
  but I think it is less obvious for a user to get diagnostic info via debug...

[buhtz]

Dear @aryoda ,
thanks for investing into that.

I think, verbosity levels like -vv are quite difficult to understand for users so I'd prefer to add a new argument with a clearer name for diagnostics.

Yeah, totally agree.

• I will introduce a new --show-diagnostics argument (we can find a better name of course).

IMHO no need for a verb here. I would prefere --diagnostic.

For now I would not offer to write a (machine-readable) json file because I see no use case for that (automation of what?).

Not a file. Just do print(json.dumps(diagnostic_dict, indent=4))) to stdout. This can be captured by any other tool (e.g. a user-callback script, Travis or other (integration) testing scripts) and escpecialy by our own unittest which are currently full with hard to understand and hard to maintain regex-parsing stuff.

Json output (with use of indent=) is nearly human readable. I would say not to add --json as an argument but just output the --diagnostic stuff as json. The --diagnostic is used only when we ask for it in a bug report.

I will add more diagnostics in a separate PR to keep this PR minimal (and to find out how the collect that info best - eg. via a Popen call. @buhtzz If you find time to implement such a collectDiagnosticsInfo() function it would be great!)

I will. Have done things like this before. I would add there

• BIT version including git-infos if present
  • Maybe try to figure out how BIT was installed (PyPi, Launchpad deb, deb repository, rpm repository, ...). On Debian I would simply do apt-cache policy backintime-qt to get all that details.
• Python version (incl. build infos etc)
• PyQt version and Qt version
• OS Name and version
• Version of external tools:
  • rsync
  • ssh
  • sshfs
  • ecnr ... dingsbums
  • shell used via subprocess.Popen()
  • pytest version if present
  • sphinx version if present
  • make version if present
• Folders
  • config location
  • user-callback location
  • is there a log file?
• Current "local" (language etc)

Some more ideas and needs?

Where would you locate such a function in the repository? I think backintime.py or tools.py are acceptable choices because they have content with the "same topic". But IMHO that files and all others need a restructuring (some far away day!). In the first place I would add a new common/diagnostic.py file. And later we maybe can join it somewhere. What would you suggest?

• I will also write the diagnostics output to the log when the --debug option is passed.

Good idea. And here you can use the content of the diagnostic dict to create nice looking debug output.

This raises the question if the --debug arg could be used instead of a new --show-diagnostics arg
but I think it is less obvious for a user to get diagnostic info via debug...

I would keep both. Users often don't produce debug output or don't know how to do it because they use the BIT gui and don't start it in a terminal emu. We can write in the issue tempale and/or contribute section: Please post the output of backintime --diagnostic here.

[aryoda]

@buhtzz Perfect, consensus found: I will use --diagnostics + print json to console (for now only the version number incl. git commit info and the user-callback location). Thanks for your dev support! 🥇

When you have PR'ed your diagnostics collector function we can extend the --diagnostics output.

@emtiu Any vetos? 😉

Edit:

Where would you locate such a function in the repository?

IMHO worth a new diagnostics.py file if you add a function per diagnostic item (tools.py is already too crowded)

[emtiu]

@emtiu Any vetos? ;)

Nope, I'm happy with your plans :)

Some more ideas and needs?

Let's try to determine whether a system is running X11 or Wayland. This will become more and more relevant in the future. I fear that there exists no clean, canoncial way to do this, but we'll find something :)

[aryoda]

@emtiu @buhtzz I have now implemented a release candidate incl. a small unit test.

The new --diagnostics argument now shows:

~/dev/backintime/common (issue/1100_unclear_user_callback_location)  > ./backintime --diagnostics
{
    "app_name": "Back In Time",
    "app_version": "1.3.2",
    "app_git_branch": "1100_unclear_user_callback_location",
    "app_git_commit": "d8715ec",
    "user_callback": "/home/xxx/.config/backintime/user-callback"
}

@buhtzz You are right, JSON is really easy in unit tests (compared to RegExp)!

The other arguments are now reverted again:

~/dev/backintime/common (issue/1100_unclear_user_callback_location)  > ./backintime -v
backintime 1.3.2

~/dev/backintime/common (issue/1100_unclear_user_callback_location)  > ./backintime

Back In Time
Version: 1.3.2

Back In Time comes with ABSOLUTELY NO WARRANTY.
This is free software, and you are welcome to redistribute it
under certain conditions; type `backintime --license' for details.

RfC

[aryoda]

Damn it, all Travis CI builds fail with

File "/home/travis/build/bit-team/backintime/common/test/test_backintime.py", line 180, in test_diagnostics_arg
...
json.decoder.JSONDecodeError: Expecting value: line 1 column 1 (char 0)
WARNING: [common/tools.py:2213 __logKeyringWarning] import keyring failed

My local unit tests work, but the Travis CI unit test fails when parsing the JSON output.

@emtiu Do you have the rights to restart the Travis CI build to check if this is just a hick-up
(otherwise I would have to push an artifical "bump only" commit)

[buhtz]

My work is in progres...
https://github.com/buhtzz/backintime/blob/feat/diagnostic/common/diagnostic.py

I will also take the X or Wayland thing into account.

[aryoda]

My work is in progres... https://github.com/buhtzz/backintime/blob/feat/diagnostic/common/diagnostic.py

Good morning (very early ;-)

I have integrated your code as first test and it works very good on Ubuntu 20.04.05 except the rsync thing:

'rsync: -VV: unknown option
rsync error: syntax or usage error (code 1) at main.c(1596) [client=3.1.3]'

and fails in line #81

My (old?) rsync says:

> rsync -v
rsync  version 3.1.3  protocol version 31

[aryoda]

I was thinking about back-porting this --diagnostics feature to v1.2.1 (which is widely used in distros) to improve our issue support.

This is not straight-forward since releases are only tagged but do not exist as separate branch where we could cherry-pick this feature-commit into.

So it looks like we have to convince the package maintainers to use 1.3.x once it is stable (or create a backport-patch and announce it the README as "recommended backports" hint for package maintainers).

[emtiu]

Please note that diagnostics is the right spelling (with an s at the end of the word) :)

[emtiu]

I was thinking about back-porting this --diagnostics feature to v1.2.1 (which is widely used in distros) to improve our issue support.

I think that's a battle we can't win. We'll just have to deal with the fact that installations of 1.3.2 and older are going to be around for a while.

Let's not try to "change history", otherwise we'll just end up asking: "So you're running 1.2.1, but is it the original 1.2.1, or the one we modified afterwards?" ;)

[buhtz]

I think that's a battle we can't win.
...
Let's not try to "change history",

I agree here. The project is not in the shape currently to handle something like that.

[aryoda]

OK, I have finalized my PR, unit tests are OK, TravisCI too.

Please note that I have git rebased to get rid of the commit mess due to testing TravisCI and reverting the first draft
(I know it is against the rules to rebase a pushed branch but I think PRs are an exception since they are one-way street).

@emtiu Could you merge please if everything is OK? Fast forward preferred for a straight feature history (one feature per commit)

Next step:

Extend the --diagnostics output in a separate PR that @buhtzz is preparing (couldn't have dreamt of this amount of output 👍 )

[emtiu]

Looks good, thanks :)