[PATCH v2] tools/make_pip: Use venv when invoking pip

[Mattijs Korpershoek]

On mer., avril 16, 2025 at 16:26, Quentin Schulz <quentin.schulz at cherry.de> wrote:

> Hi Mattijs,
>
> On 4/16/25 4:21 PM, Mattijs Korpershoek wrote:
>> Hi Quentin,
>> 
>> Thank you for the review.
>> 
>> On mer., avril 16, 2025 at 14:47, Quentin Schulz <quentin.schulz at cherry.de> wrote:
>> 
>>> Hi Mattijs,
>>>
>>> On 4/16/25 2:36 PM, Mattijs Korpershoek wrote:
>>>> Recent Ubuntu versions (24.04+) disallow pip by default when
>>>> installing packages. The recommended approach is to use a virtual
>>>> environment (venv) instead.
>>>> Because of this, "make pip" is failing on such versions.
>>>>
>>>> To prepare CI container migration to Ubuntu 24.04, use a venv in the
>>>> make_pip script.
>>>>
>>>> Note: This has been reported on [1]
>>>>
>>>> [1] https://source.denx.de/u-boot/custodians/u-boot-dm/-/issues/37
>>>>
>>>> Signed-off-by: Mattijs Korpershoek <mkorpershoek at kernel.org>
>>>> ---
>>>> This has been tested in docker on ubuntu:24.04 after running:
>>>> $ apt install python3 python3-venv
>>>>
>>>> with:
>>>> $ ./scripts/make_pip.sh u_boot_pylib "-n"
>>>>
>>>> And shows:
>>>> Successfully built u_boot_pylib-0.0.6.tar.gz and u_boot_pylib-0.0.6-py3-none-any.whl
>>>>
>>>> Also tested with "$ make pip".
>>>> ---
>>>> Changes in v2:
>>>> - Use venv instead of virtualenv (Tom)
>>>> - Link to v1: https://lore.kernel.org/r/20250409-ubuntu-24-04-v1-1-056728207b6c@kernel.org
>>>> ---
>>>>    scripts/make_pip.sh | 6 ++++++
>>>>    1 file changed, 6 insertions(+)
>>>>
>>>> diff --git a/scripts/make_pip.sh b/scripts/make_pip.sh
>>>> index d2639ffd6e43..33ad51ada703 100755
>>>> --- a/scripts/make_pip.sh
>>>> +++ b/scripts/make_pip.sh
>>>> @@ -106,6 +106,10 @@ fi
>>>>    mkdir ${dir}/tests
>>>>    cd ${dir}
>>>>    
>>>> +# Use virtual environment
>>>> +python3 -m venv .venv
>>>
>>> Do we want to make sure there are no leftovers from previous runs? If
>>> so, maybe add --clear here?
>> 
>> I've thought about this, and looking at the script itself it already
>> suffers from those issues. For example, the $dir created with mktemp is
>> also not removed.
>> 
>>>
>>>> +source .venv/bin/activate
>>>> +
>>>>    # Make sure the tools are up to date
>>>>    python3 -m pip install --upgrade build
>>>>    python3 -m pip install --upgrade twine
>>>> @@ -122,6 +126,8 @@ if [ -n "${upload}" ]; then
>>>>    	echo "Completed upload of ${tool}"
>>>>    fi
>>>>    
>>>> +# Finish using virtual environment
>>>> +deactivate
>>>
>>> Maybe you want this in a trap so that if we exit early (which may happen
>>> due to the script doing `set -e`), we're out of the venv?
>>>
>>> And I tested otherwise, with:
>>>
>>> #!/usr/bin/env bash
>>> python3 -m venv .venv
>>> source .venv/bin/activate
>>>
>>> and it seems that once I exit the script it's deactivated anyways?
>> 
>> Ah, interesting to know. I just added it to have a "symetric cleanup".
>> 
>
> Yup, absolutely makes sense.
>
>>>
>>> So, I guess no need for the trap or the deactivate, but we can play it
>>> safe and still have it :)
>> 
>> I think using a trap makes sense, but this should also be done for the
>> ${dir} directory.
>> 
>
> Indeed.
>
>> I'd also argue that this would more be the candidate for another patch
>> than to be part of this one.
>> 
>
> Fair enough. I believe the --clear addition to python3 -m venv .venv 
> would still make sense though? What do you think?

Hmm I'm a bit mixed about that one:
- .venv will be in a folder created via "mktemp -d"
- this folder will be different each time we run the script

So per my understanding, adding the --clear is not necessary here.
However, I don't feel strongly about not adding it. So if you wish, I
can spin up a v3 !

>
> Cheers,
> Quentin