.github: Ensure python3 -m pip is used in GitHub Actions

[cclauss]

Ensure that "python3 -m pip install " is used consistantly in GitHub Actions.

Find all .yml files in the .github/workflows directory and find all lines that contain " pip install " and ensure that they also contain "python3 -m pip install ".

If there is any line that has the first but not the second, then print the file name, line number, and the line. Raise an error if any non-compliant files are found.

% pytest # Improved format

Use 'python3 -m pip install' instead of 'pip install' (3 errors).
.github/workflows/test.yml:29: pip install -r requirements.txt
.github/workflows/test.yml:91: pip install lxml
.github/workflows/pylint.yml:30: pip install requests 'pylint<=3.3.7'

How was this tested?

1. localhost testing
2. This is a test that contains tests.
3. Create test_python3_minus_m_pip_in_gh_actions.py pymavlink#1095

This will run automatically once one of these pull requests has been merged.

• Allow pytest discovery to find and run Python tests #30361
• Tools: Add pre-commit hook to check copyright in ros2 files #30598

[peterbarker]

Errr... does this introduce a top-level "test" directory? That doesn't seem right

[khancyr]

why not a simple grep script like :

#!/bin/bash

# Exit if any command fails
set -e

# Fail if `pip` is used instead of `python3 -m pip`
bad_files=$(grep -rEn '(^|\s)(pip3?|pip3?)\s+(install|list|uninstall|freeze)' --include='*.yml' --include='*.py' --include='*.sh' . | grep -v 'python3\s\+-m\s\+pip')

if [[ -n "$bad_files" ]]; then
    echo "❌ Detected direct use of 'pip' instead of 'python3 -m pip':"
    echo "$bad_files"
    echo "Please use 'python3 -m pip' instead of 'pip'."
    exit 1
fi

exit 0

And register is as part of pre-commit :

- id: check-pip-usage
  name: Check for pip usage
  entry: .pre-commit-hooks/check-pip-usage.sh
  language: script
  files: \.(py|sh|yml)$

[amilcarlucas]

I like the idea of adding pytests to the ardupilot codebase. :)
@peterbarker what directory do you suggest then ?

[cclauss]

I moved this script from test/ to the pre-existing tests/ to track with pytest discovery rules and #30598.

I distrust bash scripts (especially with regex) as I find it difficult to see bugs in them and write tests. This is particularly true when scripts must run across multiple operating systems. Would the script above only process GitHub Actions, and would it pass the four tests of test_process_file()?

The idea of adding to pre-commit is a good one, but let's land it first and make sure everyone is happy with it.

[peterbarker]

Again, not really comfortable using the top-level tests directory for this content.

As covered on the DevCall, we were a bit surprised it existed, and its current use is for the gtest suite which is a rather different animal to these python tests.

Consider Tools/scripts/python_tests instead.

[peterbarker]

What versions of Python will this work back to?

[peterbarker]

... actually, it would be nice to have a linting step which picked up any use of Python features not available in whatever our minimum version of Python is

[cclauss]

https://docs.python.org/3/library/__future__.html#module-contents says Python 3.7b1

ruff check uses target-python to apply or relax lint rules.

[peterbarker]

Why skip the first line?

[cclauss]

start=1 does not skip line 1. It starts line_number at 1 so that the line_number of the first line is 1, not 0.

[peterbarker]

I'd like to apply this across the entire codebase, actually... don't want them slipping into environment install files, for example.

[cclauss]

I am becoming less and less interested in this rule because it actively encourages people to be lazy about using virtual environments and to pollute their system Pythons with dependencies that might span multiple projects. This can lead to dependency conflicts that are difficult to debug and crashes that cannot be replicated.

Applying this rule inside GHAs bugs me less because they are already virtual (Docker) environments, but doing this on developers' machines seems irresponsible because it will make system modifications that will be extremely difficult for them to undo.

[peterbarker]

Let's simplify this - just because you can doesn't mean you should. The indenting here makes this particularly bad as a construct!

Suggested change

	errors.extend(
	f"{yml_file}:{line_number}: {line}"
	for line_number, line in process_file(yml_file)
	)
	for line_number, line in process_file(yml_file):
	errors.append(f"{yml_file}:{line_number}: {line}")

[cclauss]

The proposed solution reduces function call overhead from once per failing line to once per failing file.

Runtime performance is an important consideration for code that we may choose to add as a pre-commit hook.

The indenting is done automatically, not manually.

% ruff rule FURB113 # repeated-append
[ ... ]
Why is this bad?

Consecutive calls to append can be less efficient than batching them into a single extend. Each append resizes the list individually, whereas an extend can resize the list once for all elements.
[ ... ]

[cclauss]

Please re-raise the issue of using /tests on #30361 and/or #30666 which is where that discussion belongs.

[cclauss]

Closing in favor of the bash script approach recommended at #30657 (comment)