ci: drop no-op gettext link on macOS

From: Harald Nordgren <[email protected]>

gettext ships already linked on the macOS CI runner image, so this command never links anything. It only emits a spurious "Already linked" warning in every macOS job's log.

---
ci: drop no-op gettext link on macOS

Gettext is already linked on the macOS CI runner image, so the forced relink was a no-op that only produced a spurious "Already linked" warning in every macOS job's log.

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-2402%2FHaraldNordgren%2Fci-drop-noop-gettext-link-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-2402/HaraldNordgren/ci-drop-noop-gettext-link-v1
Pull-Request: https://github.com/git/git/pull/2402

 ci/install-dependencies.sh | 1 -
 1 file changed, 1 deletion(-)

index 2f61fbb07c..2903c39fab 100755
@@ -117,7 +117,6 @@ macos-*)
 	export HOMEBREW_NO_AUTO_UPDATE=1 HOMEBREW_NO_INSTALL_CLEANUP=1
 	# Uncomment this if you want to run perf tests:
 	# brew install gnu-time
-	brew link --force gettext
 
 	mkdir -p "$CUSTOM_PATH"
 	wget -q "$P4WHENCE/bin.macosx12arm64/helix-core-server.tgz" &&

base-commit: 47ce80527c56f462cb97db4ca8125342204d3783

gitgitgadget

I feel like this is oversimplifying things a bit. The commit message doesn't explain why we even orignially had it, and consequently it's not really explaining why we don't anymore.

The gettext dependency in Homebrew is keg-only, which means that it will typically never be linked anywhere visible into your system. Instead, it is only being installed into your Homebrew cellar, which means that it will not interfer with anything. And trying to link such a dependency will not do anything, as Homebrew refuses such an operation.

By using brew link --force though you override that and thus override any system-provided equivalents by symlinking the dependency into a central place. Now the question is whether this is still needed. But that question is not answered by your commit message. The warning that you mention is this one:

+ brew link --force gettext
✔︎ JSON API formula_tap_migrations.jws.json
✔︎ JSON API cask_tap_migrations.jws.json
Warning: Already linked: /opt/homebrew/Cellar/gettext/1.0 To relink, run:

    brew unlink gettext && brew link gettext

But that warning only complains that gettext is already linked into your cellar. It does not say whether it may have linked the library into your system's path now.

So I'm still left wondering whether it really isn't required anymore, and if so why it stopped being required. Links to passing GitHub and GitLab pipelines with that change would've in this case also helped to demonstrate that it works without that line.

Thanks!

Patrick

It passed on GitHub: https://github.com/git/git/pull/2402

I'll take a look at the rest of your comments as well, thanks for the help!

Harald

If you look at the pipeline though you can see that almost all of the jobs were skippped. And in particular, macOS wasn't tested at all.

Patrick

That's after I pushed to update the commit message, it didn't re-trigger because the code was the same I think.

Here is an earlier test run when macOS tests were successfully run:
https://github.com/git/git/actions/runs/34655661080

Harald

Here is a rebased branch where all macOS tests have run:
https://github.com/git/git/pull/2402

Would you have time to take another look?

Harald

Sorry, I seem to have missed v2 because I wasn't Cc'd. I've responded there now, thanks!

Patrick

From: Harald Nordgren <[email protected]>

Homebrew's gettext formula used to be keg-only, meaning brew would install it into the Cellar without linking its headers, libraries, or utilities like msgfmt anywhere on the default search paths, to avoid clashing with software that already ships its own gettext. brew link --force gettext overrode that restriction so the keg-only install became visible on the paths git's build and tests rely on.

Homebrew dropped gettext's keg-only status in 2020
(https://github.com/Homebrew/homebrew-core/pull/53489), so a fresh install now links into the prefix by itself. brew link --force against an already-linked keg has nothing left to override, and only produces a spurious "Already linked" warning in every macOS job's log.

---
ci: drop no-op gettext link on macOS

Gettext is already linked on the macOS CI runner image, so the forced relink was a no-op that only produced a spurious "Already linked" warning in every macOS job's log.

Changes in v2:

* Clarify problem in commit message.

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-2402%2FHaraldNordgren%2Fci-drop-noop-gettext-link-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-2402/HaraldNordgren/ci-drop-noop-gettext-link-v2
Pull-Request: https://github.com/git/git/pull/2402

Range-diff vs v1:

1: 9af619d085 ! 1: 39922c8e97 ci: drop no-op gettext link on macOS
@@ Metadata
## Commit message ##
ci: drop no-op gettext link on macOS

- gettext ships already linked on the macOS CI runner image, so this
- command never links anything. It only emits a spurious "Already
- linked" warning in every macOS job's log.
+ Homebrew's gettext formula used to be keg-only, meaning brew would
+ install it into the Cellar without linking its headers, libraries, or
+ utilities like msgfmt anywhere on the default search paths, to avoid
+ clashing with software that already ships its own gettext. `brew link
+ --force gettext` overrode that restriction so the keg-only install
+ became visible on the paths git's build and tests rely on.
+
+ Homebrew dropped gettext's keg-only status in 2020
+ (https://github.com/Homebrew/homebrew-core/pull/53489), so a fresh
+ install now links into the prefix by itself. brew link --force
+ against an already-linked keg has nothing left to override, and only
+ produces a spurious "Already linked" warning in every macOS job's
+ log.

 ci/install-dependencies.sh | 1 -
 1 file changed, 1 deletion(-)

index 2f61fbb07c..2903c39fab 100755
@@ -117,7 +117,6 @@ macos-*)
 	export HOMEBREW_NO_AUTO_UPDATE=1 HOMEBREW_NO_INSTALL_CLEANUP=1
 	# Uncomment this if you want to run perf tests:
 	# brew install gnu-time
-	brew link --force gettext
 
 	mkdir -p "$CUSTOM_PATH"
 	wget -q "$P4WHENCE/bin.macosx12arm64/helix-core-server.tgz" &&

base-commit: 47ce80527c56f462cb97db4ca8125342204d3783

gitgitgadget

Sorry, but I lost track. There were review exchanges on the previous round, which crossed with this new iteration. Is this patch still relevant and do folks on macOS need this?

Thanks.

Hi, no worries!

Yes, it’s still relevant. It’s not for local macOS development, but to silence a warning message on the GitHub CI builds for macOS.

Harald

Thanks, that's useful context to have and explains why it's fine to remove this now.