[libvirt PATCH 0/2] syntax-check: Ensure Python is called via env(1)
As suggested in [1]. [1] https://listman.redhat.com/archives/libvir-list/2022-December/236211.html *** BLURB HERE *** Andrea Bolognani (2): docs: Recommend better python3 shebang syntax-check: Ensure Python is called via env(1) build-aux/syntax-check.mk | 5 +++++ docs/hooks.rst | 2 +- 2 files changed, 6 insertions(+), 1 deletion(-) -- 2.39.2
Python scripts should always invoked the interpreter through env(1) to ensure that they work on macOS and the BSDs, and at this point not explicitly asking for Python 3 doesn't really make sense. Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- docs/hooks.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/hooks.rst b/docs/hooks.rst index 9c5f3ff456..9387676a55 100644 --- a/docs/hooks.rst +++ b/docs/hooks.rst @@ -78,7 +78,7 @@ or: :: - #!/usr/bin/python + #!/usr/bin/env python3 Other command interpreters are equally valid, as is any executable binary, so you are welcome to use your favourite languages. -- 2.39.2
The syntax-check rule that calls flake8 on Python scripts expects this to be the case, and it's the best practice anyway. Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- build-aux/syntax-check.mk | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/build-aux/syntax-check.mk b/build-aux/syntax-check.mk index 96d322ee04..21f6b311ce 100644 --- a/build-aux/syntax-check.mk +++ b/build-aux/syntax-check.mk @@ -561,6 +561,11 @@ sc_require_enum_last_marker: { echo 'enum impl needs _LAST marker on second line' 1>&2; \ exit 1; } || : +sc_prohibit_python_without_env: + @prohibit='#!/usr/.*/py''thon' \ + halt='always call python via /usr/bin/env' \ + $(_sc_search_regexp) + # We're intentionally ignoring a few warnings # # E501: Force breaking lines at < 80 characters results in -- 2.39.2
On Mon, Feb 20, 2023 at 11:21:46AM +0100, Andrea Bolognani wrote:
The syntax-check rule that calls flake8 on Python scripts expects this to be the case, and it's the best practice anyway.
Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- build-aux/syntax-check.mk | 5 +++++ 1 file changed, 5 insertions(+)
diff --git a/build-aux/syntax-check.mk b/build-aux/syntax-check.mk index 96d322ee04..21f6b311ce 100644 --- a/build-aux/syntax-check.mk +++ b/build-aux/syntax-check.mk @@ -561,6 +561,11 @@ sc_require_enum_last_marker: { echo 'enum impl needs _LAST marker on second line' 1>&2; \ exit 1; } || :
+sc_prohibit_python_without_env: + @prohibit='#!/usr/.*/py''thon' \
Shouldn't this be just '#!/usr/.*/python' ? Erik
+ halt='always call python via /usr/bin/env' \ + $(_sc_search_regexp) + # We're intentionally ignoring a few warnings # # E501: Force breaking lines at < 80 characters results in -- 2.39.2
On Mon, Feb 20, 2023 at 11:40:34AM +0100, Erik Skultety wrote:
On Mon, Feb 20, 2023 at 11:21:46AM +0100, Andrea Bolognani wrote:
+sc_prohibit_python_without_env: + @prohibit='#!/usr/.*/py''thon' \
Shouldn't this be just '#!/usr/.*/python' ?
Yes, but then the syntax-check rule would flag this very line :) We could do the "exempt from syntax-check" dance, but that feels unnecessarily complicated in this scenario. sc_copyright_usage already uses a similar trick to prevent the regex to match itself. -- Andrea Bolognani / Red Hat / Virtualization
On Mon, Feb 20, 2023 at 03:02:42AM -0800, Andrea Bolognani wrote:
On Mon, Feb 20, 2023 at 11:40:34AM +0100, Erik Skultety wrote:
On Mon, Feb 20, 2023 at 11:21:46AM +0100, Andrea Bolognani wrote:
+sc_prohibit_python_without_env: + @prohibit='#!/usr/.*/py''thon' \
Shouldn't this be just '#!/usr/.*/python' ?
Yes, but then the syntax-check rule would flag this very line :)
We could do the "exempt from syntax-check" dance, but that feels unnecessarily complicated in this scenario. sc_copyright_usage already uses a similar trick to prevent the regex to match itself.
Reviewed-by: Erik Skultety <eskultet@redhat>
On Mon, Feb 20, 2023 at 02:33:55PM +0100, Erik Skultety wrote:
On Mon, Feb 20, 2023 at 03:02:42AM -0800, Andrea Bolognani wrote:
On Mon, Feb 20, 2023 at 11:40:34AM +0100, Erik Skultety wrote:
On Mon, Feb 20, 2023 at 11:21:46AM +0100, Andrea Bolognani wrote:
+sc_prohibit_python_without_env: + @prohibit='#!/usr/.*/py''thon' \
Shouldn't this be just '#!/usr/.*/python' ?
Yes, but then the syntax-check rule would flag this very line :)
We could do the "exempt from syntax-check" dance, but that feels unnecessarily complicated in this scenario. sc_copyright_usage already uses a similar trick to prevent the regex to match itself.
Reviewed-by: Erik Skultety <eskultet@redhat>
Missing the .com here, you might have a misconfiguration somewhere. Anyway, thanks for the review! Can you please ACK the first patch in the series too? I can't push this one without it. -- Andrea Bolognani / Red Hat / Virtualization
On Mon, Feb 20, 2023 at 08:38:32AM -0800, Andrea Bolognani wrote:
On Mon, Feb 20, 2023 at 02:33:55PM +0100, Erik Skultety wrote:
On Mon, Feb 20, 2023 at 03:02:42AM -0800, Andrea Bolognani wrote:
On Mon, Feb 20, 2023 at 11:40:34AM +0100, Erik Skultety wrote:
On Mon, Feb 20, 2023 at 11:21:46AM +0100, Andrea Bolognani wrote:
+sc_prohibit_python_without_env: + @prohibit='#!/usr/.*/py''thon' \
Shouldn't this be just '#!/usr/.*/python' ?
Yes, but then the syntax-check rule would flag this very line :)
We could do the "exempt from syntax-check" dance, but that feels unnecessarily complicated in this scenario. sc_copyright_usage already uses a similar trick to prevent the regex to match itself.
Reviewed-by: Erik Skultety <eskultet@redhat>
Missing the .com here, you might have a misconfiguration somewhere.
Anyway, thanks for the review! Can you please ACK the first patch in the series too? I can't push this one without it.
Sorry, I thought I replied to the cover letter. You can assume my RB for patch 1 as well. Erik
participants (2)
-
Andrea Bolognani -
Erik Skultety