fix helper

[hakavlad]

No description provided.

[codelingo]

LGTM

[hakavlad]

• fix: exclude root
• this script just works now
• todo: fix makefile to install this script in /usr
• todo: update readme: how to use this script

[hakavlad]

Do you have any questions?

[rfjakob]

Yes, why is there no help text anymore for the script itself, and why does it require 5 arguments now instead of 2?

[hakavlad]

why does it require 5 arguments now instead of 2

mem avail:     0 of  3859 MiB ( 0 %), swap free: 1233 of 7718 MiB (15 %)
mem avail:     0 of  3859 MiB ( 0 %), swap free:  846 of 7718 MiB (10 %)
low memory! at or below SIGTERM limits: mem 10 %, swap 10 %
sending SIGTERM to process 30747 "tail": badness 586, VmRSS 2128 MiB
process exited after 0.5 seconds
['./helper', '-i', 'dialog-warning', 'earlyoom', 'Low memory! Killing process 30747 tail']
mem avail:  2118 of  3859 MiB (54 %), swap free: 5504 of 7718 MiB (71 %)
mem avail:  2077 of  3859 MiB (53 %), swap free: 5512 of 7718 MiB (71 %)

The old script with two arguments did not work at all.
New script works: https://imgur.com/a/p7ucI5k

[hakavlad]

why is there no help text anymore for the script itself

It can be fixed later if you want.

[hakavlad]

@rfjakob What's up?

[codelingo]

LGTM

[hakavlad]

@rfjakob could you merge this PR?

[rfjakob]

Requiring 5 arguments is unneccessary, and the script needs to have a help text.
The old script works fine with 2 arguments on Fedora.

[hakavlad]

Requiring 5 arguments is unneccessary

On the contrary, the helper accepts exactly five arguments in argv:

[
    'notify_all_users.py',                    # argv[0]
    '-i',                                     # argv[1]
    'dialog-warning',                         # argv[2]
    'earlyoom',                               # argv[3]
    'Low memory! Killing process 30747 tail'  # argv[4]
]

This is how it works if you use this script via earlyoom -N notify_all_users.py to notify all users.

[hakavlad]

The old script works fine with 2 arguments on Fedora.

How do you use old script? How do you connect it to earlyoom?

[hakavlad]

The old script works fine with 2 arguments on Fedora.

OK, but earlyoom uses more then 2 arguments.

$ sudo ./notify_all_users.py
Usage:
  ./notify_all_users.py [notify-send options] summary [body text]
Examples:
  ./notify_all_users.py mytitle mytext
  ./notify_all_users.py -i dialog-warning earlyoom "killing process X"

OK

$ sudo ./notify_all_users.py mytitle mytext

OK, it silently shows a notification.

$ sudo ./notify_all_users.py -i dialog-warning earlyoom "killing process X"
No summary specified. 

No notification was shown. Is it OK?

The old script is completely incompatible with earlyoom.

[hakavlad]

You broke the original script (https://github.com/hakavlad/notify-send-root-wrapper). Original script and new script just work. Your version of the script does not work as intended.

[codelingo]

LGTM

[hakavlad]

script needs to have a help text

Done. And it works with any len(argv).

[rfjakob]

Merged as 23bb71a , thanks