Re: [PATCH] This PR enables a successful git build on z/OS.

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 31, 2024, 17:12 UTC
Message-ID: <xmqqr0hx77ga.fsf@gitster.g>
In-Reply-To: <pull.1663.git.git.1706710861778.gitgitgadget@gmail.com>

"Haritha  via GitGitGadget" <gitgitgadget@gmail.com> writes:

> Subject: Re: [PATCH] This PR enables a successful git build on z/OS.

Please think with longer term effects in mind when formulating the title of the commit. What title will your next patch have if we break the build for z/OS next time, after this fix goes in? "enables a successful build again"? How would one tell which commit changed what aspect of the build procedure to adjust to z/OS?

Perhaps

    [PATCH] Makefile: adjust for z/OS that lack dynamic library support

or something would be specific enough.

Show 7 quoted lines

> From: Haritha D <harithamma.d@ibm.com>
>
> Since the z/OS linker does not support searching dynamic libraries,
> and the current setting of CC_LD_DYNPATH results in a directory
> to be supplied to the link step with no option as the suffix,
> it causes a linker error because the z/OS LD linker
> does not accept directories as input.

Hmph, it is not quite clear to me where that "current setting of CC_LD_DYNPATH" comes from and what exact value it is set to.

Here is my attempt (blind guesses are involved, so please correct whatever errors you spot):

    The autoconf generated configuration gives an empty string to
    CC_LD_DYNPATH when it cannot find a way to use a shared library
    and gives up with "linker does not support runtime path to
    dynamic libraries" message.  This leaves the directory path that
    is usually appended to -Wl,-rpath, or -R, or whatever alone on
    the command line of the linker, e.g. "-L/usr/lib /usr/lib", which
    breaks the linker.

    Work it around by setting CD_LD_DYNPATH to -L; we will end up
    giving the same directory twice, e.g., "-L/usr/lib -L/usr/lib",
    but it is only ugly without breaking anything.

    While at it, define appropriate settings for z/OS (OS/390) in
    the config.mak.uname file.

Show 13 quoted lines

> Therefore, we workaround this by adding the -L option.
> And, Introduced z/OS (OS/390) as a platform in config.mak.uname
>
> Signed-off-by: Haritha D <harithamma.d@ibm.com>
> ---
>     This PR enables a successful git build on z/OS.
>     
>     Since the z/OS linker does not support searching dynamic libraries, and
>     the current setting of CC_LD_DYNPATH results in a directory to be
>     supplied to the link step with no option as the suffix, it causes a
>     linker error because the z/OS LD linker does not accept directories as
>     input. Therefore, we workaround this by adding the -L option. And,
>     Introduced z/OS (OS/390) as a platform in config.mak.uname

You do not have to write the same thing twice. The text under "---" is meant for extra explanation that does not need to become part of the final commit log message.

Show 20 quoted lines

> diff --git a/config.mak.uname b/config.mak.uname
> index dacc95172dc..c8006f854e5 100644
> --- a/config.mak.uname
> +++ b/config.mak.uname
> @@ -638,6 +638,18 @@ ifeq ($(uname_S),NONSTOP_KERNEL)
>  	SANE_TOOL_PATH = /usr/coreutils/bin:/usr/local/bin
>  	SHELL_PATH = /usr/coreutils/bin/bash
>  endif
> +ifeq ($(uname_S),OS/390)
> +        NO_SYS_POLL_H = YesPlease
> +        NO_STRCASESTR = YesPlease
> +        NO_REGEX = YesPlease
> +        NO_MMAP = YesPlease
> +        NO_NSEC = YesPlease
> +        NO_STRLCPY = YesPlease
> +        NO_MEMMEM = YesPlease
> +        NO_GECOS_IN_PWENT = YesPlease
> +        HAVE_STRINGS_H = YesPlease
> +        NEEDS_MODE_TRANSLATION = YesPlease
> +endif

I cannot tell if these are reasonable for z/OS myself and I'll take your word for it ;-) After all you're the expert.

Show 14 quoted lines

> diff --git a/configure.ac b/configure.ac
> index d1a96da14eb..64569a80d53 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -463,6 +463,9 @@ else
>              CC_LD_DYNPATH=-Wl,+b,
>            else
>               CC_LD_DYNPATH=
> +	     if test "$(uname -s)" = "OS/390"; then
> +		     CC_LD_DYNPATH=-L
> +	     fi
>               AC_MSG_WARN([linker does not support runtime path to dynamic libraries])
>            fi
>        fi

The use of "uname -s" looks totally out of place.

Wouldn't it be a better approach to set it in config.mak.uname for OS/390 above and leave this part untouched, I wonder?