BunsenLabs / bunsen-configs

Configuration files and scripts that are featured on a Bunsen Labs installation
https://pkg.bunsenlabs.org/debian/pool/main/b/bunsen-configs/
GNU General Public License v3.0
84 stars 25 forks source link

Lithium bl-xdg-autostart script args faulty error trapping #102

Closed capn-damo closed 4 years ago

capn-damo commented 4 years ago

This seems to be carried over from the original 'openbox-xdg-autostart':

Any script argument which is not one of --list,--help or --version causes errors such as

** (light-locker:996): WARNING **: 14:36:35.241: screensaver already running in this session

I don't see anything in def main(argv=sys.argv): to cope with this.

Adding this to the beginning of def main(argv=sys.argv): may fix it:

    arg_list = ['--help', '--list', '--version']
    if len(argv) == 1 or argv[1] not in arg_list:
        show_help()
        return 1
ghost commented 4 years ago

Hm, the option handling should likely be ported to argparse.ArgumentParser to replace this manual stuff. According to the code, any arguments that are not --list, --help or --version result in the argument(s) getting interpreted as desktop environment names, the choices of which is supposed to be only one or many of GNOME, KDE, XFCE, ROX and Old. So the correct fix would likely be to enforce this constraint. It seems the current code just lets it slide, resulting in this weird error because something indeterminable is getting executed. This is an interface bug.

I wonder if this tool is still required in its current form? Perhaps we can replace it with a tool from some Debian package? Surely there must be a way to launch all xdg autostart stuff apart from custom logic to this extent?

Next, is it normal for this tool to be executed like you did – manuell, in the terminal or is it usually hidden from users?

Had I known there was still a py2 program in a place like this, I'd rewritten it long ago. Perhaps it's a little late for that now :)

capn-damo commented 4 years ago

The autostart file has this instruction in the header comments: ## Run 'bl-xdg-autostart --list' to list any XDG autostarted programs.

This is how the bug came to light when a BL user used -h instead of --help.

bl-xdg-autostart is basically a re-named openbox-xdg-autostart

johnraff commented 4 years ago

Next, is it normal for this tool to be executed like you did – manuell, in the terminal or is it usually hidden from users?

It's run automatically on every startup, but users have the --list option to check what apps are being xdg autostarted and why/why not. That's quite a useful function I use myself from time to time. Had I known there was still a py2 program in a place like this, I'd rewritten it long ago. Perhaps it's a little late for that now.

There are still some (many?) py2 scripts among our utilities, and upgrading them to py3 has long been on my todo list. It's not something I can do myself, though, being a Python beginner. Would you like me to send you a list in case any of them can be easily fixed? It's not too late, as long as no outside interfaces change.

johnraff commented 4 years ago

if len(argv) == 1 or argv[1] not in arg_list: I think this needs changing. bl-xdg-autostart (like its openbox parent) is usually launched without any arguments in the user startup. With the condition len(argv) == 1 then it just returns failure and displays help instead of doing the autostarting.

EDIT: Tentative suggestion: if len(argv) == 2 and argv[1] not in arg_list: but bear in mind 2ion's proposal for more extensive rewrite.

capn-damo commented 4 years ago

OK I didn't think of the autostarted bl-xdg-autostart :(

But if the user runs it with no script args it fails with errors, so that situation needs error-trapping as well.

johnraff commented 4 years ago

if the user runs it with no script args it fails with errors

I think that's because of some of the previously autostarted processes objecting to more than one instance being started up. (Others will happily start a second instance.)

There might occasionally be situations - eg trouble-shooting - when a user actually wants to run the xdg autostart manually, no? Anyway, how to distinguish between that and the startup script? Test for a controlling terminal? We might just have to let this one be, though it's messy of course.