build: sort.py improvements #5429

kmk3 · 2022-10-18T18:12:31Z

$ git log --reverse --pretty='* %s' master..
* sort.py: enumerate lines from 1
* sort.py: pass the str.casefold function directly
* sort.py: simplify "checking" message code
* sort.py: rename raw_items to original_items
* sort.py: rename protocols to original_protocols
* sort.py: line-wrap and improve comments
* sort.py: print errors to stderr
* sort.py: print more standard error messages

Instead of manually adding 1 to lineno.

To the sort function, instead of wrapping it in a lambda function.

Which also makes it fit in under 80 characters. Always print "profile(s)" instead of changing the message based on the argument count.

To make it clearer. Both the input and output of the sort_alphabetical function are strings of comma-separated items, so there is no format conversion of any kind being done (from "raw" to "not raw"), only sorting.

To make it clearer. There are 3 different instances of protocol-related objects being used in the fix_protocol function: * The input * The array of common sorted lines * The (sorted) output

rusty-snake

LGTM

PEP-8

Actually PEP-8 suggests 72 characters for docstrings¹. And PEP 257 has some recommendations for the first line of multi-line docstrings².

Nothing to act on, just FTR.

contrib/sort.py

kmk3 · 2022-10-18T18:55:31Z

@rusty-snake reviewed on Oct 18:

PEP-8

Actually PEP-8 suggests 72 characters for docstrings1.

https://peps.python.org/pep-0008/#maximum-line-length leftwards_arrow_with_hook

Indeed; removed the reference to PEP-8.

I just wanted to make it fit in 79 or 80 columns.

Also, what do you think about making it print __doc__ if no argument is
given?

And using argv[0] instead of ./sort.py in the docstring?

Misc: Interesting, I didn't know about this footnotes functionality.

rusty-snake · 2022-10-18T19:01:25Z

Also, what do you think about making it print doc if no argument is given?

Sounds good.

It should also do this if only --help is given.

And using argv[0] instead of ./sort.py in the docstring?

Are there some rules about using f-strings for docstrings?

argv[0] tells you only the path to the file, be it relative or absolute, or the filename without path (some corner cases ignored). It does not tell you if the file is executable or if it was used with python3 argv[0].

kmk3 · 2022-10-18T21:01:55Z

@rusty-snake reviewed on Oct 18:

And PEP 257 has some recommendations for the first line of multi-line docstrings2.

https://peps.python.org/pep-0257/#multi-line-docstrings leftwards_arrow_with_hook

I saw docstrings using both forms in the docs (and in SO answers); I'm still
not sure when the first line should be blank or not.

Should it be not blank in the main docstring and blank in functions?

I think that it indeed makes sense for at least the main one to be non-blank,
to avoid a blank line in the usage output.

Though it doesn't look as good in the source and potentially makes the first
line a bit shorter than the other ones when printing, so if it's intended to be
used unprocessed as part of the output, I think that it would make sense to
leave it blank and escape the newline.

@rusty-snake commented on Oct 18:

Also, what do you think about making it print doc if no argument is
given?

Sounds good.

It should also do this if only --help is given.

Since it currently does not have any options, IMO only printing it without
arguments keeps it nice and simple (as then all arguments mean the same thing
and there is no need to add an "Options" section). But if you really want it I
can add it.

And using argv[0] instead of ./sort.py in the docstring?

Are there some rules about using f-strings for docstrings?

I don't know, but it works for me at least. The main difference is that the
imports have to appear before before the docstring in this case (for
sys.argv).

argv[0] tells you only the path to the file, be it relative or absolute, or
the filename without path (some corner cases ignored). It does not tell you
if the file is executable or if it was used with python3 argv[0].

It always prints the filename used for running the script, even if it was
called indirectly. Using print(argv[0]); return:

$ ./contrib/sort.py
./contrib/sort.py
$ (cd contrib && ./sort.py)
./sort.py
$ python contrib/sort.py
contrib/sort.py

The same applies to shell scripts (and likely also to other scripting
languages). Using echo "$0"; return:

$ ./contrib/sort.sh
./contrib/sort.sh
$ (cd contrib && ./sort.sh)
./sort.sh
$ bash contrib/sort.sh
contrib/sort.sh

For example, the shell script usage equivalent would be something like this:

sort.sh

#!/bin/sh

usage() {
	cat <<EOF
Sort the arguments of the commands that support multiple arguments in profiles;
the following commands are supported:

    private-bin, private-etc, private-lib, caps.drop, caps.keep, seccomp.drop,
    seccomp.drop, protocol

Usage: $(basename "$0") [/path/to/profile ...]

Keep in mind that this will overwrite your profile(s).

Examples:
    $ $0 MyAwesomeProfile.profile
    $ $0 new_profile.profile second_new_profile.profile
    $ $0 ~/.config/firejail/*.{profile,inc,local}
    $ sudo $0 /etc/firejail/*.{profile,inc,local}

Exit Codes:
  0: Success: No profiles needed fixing.
  1: Error: One or more profiles could not be processed correctly.
  101: Info: One or more profile were fixed.
EOF
}

usage

Output:

output of sort.sh

$ ./contrib/sort.sh
Sort the arguments of the commands that support multiple arguments in profiles;
the following commands are supported:

    private-bin, private-etc, private-lib, caps.drop, caps.keep, seccomp.drop,
    seccomp.drop, protocol

Usage: sort.sh [/path/to/profile ...]

Keep in mind that this will overwrite your profile(s).

Examples:
    $ ./contrib/sort.sh MyAwesomeProfile.profile
    $ ./contrib/sort.sh new_profile.profile second_new_profile.profile
    $ ./contrib/sort.sh ~/.config/firejail/*.{profile,inc,local}
    $ sudo ./contrib/sort.sh /etc/firejail/*.{profile,inc,local}

Exit Codes:
  0: Success: No profiles needed fixing.
  1: Error: One or more profiles could not be processed correctly.
  101: Info: One or more profile were fixed.

Anyway, the main advantage of doing this is that the help section remains
consistent even if the filename is changed. For example, if someone renames
sort.py to firejail-sort.py and puts it somewhere in $PATH.

Since it's in a standalone script (and not a module in a larger system, for
example), I think that this makes sense in this case.

Anyway, I force-pushed with both changes; let me know what you think.

Not sure if it's a very "Pythonic" solution, but I can't think of any other way
of accomplishing this.

Changes: * Line-wrap comments at 79 characters * Make comments clearer * Make main docstring more similar to a command "usage" output See the result with the following command, which generates a man-page-like output and opens it in the man pager (such as in `less`): $ pydoc ./contrib/sort.py See also PEP-257, "Docstring Conventions"[1]. [1] https://peps.python.org/pep-0257/

Misc: The trailing comma is due to using the opinionated `black` Python formatter (which seems to be a relatively common one). This was the only change made, so the code seems to already be following the format used by this tool.

Where applicable, instead of creating custom ones. Example error messages: rm -f 123 && ./contrib/sort.py 123 [ Error ] [Errno 2] No such file or directory: '123' touch 123 && chmod -rwx 123 && ./contrib/sort.py 123 [ Error ] [Errno 13] Permission denied: '123'

kmk3 · 2022-10-19T06:10:41Z

(Force-push)

Tried to make the main docstring a bit clearer; I think that it looks better
now when looking at it with pydoc:

pydoc contrib/sort.py

Also, added more details to some commit messages.

contrib/sort.py

@rusty-snake

And return a specific exit code, as suggested by @rusty-snake[1]. Escape the first line in the docstring to avoid printing a blank line as the first line of the output. [1] netblue30#5429 (comment)

With this, the help section remains consistent regardless of how the script is called and even if the filename is changed. For example, if someone renames "sort.py" to "firejail-sort" and puts it somewhere in `$PATH`. Example outputs of the script name (using `print(argv[0]); return`): $ ./contrib/sort.py ./contrib/sort.py $ python contrib/sort.sh contrib/sort.py $ (cd contrib && ./sort.py) ./sort.py Note: This depends on `os.path` and `sys.argv`, so the imports have to appear before the docstring. In which case, the docstring has to be explicitly assigned to `__doc__` (as it ceases to be the first statement in the file). Note2: When running `pydoc ./contrib/sort.py`, `argv[0]` becomes "/usr/bin/pydoc" (using python 3.10.8-1 on Artix Linux).

Fred-Barclay

Looks good to me - I say merge it in @kmk3 !

glitsj16

LGTM. Thanks for the reminder in #5479.

Relates to #5429 #5478 #5481.

Rename `line` to `original_line` to make it less likely to accidentally read from/write to it instead of the fixed line. Rename `fixed_line` to `line` to make the code shorter since it is now referenced much more often (up to 3 times in the same line of code) than the original line. See also commit aa17ca5 ("sort.py: rename protocols to original_protocols", 2022-10-17) / PR netblue30#5429.

kmk3 added 5 commits October 18, 2022 14:43

sort.py: enumerate lines from 1

89880e9

Instead of manually adding 1 to lineno.

sort.py: pass the str.casefold function directly

801a995

To the sort function, instead of wrapping it in a lambda function.

sort.py: simplify "checking" message code

b9c2843

Which also makes it fit in under 80 characters. Always print "profile(s)" instead of changing the message based on the argument count.

sort.py: rename raw_items to original_items

2a7cc2e

To make it clearer. Both the input and output of the sort_alphabetical function are strings of comma-separated items, so there is no format conversion of any kind being done (from "raw" to "not raw"), only sorting.

sort.py: rename protocols to original_protocols

aa17ca5

To make it clearer. There are 3 different instances of protocol-related objects being used in the fix_protocol function: * The input * The array of common sorted lines * The (sorted) output

kmk3 requested a review from rusty-snake October 18, 2022 18:12

kmk3 force-pushed the sort-py-improvements branch from afb2f52 to ad74763 Compare October 18, 2022 18:31

rusty-snake reviewed Oct 18, 2022

View reviewed changes

contrib/sort.py Outdated Show resolved Hide resolved

kmk3 force-pushed the sort-py-improvements branch from ad74763 to 8b0502c Compare October 18, 2022 18:45

rusty-snake approved these changes Oct 18, 2022

View reviewed changes

kmk3 force-pushed the sort-py-improvements branch from 8b0502c to 261be8d Compare October 18, 2022 21:00

kmk3 force-pushed the sort-py-improvements branch 2 times, most recently from 027fcbf to 7e172a5 Compare October 19, 2022 01:41

kmk3 added 3 commits October 19, 2022 02:59

sort.py: print errors to stderr

8635858

Misc: The trailing comma is due to using the opinionated `black` Python formatter (which seems to be a relatively common one). This was the only change made, so the code seems to already be following the format used by this tool.

kmk3 force-pushed the sort-py-improvements branch from 7e172a5 to 58e403b Compare October 19, 2022 06:05

rusty-snake reviewed Oct 19, 2022

View reviewed changes

contrib/sort.py Outdated Show resolved Hide resolved

sort.py: print usage if there are no arguments

b1bf9a8

And return a specific exit code, as suggested by @rusty-snake[1]. Escape the first line in the docstring to avoid printing a blank line as the first line of the output. [1] netblue30#5429 (comment)

kmk3 force-pushed the sort-py-improvements branch from 58e403b to 5b16fb3 Compare October 22, 2022 01:20

kmk3 force-pushed the sort-py-improvements branch from 5b16fb3 to c648adc Compare October 22, 2022 03:17

rusty-snake approved these changes Oct 22, 2022

View reviewed changes

Fred-Barclay approved these changes Nov 22, 2022

View reviewed changes

glitsj16 approved these changes Nov 23, 2022

View reviewed changes

kmk3 merged commit 9ab61c4 into netblue30:master Nov 23, 2022

kmk3 deleted the sort-py-improvements branch November 23, 2022 16:32

kmk3 added a commit that referenced this pull request Nov 25, 2022

RELNOTES: add build and ci items

8b7e343

Relates to #5429 #5478 #5481.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

build: sort.py improvements #5429

build: sort.py improvements #5429

kmk3 commented Oct 18, 2022

rusty-snake left a comment

kmk3 commented Oct 18, 2022 •

edited

Loading

rusty-snake commented Oct 18, 2022

kmk3 commented Oct 18, 2022 •

edited

Loading

kmk3 commented Oct 19, 2022

Fred-Barclay left a comment

glitsj16 left a comment

build: sort.py improvements #5429

build: sort.py improvements #5429

Conversation

kmk3 commented Oct 18, 2022

rusty-snake left a comment

Choose a reason for hiding this comment

Footnotes

kmk3 commented Oct 18, 2022 • edited Loading

rusty-snake commented Oct 18, 2022

kmk3 commented Oct 18, 2022 • edited Loading

kmk3 commented Oct 19, 2022

Fred-Barclay left a comment

Choose a reason for hiding this comment

glitsj16 left a comment

Choose a reason for hiding this comment

kmk3 commented Oct 18, 2022 •

edited

Loading

kmk3 commented Oct 18, 2022 •

edited

Loading