diff mbox series

[v2] macos: do let the build find the gettext headers/libraries/msgfmt

Message ID pull.616.v2.git.1587819266388.gitgitgadget@gmail.com (mailing list archive)
State New, archived
Headers show
Series [v2] macos: do let the build find the gettext headers/libraries/msgfmt | expand

Commit Message

John Passaro via GitGitGadget April 25, 2020, 12:54 p.m. UTC 
From: Johannes Schindelin <johannes.schindelin@gmx.de>

Apparently a recent Homebrew update now installs `gettext` into a
subdirectory under /usr/local/, requiring the CFLAGS/LDFLAGS to list
explicit directories _even_ when asking to force-link the `gettext`
package.

Likewise, the `msgfmt` tool is no longer in the `PATH`.

While it is unclear which change is responsible for this breakage (that
most notably only occurs on CI build agents that updated very recently),
https://github.com/Homebrew/homebrew-core/pull/53489 should fix it.

Nevertheless, let's work around this issue, as there are still quite a
few build agents out there that need some help in this regard: we
explicitly do not call `brew update` in our CI/PR builds anymore.

Helped-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
    Prepare for Homebrew changing the gettext package
    
    In an early Azure Pipelines preview of what is to come, I saw the 
    osx-clang and osx-gcc jobs fail consistently.
    
    This patch tries to prevent that from affecting our CI/PR builds.
    
    Changes since v1:
    
     * Described a bit better what the issue is, and that there is a
       "de-keg" change that should fix this (but as we no longer call brew
       update, some build agents, that won't matter for slightly out of date
       agents).
     * Guarded the added flags behind a check whether the directory exists
       in the first place.

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-616%2Fdscho%2Fbrew-gettext-update-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-616/dscho/brew-gettext-update-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/616

Range-diff vs v1:

 1:  c56a2321f62 ! 1:  1aa1b049e5c macos: do let the build find the gettext headers/libraries/msgfmt
     @@ Commit message
      
          Likewise, the `msgfmt` tool is no longer in the `PATH`.
      
     -    Let's work around this issue.
     +    While it is unclear which change is responsible for this breakage (that
     +    most notably only occurs on CI build agents that updated very recently),
     +    https://github.com/Homebrew/homebrew-core/pull/53489 should fix it.
      
     +    Nevertheless, let's work around this issue, as there are still quite a
     +    few build agents out there that need some help in this regard: we
     +    explicitly do not call `brew update` in our CI/PR builds anymore.
     +
     +    Helped-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
          Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
      
       ## config.mak.uname ##
     @@ config.mak.uname: ifeq ($(uname_S),Darwin)
       	HAVE_NS_GET_EXECUTABLE_PATH = YesPlease
      -	BASIC_CFLAGS += -I/usr/local/include
      -	BASIC_LDFLAGS += -L/usr/local/lib
     -+	BASIC_CFLAGS += -I/usr/local/include -I/usr/local/opt/gettext/include
     -+	BASIC_LDFLAGS += -L/usr/local/lib -L/usr/local/opt/gettext/lib
     -+	ifeq ($(shell test -x /usr/local/opt/gettext/bin/msgfmt && echo y),y)
     -+		MSGFMT = /usr/local/opt/gettext/bin/msgfmt
     ++
     ++	# Workaround for `gettext` being keg-only and not even being linked via
     ++	# `brew link --force gettext`, should be obsolete as of
     ++	# https://github.com/Homebrew/homebrew-core/pull/53489
     ++	ifeq ($(shell test -d /usr/local/opt/gettext/ && echo y),y)
     ++		BASIC_CFLAGS += -I/usr/local/include -I/usr/local/opt/gettext/include
     ++		BASIC_LDFLAGS += -L/usr/local/lib -L/usr/local/opt/gettext/lib
     ++		ifeq ($(shell test -x /usr/local/opt/gettext/bin/msgfmt && echo y),y)
     ++			MSGFMT = /usr/local/opt/gettext/bin/msgfmt
     ++		endif
      +	endif
       endif
       ifeq ($(uname_S),SunOS)


 config.mak.uname | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)


base-commit: e870325ee8575d5c3d7afe0ba2c9be072c692b65

Comments

Torsten Bögershausen April 26, 2020, 5:05 p.m. UTC | #1 
On Sat, Apr 25, 2020 at 12:54:26PM +0000, Johannes Schindelin via GitGitGadget wrote:
> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> Apparently a recent Homebrew update now installs `gettext` into a
> subdirectory under /usr/local/, requiring the CFLAGS/LDFLAGS to list
> explicit directories _even_ when asking to force-link the `gettext`
> package.
>
> Likewise, the `msgfmt` tool is no longer in the `PATH`.
>
> While it is unclear which change is responsible for this breakage (that
> most notably only occurs on CI build agents that updated very recently),
> https://github.com/Homebrew/homebrew-core/pull/53489 should fix it.
>
> Nevertheless, let's work around this issue, as there are still quite a
> few build agents out there that need some help in this regard: we
> explicitly do not call `brew update` in our CI/PR builds anymore.
>
> Helped-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>

It seems as if things are happening fast in the brew-business.
After debugging (on a local box) and travis-ing (remote) I may have a suggestion
for a better commit-message - I can probably send that out as a patch later today.
What do you think ?

=================================================================
MacOs/brew: Let the build find gettext headers/libraries/msgfmt

Apparently a recent Homebrew update now installs `gettext` into the
subdirectory /usr/local/opt/gettext/[lib/include].

Sometimes the ci job succeeds:
 brew link --force gettext
 Linking /usr/local/Cellar/gettext/0.20.1... 179 symlinks created

And sometimes installing the package "gettext" with force-link fails:
 brew link --force gettext
 Warning: Refusing to link macOS provided/shadowed software: gettext
 If you need to have gettext first in your PATH run:
  echo 'export PATH="/usr/local/opt/gettext/bin:$PATH"' >> ~/.bash_profile

(And the is not the final word either, since MacOs itself says:
 The default interactive shell is now zsh.)

Anyway, The latter requires CFLAGS to include /usr/local/opt/gettext/include
and LDFLAGS to include /usr/local/opt/gettext/lib.

Likewise, the `msgfmt` tool is no longer in the `PATH`.

While it is unclear which change is responsible for this breakage (that
most notably only occurs on CI build agents that updated very recently),
https://github.com/Homebrew/homebrew-core/pull/53489 has fixed it.

Nevertheless, let's work around this issue, as there are still quite a
few build agents out there that need some help in this regard: we
explicitly do not call `brew update` in our CI/PR builds anymore.

Helped-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Carlo Marcelo Arenas Belón April 26, 2020, 5:34 p.m. UTC | #2 
On Sat, Apr 25, 2020 at 12:54:26PM +0000, Johannes Schindelin via GitGitGadget wrote:
> diff --git a/config.mak.uname b/config.mak.uname
> index 0ab8e009383..1ea16e89288 100644
> --- a/config.mak.uname
> +++ b/config.mak.uname
> @@ -133,8 +133,17 @@ ifeq ($(uname_S),Darwin)
>  	HAVE_BSD_SYSCTL = YesPlease
>  	FREAD_READS_DIRECTORIES = UnfortunatelyYes
>  	HAVE_NS_GET_EXECUTABLE_PATH = YesPlease
> -	BASIC_CFLAGS += -I/usr/local/include
> -	BASIC_LDFLAGS += -L/usr/local/lib

keeping these flags independenly allows people that doesn't have brew or that
have brew but hadn't installed the gettext package, to still be able to use
other libraries/packages that could be installed in those directories
either through brew (ex: libgcrypt, openssl or pcre*) or manually while
also using a compiler that doesn't use by default /usr/local/{include,lib}.

even if all this might sound like a stretch, notice that it happened before
as shown by 2a1377a2a (macOS: make sure that gettext is found, 2019-04-14)

> +	# Workaround for `gettext` being keg-only and not even being linked via
> +	# `brew link --force gettext`, should be obsolete as of
> +	# https://github.com/Homebrew/homebrew-core/pull/53489
> +	ifeq ($(shell test -d /usr/local/opt/gettext/ && echo y),y)
> +		BASIC_CFLAGS += -I/usr/local/include -I/usr/local/opt/gettext/include
> +		BASIC_LDFLAGS += -L/usr/local/lib -L/usr/local/opt/gettext/lib
> +		ifeq ($(shell test -x /usr/local/opt/gettext/bin/msgfmt && echo y),y)
> +			MSGFMT = /usr/local/opt/gettext/bin/msgfmt
> +		endif
> +	endif
>  endif
>  ifeq ($(uname_S),SunOS)
>  	NEEDS_SOCKET = YesPlease
> 

since this doesn't depend on NO_GETTEXT and the gettext support doesn't get
automatically configured and used if found (unlike most other) then having
it here could work and is cleaner, but will still mean that MSGFMT will be
called to compile the translation files even when git is built with NO_GETTEXT

just because of that oddity I think having it with the other package related
entries in Makefile might still make sense (specially since we can't get
rid of it unless we also deprecate the other package managers), but if
cleaning it is what you had in mind  would also appreciate in that line your
review on 20200425102651.51961-1-carenas@gmail.com[1] and
20200425091549.42293-1-carenas@gmail.com[2] that do similar work and that
in the later case improve performance and correctness of git grep.

Carlo

[1] https://lore.kernel.org/git/20200425102651.51961-1-carenas@gmail.com/
[2] https://lore.kernel.org/git/20200425091549.42293-1-carenas@gmail.com/
diff mbox series

Patch

diff --git a/config.mak.uname b/config.mak.uname
index 0ab8e009383..1ea16e89288 100644
--- a/config.mak.uname
+++ b/config.mak.uname
@@ -133,8 +133,17 @@  ifeq ($(uname_S),Darwin)
 	HAVE_BSD_SYSCTL = YesPlease
 	FREAD_READS_DIRECTORIES = UnfortunatelyYes
 	HAVE_NS_GET_EXECUTABLE_PATH = YesPlease
-	BASIC_CFLAGS += -I/usr/local/include
-	BASIC_LDFLAGS += -L/usr/local/lib
+
+	# Workaround for `gettext` being keg-only and not even being linked via
+	# `brew link --force gettext`, should be obsolete as of
+	# https://github.com/Homebrew/homebrew-core/pull/53489
+	ifeq ($(shell test -d /usr/local/opt/gettext/ && echo y),y)
+		BASIC_CFLAGS += -I/usr/local/include -I/usr/local/opt/gettext/include
+		BASIC_LDFLAGS += -L/usr/local/lib -L/usr/local/opt/gettext/lib
+		ifeq ($(shell test -x /usr/local/opt/gettext/bin/msgfmt && echo y),y)
+			MSGFMT = /usr/local/opt/gettext/bin/msgfmt
+		endif
+	endif
 endif
 ifeq ($(uname_S),SunOS)
 	NEEDS_SOCKET = YesPlease