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

[Mattijs Korpershoek <mkorpershoek@kernel.org>]

Hi Quentin,

Thank you for the review.

On mer., avril 16, 2025 at 14:47, Quentin Schulz <quentin.schulz@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@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".

>
> 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.

I'd also argue that this would more be the candidate for another patch
than to be part of this one.

>
> Cheers,
> Quentin