Re: [PATCH v2 3/3] ci: install python on ubuntu

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Thu, Nov 24 2022, Jiang Xin wrote:

> On Thu, Nov 24, 2022 at 7:06 PM Ævar Arnfjörð Bjarmason
> <avarab@gmail.com> wrote:
>>
>>
>> On Thu, Nov 24 2022, Jiang Xin wrote:
>>
>> > From: Jiang Xin <zhiyou.jx@alibaba-inc.com>
>> >
>> > Python is missing from the default ubuntu-22.04 runner image, which
>> > prevent git-p4 from working. To install python on ubuntu, we need to
>> > provide correct package name:
>> >
>> >  * On Ubuntu 18.04 (bionic), "/usr/bin/python2" is provided by the
>> >    "python" package, and "/usr/bin/python3" is provided by the "python3"
>> >    package.
>> >
>> >  * On Ubuntu 20.04 (focal) and above, "/usr/bin/python2" is provided by
>> >    the "python2" package which has a different name from bionic, and
>> >    "/usr/bin/python3" is provided by "python3".
>> >
>> > Since the "ubuntu-latest" runner image has a higher version, so its safe
>> > to use "python2" or "python3" package name.
>> >
>> > Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
>> > ---
>> >  ci/install-dependencies.sh | 2 +-
>> >  ci/lib.sh                  | 2 ++
>> >  2 files changed, 3 insertions(+), 1 deletion(-)
>> >
>> > diff --git a/ci/install-dependencies.sh b/ci/install-dependencies.sh
>> > index 291e49bdde..e28d93a154 100755
>> > --- a/ci/install-dependencies.sh
>> > +++ b/ci/install-dependencies.sh
>> > @@ -15,7 +15,7 @@ case "$runs_on_os" in
>> >  ubuntu)
>> >       sudo apt-get -q update
>> >       sudo apt-get -q -y install language-pack-is libsvn-perl apache2 \
>> > -             $UBUNTU_COMMON_PKGS $CC_PACKAGE
>> > +             $UBUNTU_COMMON_PKGS $CC_PACKAGE $PYTHON_PACKAGE
>> >       mkdir --parents "$P4_PATH"
>> >       pushd "$P4_PATH"
>> >               wget --quiet "$P4WHENCE/bin.linux26x86_64/p4d"
>> > diff --git a/ci/lib.sh b/ci/lib.sh
>> > index a618d66b81..ebe702e0ea 100755
>> > --- a/ci/lib.sh
>> > +++ b/ci/lib.sh
>> > @@ -235,8 +235,10 @@ ubuntu)
>> >       if [ "$jobname" = linux-gcc ]
>> >       then
>> >               MAKEFLAGS="$MAKEFLAGS PYTHON_PATH=/usr/bin/python3"
>> > +             PYTHON_PACKAGE=python3
>> >       else
>> >               MAKEFLAGS="$MAKEFLAGS PYTHON_PATH=/usr/bin/python2"
>> > +             PYTHON_PACKAGE=python2
>> >       fi
>>
>> Let's not copy/paste and repeat ourselves here for no reason. Part of
>> this is pre-existing, but if you just re-arrange these variable decls
>> you can do this instead:
>>
>>         PYTHON_PACKAGE=python2
>>         if test "$jobname" = linux-gcc
>>         then
>>                 PYTHON_PACKAGE=python3
>>         fi
>>         MAKEFLAGS="$MAKEFLAGS PYTHON_PATH=/usr/bin/${PYTHON_PACKAGE}"
>
> That was exactly my first edition, but I thought it was weird to write
> as "/usr/bin/${PYTHON_PACKAGE}". But if use two variables like
> PYTHON_BINARY and PYTHON_PACKAGE, looks even more silly. So I choose
> current solution.

I don't mind if you go for your inital version, it's not much
duplication, but why does it look silly? I don't think we need to worry
that the <package-name> on Ubuntu (and Debian) won't have a 1=1 mapping
to the /usr/bin/<package-name>. So defining the path in terms of the
package name seems like an obvious thing to do.