Re: [PATCH] fixup! CI: move p4 and git-lfs variables to ci/install-dependencies.sh

From: Junio C Hamano <gitster@pobox.com>
Date: Apr 23, 2022, 22:51 UTC
Message-ID: <xmqqo80r1kid.fsf@gitster.g>
In-Reply-To: <20220423201129.34133-1-carenas@gmail.com>

Carlo Marcelo Arenas Belón  <carenas@gmail.com> writes:

Show 11 quoted lines

> Once the code was moved into an ubuntu specific branch, it doesn't make
> sense anymore to refer to logic that only applies to a different branch
>
> Cleanup the comment to allow for the implementation to change without
> making it stale, and while at it rename the variable used to track the
> P4 version to something slightly less confusing.
>
> Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
> ---
>  ci/install-dependencies.sh | 8 +++-----
>  1 file changed, 3 insertions(+), 5 deletions(-)

OK.  The place we run wget from is

    $P4WHENCE/bin.linux26x86_64/{p4,p4d}

for Linux.  And P4WHENCE is defined to be

    http://{filehost,cdist2}.perforce.com/perforce/r$LINUX_P4_VERSION

Unfortunately, what we grab are not these two files for macOS.

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

If p4 and p4d were the only files that we grabbed for macOS, then what I wanted to suggest in the previous message would have worked, i.e.

We define, per $runs_on_pool, two variables:

     P4VERSION (e.g. "r16.2", "r21.2")
     P4ARCH (e.g. "bin.macosx1015x86_64", "bin.linux26x86_64")

Then a single definition, after the "case $runs_on_pool ... esac", of a leading URL, i.e.

     P4DIST=https://cdist2.perforce.com/perforce/$P4VERSION/$P4ARCH/

can be used to help two wget commands, i.e.

     wget --quiet "$P4DIST/p4"
     wget --quiet "$P4DIST/p4d"

in install-dependencies.sh for both Linux-x86_64 and macOS.

But if we cannot unify the way the installation is done for macOS and Linux that way, I do not think renaming $LINUX_P4_VERSION to another name is worth doing (notice that the point of the above exercise was to abstract these into just two variables, VERSION and ARCH).

By the way, I spelunked https://filehost.perforce.com/perforce/ from a browser to see how the files are layed out over there, not cdist2. I am guessing that "filehost" is the name they want end-users like us to use when downloading what they distribute?

Show 27 quoted lines

> diff --git a/ci/install-dependencies.sh b/ci/install-dependencies.sh
> index 098baeef3fb..7bea45fd339 100755
> --- a/ci/install-dependencies.sh
> +++ b/ci/install-dependencies.sh
> @@ -23,11 +23,9 @@ esac
>  case "$runs_on_pool" in
>  ubuntu-latest)
>  	# The Linux build installs the defined dependency versions below.
> -	# The OS X build installs much more recent versions, whichever
> -	# were recorded in the Homebrew database upon creating the OS X
> -	# image.
> +	# The OS X build installs much more recent versions
>  	# Keep that in mind when you encounter a broken OS X build!
> -	LINUX_P4_VERSION="16.2"
> +	P4_AT_LINUX_VERSION="16.2"
>  	LINUX_GIT_LFS_VERSION="1.5.2"
>  
>  	P4_PATH="$HOME/custom/p4"
> @@ -39,7 +37,7 @@ ubuntu-latest)
>  		echo "$PATH" >>"$GITHUB_PATH"
>  	fi
>  
> -	P4WHENCE=http://filehost.perforce.com/perforce/r$LINUX_P4_VERSION
> +	P4WHENCE=http://filehost.perforce.com/perforce/r$P4_AT_LINUX_VERSION
>  	LFSWHENCE=https://github.com/github/git-lfs/releases/download/v$LINUX_GIT_LFS_VERSION
>  
>  	sudo apt-get -q update