diff mbox series

This PR enables a successful git build on z/OS.

Message ID pull.1663.git.git.1706710861778.gitgitgadget@gmail.com (mailing list archive)
State New, archived
Headers show
Series This PR enables a successful git build on z/OS. | expand

Commit Message

Haritha D Jan. 31, 2024, 2:21 p.m. UTC
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.
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

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1663%2FHarithaIBM%2Fzos-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1663/HarithaIBM/zos-v1
Pull-Request: https://github.com/git/git/pull/1663

 config.mak.uname | 12 ++++++++++++
 configure.ac     |  3 +++
 2 files changed, 15 insertions(+)


base-commit: bc7ee2e5e16f0d1e710ef8fab3db59ab11f2bbe7

Comments

Kristoffer Haugsbakk Jan. 31, 2024, 4:12 p.m. UTC | #1
Hi

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

Maybe the subject could be:

> [PATCH] build: support z/OS (OS/390)

There’s some indirectly related things in `SubmittingPatches`:

 “  Describe your changes in imperative mood, e.g. "make xyzzy do frotz"
    instead of "[This patch] makes xyzzy do frotz" or "[I] changed xyzzy
    to do frotz", as if you are giving orders to the codebase to change
    its behavior.  Try to make sure your explanation can be understood
    without external resources. Instead of giving a URL to a mailing
    list archive, summarize the relevant points of the discussion.

From which I infer that “This PR” is considered redundant.

As for the `build:` prefix: many different prefixes have been used for
this file (`git log --no-merges -- config.mak.uname`).
Junio C Hamano Jan. 31, 2024, 5:12 p.m. UTC | #2
"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.

> 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.

> 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.

> 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.

> 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?
Junio C Hamano Jan. 31, 2024, 5:58 p.m. UTC | #3
"Kristoffer Haugsbakk" <code@khaugsbakk.name> writes:

> Hi
>
>> [PATCH] This PR enables a successful git build on z/OS.
>
> Maybe the subject could be:
>
>> [PATCH] build: support z/OS (OS/390)

Nice.  This is nicer than what I came up with, that placed too much
stress on the runtime path to the dynamic library.  With all the
stuff added to config.mak.uname, yours is much more appropriate.

Thanks.
diff mbox series

Patch

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
 ifeq ($(uname_S),MINGW)
 	ifeq ($(shell expr "$(uname_R)" : '1\.'),2)
 		$(error "Building with MSys is no longer supported")
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