Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

gnu-tar: fix build for High Sierra #155992

Closed

Conversation

Jerry1144
Copy link
Contributor

  • Have you followed the guidelines for contributing?
  • Have you ensured that your commits follow the commit style guide?
  • Have you checked that there aren't other open pull requests for the same formula update/change?
  • Have you built your formula locally with HOMEBREW_NO_INSTALL_FROM_API=1 brew install --build-from-source <formula>, where <formula> is the name of the formula you're submitting?
  • Is your test running fine brew test <formula>, where <formula> is the name of the formula you're submitting?
  • Does your build pass brew audit --strict <formula> (after doing HOMEBREW_NO_INSTALL_FROM_API=1 brew install --build-from-source <formula>)? If this is a new formula, does it pass brew audit --new <formula>?

Mimicking #136880, on High Sierra there are complaints about missing _libintl_ symbols too.

Configure also checked for external libintl but did not add -lintl. Seeing this formula built fine on newest macOSes, it's probably the system's libiconv being too old.

Or is it a bug just like 136880?

Mimicking Homebrew#136880, on High Sierra there are complaints about missing _libintl_ symbols too. 

Configure also checked for external libintl but did not add this LDFLAG. Seeing this formula built fine on newest macOSes, it's probably the system's `libiconv` being too old.
@Bo98
Copy link
Member

Bo98 commented Nov 30, 2023

libintl is from gettext but gettext isn't a dependency here. I'm guessing there's some opportunistic linking going on.

@Bo98
Copy link
Member

Bo98 commented Nov 30, 2023

We probably want to add --disable-nls to the configure options. That'll probably fix the issue you have.

@Jerry1144
Copy link
Contributor Author

On my system the compiled binary links like this:

objdump -macho -dylibs-used "$(which gtar)"
/usr/local/bin/gtar:
	/usr/local/opt/gettext/lib/libintl.8.dylib (compatibility version 13.0.0, current version 13.0.0)
	/usr/lib/libiconv.2.dylib (compatibility version 7.0.0, current version 7.0.0)
	/usr/lib/libSystem.B.dylib (compatibility version 1.0.0, current version 1252.50.4)

And works fine (i.e. displays localized characters). I guess the linking to libintl isn't as catastrophic as it seems?

The bug report referenced in the -liconv line said, libiconv symbols are not included in the standard system libraries on Macs, so there needs to be a separate, system-provided libiconv to be linked. Seeing that, I suspect the libiconv on my system is also too old to provide the required libintl symbols. Linking the separate libintl (from gettext) fix this for me, though this linkage is not needed for current macOS, as the bottled binaries did not link these. Showing Big Sur bottle.

objdump -macho -dylibs-used /path/to/bottled/gnu-tar/1.35/bin/gtar 
/path/to/bottled/gnu-tar/1.35/bin/gtar:
	/usr/lib/libiconv.2.dylib (compatibility version 7.0.0, current version 7.0.0)
	/usr/lib/libSystem.B.dylib (compatibility version 1.0.0, current version 1292.100.5)

@Bo98
Copy link
Member

Bo98 commented Dec 5, 2023

I guess the linking to libintl isn't as catastrophic as it seems?

Not catastrophic per se, but not expected behaviour as we don't have depends_on "gettext" here.

It should be made consistent: either everything uses gettext or nothing uses it. Arbitrarily using it depending on whether you happen to have it installed isn't ideal, unless that check happens at runtime.

Seeing that, I suspect the libiconv on my system is also too old to provide the required libintl symbols.

libiconv does not provide libintl symbols at all, even on newer macOS. libintl and libiconv are separate things. libintl support in gnu-tar is completely optional - it will use it if it can find it but ignore it if it can't. It can be forced disabled with --disable-nls.

@Jerry1144
Copy link
Contributor Author

Pulled out gnu-tar.rb for tar-1.34 and did a little experiment:

  • Depends on gettext, no disable-nls: binary links to /usr/local/opt/gettext/lib/libintl.8.dylib, localized prompt.
    • Can remove the symlink from opt/gettext to Cellar/gettext, and the program still runs fine (!) Need a way to stretch its libintl muscle.
  • Remove dependency on gettext: binary no longer links to gettext, nor has localized prompt. Probably Indeed with implicit disable-nls when no gettext is found.
    • Can still pass CJK-character-named arguments and tar them. I need to somehow trigger encoding conversion and see what happens.

For tar-1.35:

  • Depends on gettext, no disable-nls: build error on my High Sierra system, for not finding libintl symbols.
    • Add -lintl: builds fine, runs with localized prompt. Runs with localized prompt even with gettext severed from /usr/local/opt/
    • brew unlink gettext before install: builds fine, configure finds not gettext so goes on as if --disable-nls is passed. No localized prompt or linkage to gettext.
    • I now wonder if those bottled binaries run fine with localized prompt on current macOS, if they don't link to gettext's libintl. On linux libintl.h ship with system glibc so no need for that.

Found these lines in /usr/include/c++/4.2.1/bits/c++config.h on my system. Maybe on modern macOS there really is a system-provided libintl.h, and the whole depends_on gettext => :build can reside in a conditional block instead.

/* Define to 1 if you have the <libintl.h> header file. */
/* #undef _GLIBCXX_HAVE_LIBINTL_H */

@Bo98
Copy link
Member

Bo98 commented Dec 6, 2023

I now wonder if those bottled binaries run fine with localized prompt on current macOS

Bottled gnu-tar does not offer localisation. Nobody has really asked for it, and we generally don't provide support outside of English.

Found these lines in /usr/include/c++/4.2.1/bits/c++config.h on my system.

This is a legacy compatibly file from when Apple used GCC rather than Clang. They even had a LLVM-GCC layer for a while. The last version was 4.2 (hence the version number you see). A part of this move was libstdc++ (GCC) to libc++ (LLVM), the latter coming the default in OS X 10.8.

The code in question will be from GCC's Linux support. On Linux, libintl can be found in glibc.

GCC support was removed in Xcode 5 in 2013 (OS X 10.9 release), and that compatibility header was removed in the 10.14 SDK in 2018.

libintl doesn't ship in latest macOS SDKs either. Apple have largely gone in the opposite direction and have dropped anything GNU related for licensing reasons.

@Jerry1144
Copy link
Contributor Author

Thanks for the historical background!

Bottled gnu-tar does not offer localisation. Nobody has really asked for it, and we generally don't provide support outside of English.

Well if that is the case, then I probably should open a PR to pass --disable-nls to wget too? Arrow mine. Was a bit surprised when Homebrew's wget spoke my language!

$ objdump -macho -dylibs-used /path/to/bottled/big_sur/wget/1.21.4/bin/wget 
/path/to/bottled/big_sur/wget/1.21.4/bin/wget:
	/usr/lib/libiconv.2.dylib (compatibility version 7.0.0, current version 7.0.0)
-->	@@HOMEBREW_PREFIX@@/opt/gettext/lib/libintl.8.dylib (compatibility version 12.0.0, current version 12.0.0)
	/System/Library/Frameworks/CoreFoundation.framework/Versions/A/CoreFoundation (compatibility version 150.0.0, current version 1775.118.101)
	@@HOMEBREW_PREFIX@@/opt/libunistring/lib/libunistring.5.dylib (compatibility version 6.0.0, current version 6.0.0)
	@@HOMEBREW_PREFIX@@/opt/libidn2/lib/libidn2.0.dylib (compatibility version 4.0.0, current version 4.8.0)
	@@HOMEBREW_PREFIX@@/opt/openssl@3/lib/libssl.3.dylib (compatibility version 3.0.0, current version 3.0.0)
	@@HOMEBREW_PREFIX@@/opt/openssl@3/lib/libcrypto.3.dylib (compatibility version 3.0.0, current version 3.0.0)
	/usr/lib/libSystem.B.dylib (compatibility version 1.0.0, current version 1292.100.5)
	/usr/lib/libz.1.dylib (compatibility version 1.0.0, current version 1.2.11)

$ ls /path/to/bottled/big_sur/wget/1.21.4/share/locale
af	da	es	ga	id	lt	pt	sk	uk
be	de	et	gl	it	ms	pt_BR	sl	vi
bg	el	eu	he	ja	nb	ro	sr	zh_CN
ca	en_GB	fi	hr	ka	nl	ru	sv	zh_TW
cs	eo	fr	hu	ko	pl	rw	tr

Just kidding. I personally prefer to leave in gettext as a build-only dependency for those GNU efforts, and write off tar-1.35 as being bugged. After all, the symptom is similar enough to the -liconv line below (configure finds library, but does not add appropriate LDFLAG) and there's no new runtime dependency to begin with.

Arbitrarily using it depending on whether you happen to have it installed isn't ideal, unless that check happens at runtime.

It probably checks at runtime, or maybe not at all. I can safely remove /usr/local/opt/gettext and continue using gtar and wget. Gettext is not even in wget's dependency list, so every user of bottled wget should have noticed crashes for a missing libintl.dylib. No, they are happily greeted with a localized wget, without even pouring gettext.

Bottom line: if --disable-nls from now on, that depends_on gettext => :build line should be gone, too. That line came from 5 years ago, so it'd be 5 years of free locale support, suddenly stopped counting when some user opened a PR to highlight its existence. Sounds wrong to me.

@Jerry1144
Copy link
Contributor Author

OK I found the correct way to remove gettext and indeed gnu-tar and wget will crash afterwards, if built with localization support. Both brew unlink and removing opt/gettext link need to be done. Closing this PR.

@Jerry1144 Jerry1144 closed this Dec 9, 2023
@Jerry1144 Jerry1144 deleted the gnu-tar-fix-build-high-sierra branch December 9, 2023 14:38
@github-actions github-actions bot added the outdated PR was locked due to age label Jan 9, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 9, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated PR was locked due to age
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants