Re: [PATCH v3 5/9] i5500-git-daemon.sh: use compile-able version of Git without OpenSSL

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 11, 2024, 15:28 UTC
Message-ID: <xmqqr09qb462.fsf@gitster.g>
In-Reply-To: <20240911061009.GA1538383@coredump.intra.peff.net>

Jeff King <peff@peff.net> writes:

Show 6 quoted lines

> How about this instead?
>
> -- >8 --
> Subject: [PATCH] t/interop: allow per-version make options
> ...
>      imap-send.c to barf, because it declares a fallback typdef for SSL.

"typedef".

Show 22 quoted lines

> diff --git a/t/interop/i5500-git-daemon.sh b/t/interop/i5500-git-daemon.sh
> index 4d22e42f84..88712d1b5d 100755
> --- a/t/interop/i5500-git-daemon.sh
> +++ b/t/interop/i5500-git-daemon.sh
> @@ -2,6 +2,7 @@
>  
>  VERSION_A=.
>  VERSION_B=v1.0.0
> +MAKE_OPTS_B="NO_OPENSSL=TooOld"
>  
>  : ${LIB_GIT_DAEMON_PORT:=5500}
>  LIB_GIT_DAEMON_COMMAND='git.a daemon'
> diff --git a/t/interop/interop-lib.sh b/t/interop/interop-lib.sh
> index 62f4481b6e..1b5864d2a7 100644
> --- a/t/interop/interop-lib.sh
> +++ b/t/interop/interop-lib.sh
> @@ -45,7 +45,7 @@ build_version () {
>  
>  	(
>  		cd "$dir" &&
> -		make $GIT_INTEROP_MAKE_OPTS >&2 &&
> +		make $2 $GIT_INTEROP_MAKE_OPTS >&2 &&

The build options should be simple enough and this should do for now (and when it becomes needed, it is easy to add an eval around it).

The use of $GIT_INTEROP_MAKE_OPTS here looks a bit curious. It overrides what the inidividual script gave in MAKE_OPTS_{A,B} and what is globally given in GIT_INTEROP_MAKE_OPTS_{A,B}.

With this design, the following is not what we should write:

    # by default we use the frotz feature
    GIT_INTEROP_MAKE_OPTS=USE_FROTZ=YesPlease
    # but version A is too old for it
    MAKE_OPTS_A=USE_FROTZ=NoThanks
    # we do not need any cutomization for version B
    MAKE_OPTS_B=

Rather we would want to say:

    # the default should say nothing conflicting with A or B
    GIT_INTEROP_MAKE_OPTS=
    # version A is too old to use the frotz feature
    MAKE_OPTS_A=USE_FROTZ=NoThanks
    # version B is OK
    MAKE_OPTS_B=USE_FROTZ=YesPlease

As long as it is understood that GIT_INTEROP_MAKE_OPTS and *_{A,B} are *not* meant to be used in a way for one to give default and the other to override the defautl, but they are to give orthogonal settings, this is fine.

Show 9 quoted lines

>  		touch .built
>  	) || return 1
>  
> @@ -76,9 +76,11 @@ generate_wrappers () {
>  
>  VERSION_A=${GIT_TEST_VERSION_A:-$VERSION_A}
>  VERSION_B=${GIT_TEST_VERSION_B:-$VERSION_B}
> +MAKE_OPTS_A=${GIT_INTEROP_MAKE_OPTS_A:-$MAKE_OPTS_A}
> +MAKE_OPTS_B=${GIT_INTEROP_MAKE_OPTS_B:-$MAKE_OPTS_B}

Among the variables we see around here, GIT_INEROP_MAKE_OPTS is the only one that is recorded in the GIT-BUILD-OPTIONS file, which is included in t/interop/interop-lib.sh file. Shouldn't we record GIT_INEROP_MAKE_OPTS_{A,B} as well?

Show 7 quoted lines

> -if ! DIR_A=$(build_version "$VERSION_A") ||
> -   ! DIR_B=$(build_version "$VERSION_B")
> +if ! DIR_A=$(build_version "$VERSION_A" "$MAKE_OPTS_A") ||
> +   ! DIR_B=$(build_version "$VERSION_B" "$MAKE_OPTS_B")
>  then
>  	echo >&2 "fatal: unable to build git versions"
>  	exit 1

Thanks.