| 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 |
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`).
"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?
"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 --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