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

[Quentin Schulz]

Hi Mattijs,

On 4/16/25 5:06 PM, Mattijs Korpershoek wrote:
> 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
> 

I clearly have missed this was running from an "mktemp -d"'ed directory. 
So it's useless indeed :)

Reviewed-by: Quentin Schulz <quentin.schulz at cherry.de>

Thanks!
Quentin