patch, 2 partsUse fixed github-actions runner image

From: Jiang Xin <zhiyou.jx@alibaba-inc.com>

Today I found lots of CI errors of my private host git repository on GitHub. It is because the runner image "ubuntu-latest" of the CI jobs is ubuntu 22.04 (jammy) instead of ubuntu 20.04 (focal).

The upgrade of runner images is in progress, and my public forked repository still use 20.04 (focal) as "ubuntu-latest".

Two patches in this series try to protect our CI in advance.

New CI instance see below:

 * https://github.com/jiangxin/git/actions/runs/3532978329

--

Jiang Xin (2):
  github-actions: run gcc-8 on ubuntu-20.04 image
  ci: upgrade version of p4

 .github/workflows/main.yml | 16 ++++++++++++----
 ci/install-dependencies.sh | 10 +++++-----
 ci/lib.sh                  |  8 ++++----
 3 files changed, 21 insertions(+), 13 deletions(-)

From: Jiang Xin <zhiyou.jx@alibaba-inc.com>

GitHub starts to upgrade its runner image "ubuntu-latest" from version "ubuntu-20.04" to version "ubuntu-22.04". It will fail to find and install "gcc-8" package on the new runner image.

Change the runner images from "ubuntu-latest" to "ubuntu-20.04" in order to run with "gcc-8" as a dependency.

Instead of use the environment "$runs_on_pool" as below:

    case "$runs_on_pool" in
    ubuntu-20.04 | ubuntu-latest)
	;;

we can reuse the os field in the matrix, and use a new environment "$runs_on_os" as below:

    case "$runs_on_os" in
    ubuntu)
        ;;

Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
---
 .github/workflows/main.yml | 16 ++++++++++++----
 ci/install-dependencies.sh |  6 +++---
 ci/lib.sh                  |  6 +++---
 3 files changed, 18 insertions(+), 10 deletions(-)

Hi,

On Wed, 23 Nov 2022, Jiang Xin wrote:

While this is a bit more refactoring than I usually recommend when trying to fix an urgent bug, the outcome does look nice and correct.

Here is my

	Reviewed-by: Johannes Schindelin <johannes.schindelin@gmx.de>

Thank you for working on this! Dscho

From: Jiang Xin <zhiyou.jx@alibaba-inc.com>

There would be a segmentation fault when running p4 v16.2 on ubuntu 22.04 which is the latest version of ubuntu runner image for github actions. Upgrade p4 from version 16.2 to 19.2 will fix this issue.

Also add some instructions to show errors of command "p4 -V", so we can see why the output doesn't match.

Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
---
 ci/install-dependencies.sh | 4 ++--
 ci/lib.sh                  | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

Hi,

On Wed, 23 Nov 2022, Jiang Xin wrote:

I was about to embark on a debugging session when I had the splendid idea to look at the Git mailing list archives to see whether anybody else had encountered that particular problem.

However, when I pushed a branch with this fix, it still segfaulted even when downloading version 19.2 (link: https://github.com/dscho/git/actions/runs/3538788474/jobs/5939977231#step:3:387):

 + wget --quiet https://cdist2.perforce.com/perforce/r19.2/bin.linux26x86_64/p4d
 + wget --quiet https://cdist2.perforce.com/perforce/r19.2/bin.linux26x86_64/p4
 + [...]
 + echo 'Perforce Server Version'
 Perforce Server Version
 + p4d -V
 + grep Rev.
 + echo 'p4d: bad version'
 p4d: bad version
 + p4d -V
 ci/install-dependencies.sh: line 91:  3051 Segmentation fault      (core
 dumped) p4d -V

I guess I'll embark on that debugging session after all ;-)

This is an excellent addition. If I read the logs correctly, this change adds the error message "Segmentation fault" to the output, which is really helpful.

Thank you for working on this! Dscho

Hi,

On Thu, 24 Nov 2022, Johannes Schindelin wrote:

And I did. It turns out that r22.2 works both on ubuntu-20.04 and ubuntu-22.04 (I locally verified for the former, proof for the latter: https://github.com/dscho/git/actions/runs/3538941550/jobs/5940295721#step:3:384). So I propose this fixup:

-- snip --
From 81e2d9a4f6ad2d2d9df27edfb666fe2112bdef57 Mon Sep 17 00:00:00 2001
From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Thu, 24 Nov 2022 09:31:41 +0100
Subject: [PATCH] amend! ci: upgrade version of p4

ci: upgrade version of p4

There would be a segmentation fault when running p4 v16.2 on ubuntu 22.04 which is the latest version of ubuntu runner image for github actions. Upgrade p4 from version 16.2 to 22.2 will fix this issue.

Also add some instructions to show errors of command "p4 -V", so we can see why the output doesn't match.

Reviewed-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
---
 ci/lib.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Me again,

On Thu, 24 Nov 2022, Johannes Schindelin wrote:

Hmm. Hold on. While `p4d` now no longer segfaults, it looks as if `git p4` is completely broken (see https://github.com/dscho/git/actions/runs/3538941550/jobs/5940295721#step:4:2005):

  failure: t9800.3 basic git p4 clone
  	git p4 clone --dest="$git" //depot &&
  	test_when_finished cleanup_git &&
  	(
  		cd "$git" &&
  		git log --oneline >lines &&
  		test_line_count = 1 lines
  	)

  + git p4 clone --dest=/home/runner/work/git/git/t/trash directory.t9800-git-p4-basic/git //depot
  Perforce db files in '.' will be created if missing...
  fatal: 'p4' appears to be a git command, but we were not
  able to execute it. Maybe git-p4 is broken?
  error: last command exited with $?=128
  not ok 3 - basic git p4 clone
  #
  #		git p4 clone --dest="$git" //depot &&
  #		test_when_finished cleanup_git &&
  #		(
  #			cd "$git" &&
  #			git log --oneline >lines &&
  #			test_line_count = 1 lines
  #		)
  #

I guess I will keep digging, Dscho

On Thu, Nov 24, 2022 at 4:54 PM Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:

I found this is because python is not installed by default on ubuntu-22.04. This issue is fix in patch 3/3 of reroll v2.

-- Jiang Xin

Hi,

On Thu, 24 Nov 2022, Jiang Xin wrote:

Excellent find!

I was pulling out my hair because I could not reproduce the issue, but then I did not override `PYTHON_PATH` in my build and therefore the already-installed python3 was used.

FWIW I confirm that this fixes the build. I restricted the build to the necessary minimum (and threw in an `action-tmate` step for more efficient debugging) and it passed the p4 tests: https://github.com/dscho/git/actions/runs/3539338333/jobs/5941122554

Thank you so much! Dscho

On Thu, Nov 24, 2022 at 4:16 PM Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:

Yes, p4d 19.2 also has segfault errors, and p4 version 21.x and above is OK. Maybe yesterday I only check p4, and forgot to check p4d by hands.

It is fixed in v2 by upgrade p4 to 21.2.

-- Jiang Xin

Hi,

On Wed, 23 Nov 2022, Jiang Xin wrote:

Excellent timing! This is indeed what I, too, ran into when adjusting microsoft/git to v2.39.0-rc0. Here are the relevant links for the failing and the succeeding runs, for the very same commit, but in different repositories and with different `ubuntu-*` images:

Failing (ubuntu-22.04): https://github.com/dscho/git/actions/runs/3533530319/jobs/5939252441#step:1:8

Succeeding (ubuntu-20.04): https://github.com/microsoft/git/actions/runs/3532815597/jobs/5927646267#step:1:9

Having said that, I applied your fixes on top and pushed out a test branch, but `p4d` still segfaults for me:

https://github.com/dscho/git/actions/runs/3538788474/jobs/5939977231#step:3:387

I hope that we can get this fixed very soon and am _very much_ in favor of fast-tracking the patches once we confirm that they fix the CI builds.

Thanks, Dscho

From: Jiang Xin <zhiyou.jx@alibaba-inc.com>

GitHub CI runner image "ubuntu-latest" will upgrade to "ubuntu-22.04" soon, and the CI runner image of my private host repository has already been upgraded.

Our CI will break on new runner image because there is not "gcc-8" package in ubuntu-22.04. See log of CI#2:

 * https://github.com/jiangxin/git-ci-test/actions/runs/3537628107/jobs/5937769957

This issue is fixed in patch 1/3 by downgrade the version of the runner image for jobs which need "gcc-8". But there are still some CI errors. This is because p4/p4d version 16.2 cannot run on ubuntu-22.04. See this log of CI#3:

 * https://github.com/jiangxin/git-ci-test/actions/runs/3537650146/jobs/5937813922

This issue is fixed in patch 2/3 by upgrade p4 and p4d. But all p4 related test cases failed becasue python was missing on ubuntu-22.04. See the log of CI#4:

 * https://github.com/jiangxin/git-ci-test/actions/runs/3537695959/jobs/5937929695

This issue is fixed in patch 3/3 by install python2/python3 on ubutnu.

If install p4 version 22.2, will break several p4 related test cases, see the log of CI#7:

 * https://github.com/jiangxin/git-ci-test/actions/runs/3538795233/jobs/5939989823

So we choose p4 21.2, and the final successful log of CI#8 is below:

 * https://github.com/jiangxin/git-ci-test/actions/runs/3538946849

--
Jiang Xin (3):
  github-actions: run gcc-8 on ubuntu-20.04 image
  ci: upgrade version of p4 to 21.2
  ci: install python on ubuntu

 .github/workflows/main.yml | 16 ++++++++++++----
 ci/install-dependencies.sh | 12 ++++++------
 ci/lib.sh                  | 10 ++++++----
 3 files changed, 24 insertions(+), 14 deletions(-)

Hi,

On Thu, 24 Nov 2022, Jiang Xin wrote:

ACK!

I verified that this patch series does the job, by applying it on top of `microsoft/git`'s tentative rebase to v2.39.0-rc0: https://github.com/dscho/git/actions/runs/3539338333/jobs/5941122554

This run was restricted to `linux-clang` and the p4 tests. I care a lot about not using more resources than one's fair share, therefore I wanted to avoid using build minutes unnecessarily during my debuging.

To build even more confidence in the patch series, I will now start a full run (which will take *a lot* of build minutes, unfortunately).

Thank you so much! Dscho

Hi,

On Thu, 24 Nov 2022, Johannes Schindelin wrote:

And it passed: https://github.com/dscho/git/actions/runs/3539451056

I also had a look at the range-diff between v1 and v2:

-- snip --
1:  ef80c39de1e5 ! 1:  6d4607a4ee46 github-actions: run gcc-8 on ubuntu-20.04 image
    @@ Commit message
             ubuntu)
                 ;;

    +    In this way, we can change the "ubuntu-latest" runner image to any
    +    version such as "ubuntu-22.04" to test CI behavior in advance.
    +
         Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>

2:  1d0903c8b2f9 ! 2:  eba96648368a ci: upgrade version of p4
    @@ Metadata
     Author: Jiang Xin <zhiyou.jx@alibaba-inc.com>

      ## Commit message ##
    -    ci: upgrade version of p4
    +    ci: upgrade version of p4 to 21.2

         There would be a segmentation fault when running p4 v16.2 on ubuntu
         22.04 which is the latest version of ubuntu runner image for github
    -    actions. Upgrade p4 from version 16.2 to 19.2 will fix this issue.
    +    actions.

    -    Also add some instructions to show errors of command "p4 -V", so we can
    -    see why the output doesn't match.
    +    By checking each version from [1], p4d version 21.1 and above can work
    +    properly on ubuntu 22.04. But version 22.x will break some p4 test
    +    cases. So p4 version 21.x is exactly the version we can use.
    +
    +    In addition to upgrade p4 from version 16.2 to 21.2, also add some
    +    instructions to show errors of command "p4 -V", so we can see why the
    +    command output doesn't match.
    +
    +    [1]: https://cdist2.perforce.com/perforce/

         Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
    @@ ci/lib.sh: ubuntu)
      	# image.
      	# Keep that in mind when you encounter a broken OS X build!
     -	export LINUX_P4_VERSION="16.2"
    -+	export LINUX_P4_VERSION="19.2"
    ++	export LINUX_P4_VERSION="21.2"
      	export LINUX_GIT_LFS_VERSION="1.5.2"

      	P4_PATH="$HOME/custom/p4"
-:  ------------ > 3:  8e432f13bef8 ci: install python on ubuntu
-- snap --

The changes look good!

One alternative I considered about 8e432f13bef8 (ci: install python on ubuntu, 2022-11-24) was to drop testing Python v2.x (it's years past end of life after all, see https://endoflife.date/python).

However, as we see frequently demonstrated on the Git mailing list: losing focus, trying to force patch series to address more than a single concern, gets us nowhere, and not even fast.

So I agree that the best idea in this patch series is the stop-gap solution to install `python2` on `ubuntu-22.04`, and deal with deprecating Python v2.x support separately, later, or never, whichever comes first ;-)

With this, here goes my (probably final, because I think this patch series is ready):

	Reviewed-by: Johannes Schindelin <johannes.schindelin@gmx.de>

Thank you so much for working on this, it was a thoroughly enjoyable experience for me!

Ciao, Dscho

On Thu, Nov 24, 2022 at 6:48 PM Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:

Thank you for providing this range-diff.

-- Jiang Xin

On Thu, Nov 24 2022, Johannes Schindelin wrote:

Just 2 years? :)

We're still pinning "perl" to a supported 5.8, which is so out-of-life that I couldn't find a good reference to when exactly it went EOL. My guess based on [1] and "perldoc perlhist" is sometime around 2009-2010.

Yes, let's address that later. We had a recent discussion relating to EOL-ing it in-tree. See the ML discussion around[2].

I would like to note that you seem to be assuming that upstream's EOL for something like this should match our EOL for supporting the software.

I think one could argue that, but that's not at all the stance we've taken in the past, as the "perl" example shows.

I've personally wanted to bump the "perl" dependency more aggressively for purely selfish reasons in the past (being able to use some newer feature), but the reality is that people "back-port" newer git versions onto various older platforms.

But in terms of the cost-benefit of the disruption that would incur I also don't think it's worth it (although a bit past 5.8 is probably justifiable at this point).

Someone using "perl" on an older system for git's tests and git-svn etc. really doesn't need to worry much about the full security surface that "perl" might provide, which includes e.g. all the CPAN libraries it ships with, I expect that the same would go for "python".

The one potential security issue I can think of that we've ever had because of it is that you could trick "gitweb" and the like into ever-growing memory use if you had a perl version older than the "hash randomization" security/DoS fix.

But other than that we're not exposing perl, python etc. directly over the Internet, so I think we don't need to be too paranoid about it.

1. https://endoflife.date/perl
2. f7b5ff607fa (git-p4: improve encoding handling to support
   inconsistent encodings, 2022-04-30)

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

I agree with two of you that a good first step is the posted patch.

I do not seem to find any message in the thread that led to the commit about dropping Python 2 support, though.

Thanks.

From: Jiang Xin <zhiyou.jx@alibaba-inc.com>

# Changes since v2

1. Split patch 2/3 of v2 into two patches:

   patch 2/4: ci: show error message of "p4 -V"
   patch 3/4: ci: p4 on Linux has the same version as on macOS

2. In patch 3/4, remove variable "LINUX_P4_VERSION" and use
   a fixed version "21.2" for p4 on both Linux and macOS.

3. For patch 4/4, follow Ævar's suggestion to make the code simpler.

# Range-diff v2..v3

1:  79c851529c ! 1:  1c0d639487 github-actions: run gcc-8 on ubuntu-20.04 image
    @@ Commit message
         In this way, we can change the "ubuntu-latest" runner image to any
         version such as "ubuntu-22.04" to test CI behavior in advance.
     
    +    Reviewed-by: Johannes Schindelin <johannes.schindelin@gmx.de>
         Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
     
      ## .github/workflows/main.yml ##
2:  faa5076fe0 ! 2:  fed880b8bd ci: upgrade version of p4 to 21.2
    @@ Metadata
     Author: Jiang Xin <zhiyou.jx@alibaba-inc.com>
     
      ## Commit message ##
    -    ci: upgrade version of p4 to 21.2
    +    ci: show error message of "p4 -V"
     
    -    There would be a segmentation fault when running p4 v16.2 on ubuntu
    -    22.04 which is the latest version of ubuntu runner image for github
    -    actions.
    +    When installing p4 as a dependency, we used to pipe output of "p4 -V" to
    +    validate the installation, but this would hide potential errors of p4.
    +    E.g.: A broken p4 installation fails to run.
     
    -    By checking each version from [1], p4d version 21.1 and above can work
    -    properly on ubuntu 22.04. But version 22.x will break some p4 test
    -    cases. So p4 version 21.x is exactly the version we can use.
    -
    -    In addition to upgrade p4 from version 16.2 to 21.2, also add some
    -    instructions to show errors of command "p4 -V", so we can see why the
    -    command output doesn't match.
    -
    -    [1]: https://cdist2.perforce.com/perforce/
    +    Add some instructions to show errors of command "p4 -V", so we can see
    +    why the command output doesn't match.
     
    +    Reviewed-by: Johannes Schindelin <johannes.schindelin@gmx.de>
         Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
     
      ## ci/install-dependencies.sh ##
    @@ ci/install-dependencies.sh: esac
      else
      	echo >&2 "WARNING: perforce wasn't installed, see above for clues why"
      fi
    -
    - ## ci/lib.sh ##
    -@@ ci/lib.sh: ubuntu)
    - 	# were recorded in the Homebrew database upon creating the OS X
    - 	# image.
    - 	# Keep that in mind when you encounter a broken OS X build!
    --	export LINUX_P4_VERSION="16.2"
    -+	export LINUX_P4_VERSION="21.2"
    - 	export LINUX_GIT_LFS_VERSION="1.5.2"
    - 
    - 	P4_PATH="$HOME/custom/p4"
-:  ---------- > 3:  da2f516fc9 ci: p4 on Linux has the same version as on macOS
3:  f080b2fdcd ! 4:  bd1850cc93 ci: install python on ubuntu
    @@ Commit message
         Since the "ubuntu-latest" runner image has a higher version, so its safe
         to use "python2" or "python3" package name.
     
    +    Helped-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
         Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
     
      ## ci/install-dependencies.sh ##
    @@ ci/install-dependencies.sh: case "$runs_on_os" in
     
      ## ci/lib.sh ##
     @@ ci/lib.sh: ubuntu)
    - 	if [ "$jobname" = linux-gcc ]
    + 		break
    + 	fi
    + 
    +-	if [ "$jobname" = linux-gcc ]
    ++	PYTHON_PACKAGE=python2
    ++	if test "$jobname" = linux-gcc
      	then
    - 		MAKEFLAGS="$MAKEFLAGS PYTHON_PATH=/usr/bin/python3"
    +-		MAKEFLAGS="$MAKEFLAGS PYTHON_PATH=/usr/bin/python3"
    +-	else
    +-		MAKEFLAGS="$MAKEFLAGS PYTHON_PATH=/usr/bin/python2"
     +		PYTHON_PACKAGE=python3
    - 	else
    - 		MAKEFLAGS="$MAKEFLAGS PYTHON_PATH=/usr/bin/python2"
    -+		PYTHON_PACKAGE=python2
      	fi
    ++	MAKEFLAGS="$MAKEFLAGS PYTHON_PATH=/usr/bin/$PYTHON_PACKAGE"
      
      	export GIT_TEST_HTTPD=true
    + 

--

Jiang Xin (4):
  github-actions: run gcc-8 on ubuntu-20.04 image
  ci: show error message of "p4 -V"
  ci: p4 on Linux has the same version as on macOS
  ci: install python on ubuntu

 .github/workflows/main.yml | 16 ++++++++++++----
 ci/install-dependencies.sh | 16 ++++++++--------
 ci/lib.sh                  | 15 +++++++--------
 3 files changed, 27 insertions(+), 20 deletions(-)

From: Jiang Xin <zhiyou.jx@alibaba-inc.com>

GitHub CI runner image "ubuntu-latest" will upgrade to "ubuntu-22.04" soon. Our CI will break on the new runner image because there is not "gcc-8" package in ubuntu-22.04. See log of CI#10:

 * https://github.com/jiangxin/git-ci-test/actions/runs/3546701459/jobs/5956020103#step:3:106

Another issue in CI#10 is "pd4 -V” failed without an error output. See:

 * https://github.com/jiangxin/git-ci-test/actions/runs/3546701459/jobs/5956019973#step:3:315

This first issue is fixed in patch 1/4 by downgrading the version of the runner image for jobs which need "gcc-8". The second issue is fixed in patch 2/4 by removing the pipe from "p4d -V". Then, we can see there was a sigfault when running "p4d -V". See log of CI#11:

 * https://github.com/jiangxin/git-ci-test/actions/runs/3546723251/jobs/5956063536#step:3:311

This issue is fixed in patch 3/4 by upgrading version of p4 on Linux to the same version as on macOS. But all p4 related test cases failed becasue python was missing on ubuntu-22.04. See the log of CI#12:

 * https://github.com/jiangxin/git-ci-test/actions/runs/3546734918/jobs/5956087614#step:4:22733

This issue is fixed in patch 4/4 by installing python2/python3 on Linux. All test cases passed. See log of CI#13:

 * https://github.com/jiangxin/git-ci-test/actions/runs/3546744257

# range-diff v3..v4

1:  1c0d639487 ! 1:  28588dad56 github-actions: run gcc-8 on ubuntu-20.04 image
    @@ Commit message
         "ubuntu-20.04" to version "ubuntu-22.04". It will fail to find and
         install "gcc-8" package on the new runner image.
     
    -    Change the runner images from "ubuntu-latest" to "ubuntu-20.04" in order
    -    to run with "gcc-8" as a dependency.
    +    Change some of the runner images from "ubuntu-latest" to "ubuntu-20.04"
    +    in order to install "gcc-8" as a dependency.
     
    -    Instead of use the environment "$runs_on_pool" as below:
    +    The first revision of this patch tried to replace "$runs_on_pool" in
    +    "ci/*.sh" with a new "$runs_on_os" environment variable based on the
    +    "os" filed in the matrix strategy. But these "os" fields in matrix
    +    strategies are obsolete legacies from commit [1] and commit [2], and
    +    are no longer useful. So remove these unused "os" fields.
     
    -        case "$runs_on_pool" in
    -        ubuntu-20.04 | ubuntu-latest)
    -            ;;
    -
    -    we can reuse the os field in the matrix, and use a new environment
    -    "$runs_on_os" as below:
    -
    -        case "$runs_on_os" in
    -        ubuntu)
    -            ;;
    -
    -    In this way, we can change the "ubuntu-latest" runner image to any
    -    version such as "ubuntu-22.04" to test CI behavior in advance.
    +    [1]: c08bb26010 (CI: rename the "Linux32" job to lower-case "linux32",
    +                     2021-11-23)
    +    [2]: 25715419bf (CI: don't run "make test" twice in one job, 2021-11-23)
     
         Reviewed-by: Johannes Schindelin <johannes.schindelin@gmx.de>
    +    Helped-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
         Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
     
      ## .github/workflows/main.yml ##
     @@ .github/workflows/main.yml: jobs:
    -         vector:
    -           - jobname: linux-clang
    -             cc: clang
    -+            os: ubuntu
                  pool: ubuntu-latest
                - jobname: linux-sha256
                  cc: clang
    -@@ .github/workflows/main.yml: jobs:
    +-            os: ubuntu
    +             pool: ubuntu-latest
                - jobname: linux-gcc
                  cc: gcc
                  cc_package: gcc-8
     -            pool: ubuntu-latest
    -+            os: ubuntu
     +            pool: ubuntu-20.04
                - jobname: linux-TEST-vars
                  cc: gcc
     -            os: ubuntu
                  cc_package: gcc-8
     -            pool: ubuntu-latest
    -+            os: ubuntu
     +            pool: ubuntu-20.04
                - jobname: osx-clang
                  cc: clang
    -+            os: macos
                  pool: macos-latest
    -           - jobname: osx-gcc
    -             cc: gcc
    -             cc_package: gcc-9
    -+            os: macos
    -             pool: macos-latest
    -           - jobname: linux-gcc-default
    -             cc: gcc
    -+            os: ubuntu
    -             pool: ubuntu-latest
    -           - jobname: linux-leaks
    -             cc: gcc
    -+            os: ubuntu
    -             pool: ubuntu-latest
    -           - jobname: linux-asan
    -             cc: gcc
    -+            os: ubuntu
    -             pool: ubuntu-latest
    -           - jobname: linux-ubsan
    -             cc: gcc
    -+            os: ubuntu
    -             pool: ubuntu-latest
    -     env:
    -       CC: ${{matrix.vector.cc}}
    -       CC_PACKAGE: ${{matrix.vector.cc_package}}
    -       jobname: ${{matrix.vector.jobname}}
    --      runs_on_pool: ${{matrix.vector.pool}}
    -+      runs_on_os: ${{matrix.vector.os}}
    -     runs-on: ${{matrix.vector.pool}}
    -     steps:
    -     - uses: actions/checkout@v2
    +@@ .github/workflows/main.yml: jobs:
    +         - jobname: linux-musl
    +           image: alpine
    +         - jobname: linux32
    +-          os: ubuntu32
    +           image: daald/ubuntu32:xenial
    +         - jobname: pedantic
    +           image: fedora
     
      ## ci/install-dependencies.sh ##
     @@ ci/install-dependencies.sh: UBUNTU_COMMON_PKGS="make libssl-dev libcurl4-openssl-dev libexpat-dev
    -  tcl tk gettext zlib1g-dev perl-modules liberror-perl libauthen-sasl-perl
       libemail-valid-perl libio-socket-ssl-perl libnet-smtp-ssl-perl"
      
    --case "$runs_on_pool" in
    + case "$runs_on_pool" in
     -ubuntu-latest)
    -+case "$runs_on_os" in
    -+ubuntu)
    ++ubuntu-*)
      	sudo apt-get -q update
      	sudo apt-get -q -y install language-pack-is libsvn-perl apache2 \
      		$UBUNTU_COMMON_PKGS $CC_PACKAGE
    -@@ ci/install-dependencies.sh: ubuntu-latest)
    - 		cp git-lfs-$LINUX_GIT_LFS_VERSION/git-lfs .
    - 	popd
    - 	;;
    --macos-latest)
    -+macos)
    - 	export HOMEBREW_NO_AUTO_UPDATE=1 HOMEBREW_NO_INSTALL_CLEANUP=1
    - 	# Uncomment this if you want to run perf tests:
    - 	# brew install gnu-time
     
      ## ci/lib.sh ##
    -@@ ci/lib.sh: export DEFAULT_TEST_TARGET=prove
    - export GIT_TEST_CLONE_2GB=true
    +@@ ci/lib.sh: export GIT_TEST_CLONE_2GB=true
      export SKIP_DASHED_BUILT_INS=YesPlease
      
    --case "$runs_on_pool" in
    + case "$runs_on_pool" in
     -ubuntu-latest)
    -+case "$runs_on_os" in
    -+ubuntu)
    ++ubuntu-*)
      	if test "$jobname" = "linux-gcc-default"
      	then
      		break
    -@@ ci/lib.sh: ubuntu-latest)
    - 	GIT_LFS_PATH="$HOME/custom/git-lfs"
    - 	export PATH="$GIT_LFS_PATH:$P4_PATH:$PATH"
    - 	;;
    --macos-latest)
    -+macos)
    - 	if [ "$jobname" = osx-gcc ]
    - 	then
    - 		MAKEFLAGS="$MAKEFLAGS PYTHON_PATH=$(which python3)"
2:  fed880b8bd ! 2:  b914f1d2fc ci: show error message of "p4 -V"
    @@ Metadata
     Author: Jiang Xin <zhiyou.jx@alibaba-inc.com>
     
      ## Commit message ##
    -    ci: show error message of "p4 -V"
    +    ci: remove the pipe after "p4 -V" to cache errors
     
    -    When installing p4 as a dependency, we used to pipe output of "p4 -V" to
    -    validate the installation, but this would hide potential errors of p4.
    -    E.g.: A broken p4 installation fails to run.
    +    When installing p4 as a dependency, we used to pipe output of "p4 -V"
    +    and "p4d -V" to validate the installation and output a condensed version
    +    information. But this would hide potential errors of p4 and would stop
    +    with an empty output. E.g.: p4d version 16.2 running on ubuntu 22.04
    +    causes sigfaults.
     
    -    Add some instructions to show errors of command "p4 -V", so we can see
    -    why the command output doesn't match.
    +    By removing the pipe after "p4 -V" and "p4d -V", we may get a verbose
    +    output, and stop immediately on errors becuase we have "set -e" in
    +    "ci/lib.sh". Since we won't look at these trace logs unless something
    +    fails, so just including the raw output seems most sensible.
     
         Reviewed-by: Johannes Schindelin <johannes.schindelin@gmx.de>
    +    Helped-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
         Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
     
      ## ci/install-dependencies.sh ##
    @@ ci/install-dependencies.sh: esac
      then
      	echo "$(tput setaf 6)Perforce Server Version$(tput sgr0)"
     -	p4d -V | grep Rev.
    -+	p4d -V | grep Rev. || { echo >&2 "p4d: bad version"; p4d -V; exit 1; }
    ++	p4d -V
      	echo "$(tput setaf 6)Perforce Client Version$(tput sgr0)"
     -	p4 -V | grep Rev.
    -+	p4 -V | grep Rev. || { echo >&2 "p4: bad version"; p4 -V; exit 1; }
    ++	p4 -V
      else
      	echo >&2 "WARNING: perforce wasn't installed, see above for clues why"
      fi
3:  da2f516fc9 = 3:  4aa2b30c36 ci: p4 on Linux has the same version as on macOS
4:  bd1850cc93 = 4:  35909fd459 ci: install python on ubuntu
      
--

Jiang Xin (4):
  github-actions: run gcc-8 on ubuntu-20.04 image
  ci: remove the pipe after "p4 -V" to cache errors
  ci: p4 on Linux has the same version as on macOS
  ci: install python on ubuntu

 .github/workflows/main.yml |  7 ++-----
 ci/install-dependencies.sh | 12 ++++++------
 ci/lib.sh                  | 11 +++++------
 3 files changed, 13 insertions(+), 17 deletions(-)

From: Jiang Xin <zhiyou.jx@alibaba-inc.com>

When installing p4 as a dependency, we used to pipe output of "p4 -V" and "p4d -V" to validate the installation and output a condensed version information. But this would hide potential errors of p4 and would stop with an empty output. E.g.: p4d version 16.2 running on ubuntu 22.04 causes sigfaults.

By removing the pipe after "p4 -V" and "p4d -V", we may get a verbose output, and stop immediately on errors becuase we have "set -e" in "ci/lib.sh". Since we won't look at these trace logs unless something fails, so just including the raw output seems most sensible.

Reviewed-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Helped-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
---
 ci/install-dependencies.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Jiang Xin <worldhello.net@gmail.com> writes:

"cache" -> "catch", I think.

... before it produces any output.

"because".

You started the sentence with "Since", so "so" should be dropped.

Well reasoned.  Will queue.  Thanks.

On Sun, Nov 27, 2022 at 8:24 AM Junio C Hamano <gitster@pobox.com> wrote:

oops, it's a typo. I used to copy and paste the commit log to MS word or Apple pages to check for typos, but this step is very cumbersome, and I always forget it.

I see this has been fixed in your tree, so there is no need to send v5.

-- Jiang Xin

From: Jiang Xin <zhiyou.jx@alibaba-inc.com>

GitHub starts to upgrade its runner image "ubuntu-latest" from version "ubuntu-20.04" to version "ubuntu-22.04". It will fail to find and install "gcc-8" package on the new runner image.

Change some of the runner images from "ubuntu-latest" to "ubuntu-20.04" in order to install "gcc-8" as a dependency.

The first revision of this patch tried to replace "$runs_on_pool" in "ci/*.sh" with a new "$runs_on_os" environment variable based on the "os" filed in the matrix strategy. But these "os" fields in matrix strategies are obsolete legacies from commit [1] and commit [2], and are no longer useful. So remove these unused "os" fields.

[1]: c08bb26010 (CI: rename the "Linux32" job to lower-case "linux32",
                 2021-11-23)
[2]: 25715419bf (CI: don't run "make test" twice in one job, 2021-11-23)

Reviewed-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Helped-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
---
 .github/workflows/main.yml | 7 ++-----
 ci/install-dependencies.sh | 2 +-
 ci/lib.sh                  | 2 +-
 3 files changed, 4 insertions(+), 7 deletions(-)

Jiang Xin <worldhello.net@gmail.com> writes:

"filed" -> "field".

Very well explained. With the patch text alone, readers would have been puzzled about the removal of "os" and the description certainly helps.

I also like the changes to ci/*.sh to match any future versions of "ubuntu-*" used in the pool name. It is a sensible way to future proofing.

Will queue.  Thanks, all.

From: Jiang Xin <zhiyou.jx@alibaba-inc.com>

There would be a segmentation fault when running p4 v16.2 on ubuntu 22.04 which is the latest version of ubuntu runner image for github actions.

By checking each version from [1], p4d version 21.1 and above can work properly on ubuntu 22.04. But version 22.x will break some p4 test cases. So p4 version 21.x is exactly the version we can use.

With this update, the versions of p4 for Linux and macOS happen to be the same. So we can add the version number directly into the "P4WHENCE" variable, and reuse it in p4 installation for macOS.

By removing the "LINUX_P4_VERSION" variable from "ci/lib.sh", the comment left above has nothing to do with p4, but still applies to git-lfs. Since we have a fixed version of git-lfs installed on Linux, we may have a different version on macOS.

[1]: https://cdist2.perforce.com/perforce/

Reviewed-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Helped-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
---
 ci/install-dependencies.sh | 4 ++--
 ci/lib.sh                  | 1 -
 2 files changed, 2 insertions(+), 3 deletions(-)

Jiang Xin <worldhello.net@gmail.com> writes:

Makes sense.  This needs to be retitled from a statement of the fact ...

... to description of a choice _we_ made, e.g.

    ci: use the same version of p4 on both Linux and macOS

or something like that.

Other than that, looks good.

Thanks.

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.

Helped-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
---
 ci/install-dependencies.sh | 2 +-
 ci/lib.sh                  | 8 ++++----
 2 files changed, 5 insertions(+), 5 deletions(-)

Jiang Xin <worldhello.net@gmail.com> writes:

Makes sense. Just out of curiousity (read: not a suggestion to change anything), what happens if you say "apt install python" on a recent system?

Will queue.  Thanks.

On Sun, Nov 27, 2022 at 8:30 AM Junio C Hamano <gitster@pobox.com> wrote:

In order to reproduce, I start a docker container like  this:

    $ docker run -it --rm ubuntu:22.04 /bin/bash

Fetch the latest apt source list, by running:

    $ apt-get update

Then install python with the following errors:

    $ apt-get install python
    Reading package lists... Done
    Building dependency tree... Done
    Reading state information... Done
    Package python is not available, but is referred to by another package.
This may mean that the package is missing, has been obsoleted, or
is only available from another source
However the following packages replace it:
  2to3 python2-minimal python2 dh-python python-is-python3

E: Package 'python' has no installation candidate

We can also find python package name from packages.ubuntu.com:

1. form the link below, we can say Ubuntu 18.04 is the last version
(LTS?) which has python package:

    https://packages.ubuntu.com/search?keywords=python&searchon=names&suite=all&section=all

2. Ubuntu 20.04 is the first version (LTS) which rename its python
pacakge to python2 package:

    https://packages.ubuntu.com/search?keywords=python2&searchon=names&suite=all&section=all

Thanks.

-- Jiang Xin

Jiang Xin <worldhello.net@gmail.com> writes:

OK, I see. Thanks. I wondered if "python" redirects to either one of these versions. Not that I would suggest another approach based on such a redirection, even if it existed--as I said, the question was purely out of curiosity.

From: Jiang Xin <zhiyou.jx@alibaba-inc.com>

GitHub starts to upgrade its runner image "ubuntu-latest" from version "ubuntu-20.04" to version "ubuntu-22.04". It will fail to find and install "gcc-8" package on the new runner image.

Change the runner images from "ubuntu-latest" to "ubuntu-20.04" in order to run with "gcc-8" as a dependency.

Instead of use the environment "$runs_on_pool" as below:

    case "$runs_on_pool" in
    ubuntu-20.04 | ubuntu-latest)
	;;

we can reuse the os field in the matrix, and use a new environment "$runs_on_os" as below:

    case "$runs_on_os" in
    ubuntu)
        ;;

In this way, we can change the "ubuntu-latest" runner image to any version such as "ubuntu-22.04" to test CI behavior in advance.

Reviewed-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
---
 .github/workflows/main.yml | 16 ++++++++++++----
 ci/install-dependencies.sh |  6 +++---
 ci/lib.sh                  |  6 +++---
 3 files changed, 18 insertions(+), 10 deletions(-)

On Thu, Nov 24 2022, Jiang Xin wrote:

At the end of [1] I suggested splitting up the refactoring part of this. I still think that would be a good thing to do, but just to comment on the *actual* change (which I was hoping a split-out would make clearer).

This is proposing to refactor "runs-on-pool={ubuntu-latest,macos-latest}" to "os={ubuntu,macos}". I think it's worth cleaning up how we match main.yml and ci/*.sh, but think it's a bit of a digression in this otherwise narrowly focused series.

Now, this seems to somewhat come back to being all my fault. AFAICT I added "os" in c08bb260105 (CI: rename the "Linux32" job to lower-case "linux32", 2021-11-23) as part of some WIP (or from an earlier version?) that ende dup doing nothing.

But looking at it again, I don't see how an "os" here makes much
sense. For "macos-latest" if we're going to refactor it let's just stop
selecting stuff based on this "pool name", and instead fold it into the
adjacent "jobname" selection:
	
	diff --git a/ci/install-dependencies.sh b/ci/install-dependencies.sh
	index 107757a1fea..b5a373060c1 100755
	--- a/ci/install-dependencies.sh
	+++ b/ci/install-dependencies.sh
	@@ -30,7 +30,9 @@ ubuntu-latest)
	 		cp git-lfs-$LINUX_GIT_LFS_VERSION/git-lfs .
	 	popd
	 	;;
	-macos-latest)
	+esac
	+case "$jobname" in
	+osx-*)
	 	export HOMEBREW_NO_AUTO_UPDATE=1 HOMEBREW_NO_INSTALL_CLEANUP=1
	 	# Uncomment this if you want to run perf tests:
	 	# brew install gnu-time
	diff --git a/ci/lib.sh b/ci/lib.sh
	index 24d20a5d648..fef60a507e9 100755
	--- a/ci/lib.sh
	+++ b/ci/lib.sh
	@@ -253,19 +253,17 @@ ubuntu-latest)
	 	GIT_LFS_PATH="$HOME/custom/git-lfs"
	 	export PATH="$GIT_LFS_PATH:$P4_PATH:$PATH"
	 	;;
	-macos-latest)
	-	if [ "$jobname" = osx-gcc ]
	-	then
	-		MAKEFLAGS="$MAKEFLAGS PYTHON_PATH=$(which python3)"
	-	else
	-		MAKEFLAGS="$MAKEFLAGS PYTHON_PATH=$(which python2)"
	-		MAKEFLAGS="$MAKEFLAGS NO_APPLE_COMMON_CRYPTO=NoThanks"
	-		MAKEFLAGS="$MAKEFLAGS NO_OPENSSL=NoThanks"
	-	fi
	-	;;
	 esac
	 
	 case "$jobname" in
	+osx-gcc)
	+	MAKEFLAGS="$MAKEFLAGS PYTHON_PATH=$(which python3)"
	+	;;
	+osx-clang)
	+	MAKEFLAGS="$MAKEFLAGS PYTHON_PATH=$(which python2)"
	+	MAKEFLAGS="$MAKEFLAGS NO_APPLE_COMMON_CRYPTO=NoThanks"
	+	MAKEFLAGS="$MAKEFLAGS NO_OPENSSL=NoThanks"
	+	;;
	 linux32)
	 	CC=gcc
	 	;;

That nicely sidesteps the need to refactor it here, and IMO makes it more future-proof.

For the "ubuntu" case I think it's even worse, because it now *looks* like a case that should be equivalent to just:

	test "$(lsb_release -si 2>&1 | grep -x Debian)" = "Debian"

But it's not, because we're not setting this "os=ubuntu" in all the places where the OS is Ubuntu. It really just means "the stuff that used to set env.runs_on_pool=ubuntu-latest, which e.g. doesn't cover the "documentation" job, which runs on "ubuntu-latest".

So I think *if* we're going to refactor this while-at-it, let's not make it "os=ubuntu", but e.g. "extra_ubuntu_packages: 1", which is really what it means. I.e. we're intercepting it in ci/lib.sh to set the "p4" variables etc., which we only need if we're doing the optional "apt install" on the "ubuntu-latest".

1. https://lore.kernel.org/git/221124.86mt8gpqra.gmgdl@evledraar.gmail.com/

From: Jiang Xin <zhiyou.jx@alibaba-inc.com>

When installing p4 as a dependency, we used to pipe output of "p4 -V" to validate the installation, but this would hide potential errors of p4. E.g.: A broken p4 installation fails to run.

Add some instructions to show errors of command "p4 -V", so we can see why the command output doesn't match.

Reviewed-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
---
 ci/install-dependencies.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

On Thu, Nov 24 2022, Jiang Xin wrote:

I think it makes sense to detect this, but the specific remedy makes no sense to me.

I.e. as your CL's CI output shows the reason we want this is because you had a p4d (and p4?) that segfaulted. I can reproduce that locally as:

	wget --quiet https://cdist2.perforce.com/perforce/r16.2/bin.linux26x86_64/p4d -O p4d; chmod +x p4d; ./p4d
	Segmentation fault

But we had the initial command piped into Rev, so we wouldn't show that, just

	$ wget --quiet https://cdist2.perforce.com/perforce/r16.2/bin.linux26x86_64/p4d -O p4d; chmod +x p4d; ./p4d | grep Rev; echo $?
	1

So we want to show that it segfaults, but how it is useful once that command fails to pretend that it's a "bad version?" The issue is that the command can't show the version at all.

	$ { echo >&2 "p4d: bad version"; ./p4d -V; exit 1; }
	p4d: bad version
	Segmentation fault
	exit

I think this should just be a plain:

	echo "$(tput setaf 6)Perforce Server Version$(tput sgr0)"
        p4d -V
        echo "$(tput setaf 6)Perforce Client Version$(tput sgr0)"
	p4 -V

As we're already under "set -e" (in ci/lib.sh).

I.e. I don't see why we need to work at making the output less verbose, and then if we fail run the command again.

If we really want to make it less verbose and avoid hiding segfault
messages perhaps we could filter out the stuff we don't care about?:
	
	$ ./p4d -V | sed -ne '/[Cc]opyright/d; /[Ll]icense/d; /http/d; /product/d; p'
	Perforce - The Fast Software Configuration Management System.
	Version of OpenSSL Libraries: OpenSSL 1.1.1q  5 Jul 2022
	Version of OpenLDAP Libraries: 2.4.59
	Version of Cyrus SASL Libraries: 2.1.27
	Using the 'SmartHeap' memory manager.
	Rev. P4D/LINUX26X86_64/2021.1/2313999 (2022/07/15).
	
But I don't see the point of that effort, we won't look at these trace
logs unless something fails, so just including the raw output seems most
sensible.

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

Yup, makes sense.

Thanks, both, for working on this topic.

From: Jiang Xin <zhiyou.jx@alibaba-inc.com>

There would be a segmentation fault when running p4 v16.2 on ubuntu 22.04 which is the latest version of ubuntu runner image for github actions.

By checking each version from [1], p4d version 21.1 and above can work properly on ubuntu 22.04. But version 22.x will break some p4 test cases. So p4 version 21.x is exactly the version we can use.

With this update, the versions of p4 for Linux and macOS happen to be the same. So we can add the version number directly into the "P4WHENCE" variable, and reuse it in p4 installation for macOS.

By removing the "LINUX_P4_VERSION" variable from "ci/lib.sh", the comment left above has nothing to do with p4, but still applies to git-lfs. Since we have a fixed version of git-lfs installed on Linux, we may have a different version on macOS.

[1]: https://cdist2.perforce.com/perforce/

Reviewed-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Helped-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
---
 ci/install-dependencies.sh | 4 ++--
 ci/lib.sh                  | 1 -
 2 files changed, 2 insertions(+), 3 deletions(-)

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.

Helped-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
---
 ci/install-dependencies.sh | 2 +-
 ci/lib.sh                  | 8 ++++----
 2 files changed, 5 insertions(+), 5 deletions(-)

From: Jiang Xin <zhiyou.jx@alibaba-inc.com>

GitHub starts to upgrade its runner image "ubuntu-latest" from version "ubuntu-20.04" to version "ubuntu-22.04". It will fail to find and install "gcc-8" package on the new runner image.

Change the runner images from "ubuntu-latest" to "ubuntu-20.04" in order to run with "gcc-8" as a dependency.

Instead of use the environment "$runs_on_pool" as below:

    case "$runs_on_pool" in
    ubuntu-20.04 | ubuntu-latest)
	;;

we can reuse the os field in the matrix, and use a new environment "$runs_on_os" as below:

    case "$runs_on_os" in
    ubuntu)
        ;;

In this way, we can change the "ubuntu-latest" runner image to any version such as "ubuntu-22.04" to test CI behavior in advance.

Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
---
 .github/workflows/main.yml | 16 ++++++++++++----
 ci/install-dependencies.sh |  6 +++---
 ci/lib.sh                  |  6 +++---
 3 files changed, 18 insertions(+), 10 deletions(-)

On Thu, Nov 24 2022, Jiang Xin wrote:

I very much like this direction, as we shouldn't have to scramble to update our CI every time GitHub switches defaults, and by using "latest" in general we also break e.g. re-pushes or re-rolls of outstanding topics/PRs.

I.e. you'll push today, it'll work, tomorrow the "latest" image has changed and your CI breaks, but through no fault of the code you're testing.

But per previous discussion on this exact topic at least Junio & Derrick (CC'd) disagreed with going in that direction, see https://lore.kernel.org/git/220825.865yig4bd7.gmgdl@evledraar.gmail.com/ (and the E-Mail it links to).

Now, I think that if we have some notification so we'll update the "pinned" image sooner than later it'll address the concerns they had, but do we have that? I've seen notices from GitHub CI about e.g. some things in the main.yml file being deprecated, will we get those for these "pinned" images too in a timely manner?

Because while I think a non-pinned "latest" sucks because it forces us to scramble with updates like this, having a "pinned" image go away entirely (because nobody noticed we should update it sooner-than-later) would suck more.

I think this is probably a good change, as we won't need to search-replace the image name in as many places in the future.

But let's have this as a seperate change. Here you're just refactoring the selection behavior of the main.yml + CI lib, it doesn't need to be bundled up with the change to change the target image.

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

Yeah, that matches my preference, assuming that we do not have a pre-warning to help us avoid scrambling either way.

If we have such a pre-warning, I would say it may be nicer to pin a version, even if the only upside is that we can say we test with "this exact version".

From: Jiang Xin <zhiyou.jx@alibaba-inc.com>

There would be a segmentation fault when running p4 v16.2 on ubuntu 22.04 which is the latest version of ubuntu runner image for github actions.

By checking each version from [1], p4d version 21.1 and above can work properly on ubuntu 22.04. But version 22.x will break some p4 test cases. So p4 version 21.x is exactly the version we can use.

In addition to upgrade p4 from version 16.2 to 21.2, also add some instructions to show errors of command "p4 -V", so we can see why the command output doesn't match.

[1]: https://cdist2.perforce.com/perforce/

Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
---
 ci/install-dependencies.sh | 4 ++--
 ci/lib.sh                  | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

On Thu, Nov 24 2022, Jiang Xin wrote:

Omitted from this context is:

	# The Linux build installs the defined dependency versions below.
	# The OS X build installs much more recent versions,

That part should be updated here, as it's now out-of-date, they're now installing the same version: 21.2.

This is a welcome change, but it would be even more welcome if you followed-up and unified the linux and osx p4 logic as a follow-up.

I.e. after this we'll install 21.2 on both osx and linux, so the versions are no longer different.

I think we probably won't need to install different versions for the two ever, we just drifted on the linux version, or maybe (per the comment we'll need to adjust) there was some problem before with upgrading the linux version, but no longer?

I didn't dig, but covering some of that in the commit message would be most welcome.

So can we just s/LINUX_P4_VERSION/P4_VERSION/ or something, and then change this in the "macos-latest";

	wget -q "https://cdist2.perforce.com/perforce/r21.2/bin.macosx1015x86_64/helix-core-server.tgz"

To:

	wget -q "https://cdist2.perforce.com/perforce/r${P4_VERSION}/bin.macosx1015x86_64/helix-core-server.tgz"

While doing that we can just move the "LINUX_P4_VERSION" (or whatever we rename it to) from lib.sh to "install-dependencies.sh".

On Thu, Nov 24, 2022 at 7:01 PM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:

Yes, the versions of p4 for linux and macos happen to be the same. We can use one variable to define p4 version for both linux and macos.

It is commit d1c9195116 (ci: avoid brew for installing perforce, 2022-05-12), which changed the installation method of p4 on macOS. There is also a note about the obsolete comment in ci/lib.sh in this commit.

Will do.

-- Jiang Xin

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(-)

On Thu, Nov 24 2022, Jiang Xin wrote:

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}"

Even if you don't factor out the "else" like that (which I think would be OK to do while-we'er-at-it) this should be changed so that the "PYTHON_PACKAGE" comes before "MAKEFLAGS" in the two if/else branches here, so we can then use the variable.

On Thu, Nov 24, 2022 at 7:06 PM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:

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.

-- Jiang Xin

On Thu, Nov 24 2022, Jiang Xin wrote:

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.