Skip to content

Conversation

@chrisd8088
Copy link
Member

The macOS runners provided by GitHub Actions are now provisioned with pre-installed versions of several Homebrew formulae that we currently re-install, which leads to annotation warnings in our CI job summaries. We can therefore simplify our CI jobs on macOS slightly by eliminating the unnecessary Homebrew invocations.

As well, we now also have a warning from CodeQL that our go.mod file does not specify a Go toolchain, which we can resolve most easily by providing a full Go version in the go line of that file.

This PR will be most easily reviewed on a commit-by-commit basis.

@chrisd8088 chrisd8088 requested a review from a team as a code owner December 10, 2024 07:43
Copy link
Member

@larsxschneider larsxschneider left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Since we migrated our CI jobs to GitHub Actions in PR git-lfs#3808, our
"build-latest" and "build-earliest" jobs, when running on macOS, have
explicitly installed and linked the "curl", "openssl", "pcre2", and
"zlib" Homebrew formulae before building a custom version of Git.

However, we have always set the NO_OPENSSL environment variable when
building custom versions of Git for our CI jobs, since at least commit
d874af9 of PR git-lfs#1617 in 2016.  This
implies that our Git builds do not make use of OpenSSL at all, either
for SHA hashing or for SSL support in the git-imap-send(1) command.
(Further, since Git version 7.34.0 Git does not require OpenSSL to
make IMAP connections over SSL, and in any case our test suite does not
use IMAP at all.)  Hence we do not need to install the "openssl"
Homebrew formula, since this library will never be linked into the
Git binaries we build.

Meanwhile, we do not set the USE_LIBPCRE environment variable when
building Git, so the PCRE2 library is not linked into our custom Git
binaries and we can thus skip installing the "pcre2" Homebrew formula.
(Also, on the Ubuntu Linux runners provided by GitHub Actions, the
"libpcre2-8-0" package is not available by default, so if we wanted to
set the USE_LIBPCRE variable we would need to install that package
for our Linux CI jobs.)

In addition,the modified version of zlib 1.2.12 supplied by macOS 14
(Sonoma) on the GitHub Actions runners we are using at present is
is sufficient to build Git, so we do not have to install the "zlib"
Homebrew formula either.

As described in commit f3cd1ef
of PR git-lfs#5866, for the moment we must continue to utilize the version of
libcurl from the Homebrew "curl" formula rather than the one supplied
by macOS itself, because the 8.7.1 version of libcurl currently shipped
with macOS 13 and 14 (Ventura and Sonoma) has a regression which
affects the programs used by git-http-backend(1).

However, the macOS 14 runners provided by GitHub Actions are provisioned
with a pre-installed version of the Homebrew "curl" formula, so we do
not need to actually install this formula ourselves.

We can therefore simply drop the "brew install" command entirely, since
there are no Homebrew formulae we have to install any more.

Note, though, that by default Homebrew does not create the symlinks
that would make its version of libcurl take precedence over the one
supplied by macOS.  As commit f3cd1ef
of PR git-lfs#5866 describes, we have to ensure the CURLDIR variable is set
properly so that our "build-earliest" CI jobs will link the Git programs
they build against the Homebrew version of libcurl rather than the one
supplied by macOS.

For this reason we currently run the "brew link" command to make sure
the Homebrew version of the curl-config(1) command is used to set the
value of the CURLDIR variable.  Using the "brew link --force" command
with "keg-only" libraries such as those from the "curl" formula is not
recommended practice, though, so in a subsequent commit in this PR we
will further adjust our script to set the value of the CURLDIR variable
as we require, but without running the "brew link" command first.
Since we migrated our CI jobs to GitHub Actions in PR git-lfs#3808, our
"build-latest" and "build-earliest" jobs, when running on macOS, have
explicitly installed and linked the "curl", "openssl", "pcre2", and
"zlib" Homebrew formulae before building a custom version of Git.

In a prior commit in this PR we removed the "brew install" command from
our script/build-git script entirely, as we no longer need to install
any Homebrew formulae, for the reasons described in that commit.

However, we left an invocation of the "brew link" command for the "curl"
formula in place in order to ensure that in our "build-earliest" CI job
the CURLDIR variable is set such that when we build Git version 2.0.0 in
that job, the Git binaries are linked against the Homebrew instance of
the libcurl library rather than the one provided by macOS.  We introduced
the CURLDIR variable in commit f3cd1ef
of PR git-lfs#5866 so as to prevent our Git builds from linking with the libcurl
library supplied by macOS, because at present macOS 13 and 14 (Ventura
and Sonoma) provide version 8.7.1 of libcurl and it has a regression that
affects the programs used by git-http-backend(1).

The Homebrew project recommends against using the "brew link --force"
command, though, for "keg-only" libraries like those from the "curl"
formula:

  https://docs.brew.sh/How-to-Build-Software-Outside-Homebrew-with-Homebrew-keg-only-Dependencies

Fortunately, we can achieve our desired result of linking our custom
Git binaries against the Homebrew version of libcurl just by setting
the CURLDIR variable using the "brew --prefix curl" command, so we do
that now.
As suggested by larsxschneider on PR review, to make it easier to
diagnose problems with our "build-earliest" and "build-latest" CI jobs
we can log the build options and dynamic library dependencies of the
custom versions of Git that we compile in those jobs.

The --build-options argument to the git-version(1) command was introduced
with Git v2.46.0, but will be silently ignored by earlier versions, so
we can use it directly in the script/build-git script called by our
"build-earliest" and "build-latest" CI jobs, including the former, which
builds Git version 2.0.0.

In addition, we can also list the dynamic library dependencies of the
"git-http-fetch" binary we build, since this is one of the Git programs
which must be linked with the libcurl library and we depend on this Git
program in our test suite.  We therefore use either the ldd(1) command
on Linux or the otool(1) command on macOS to display the dependencies
of the "git-http-fetch" binary after we have built and installed it.
Since at least commit d874af9 of
PR git-lfs#1617 we have built the custom versions of Git used in our CI suite
with --jobs=2 or -j2 arguments to the make(1) command, which means
we run no more than two build processes simultaneously.

We may slightly improve the speed of our CI jobs, though, if we use a
higher number of parallel build processes, since we typically now run
our CI suite on hardware with 3 or 4 CPU cores:

  https://github.com/github/docs/blob/7c3c8e7c936fbe1d5d71080098eeccf5cb37bf43/data/reusables/actions/supported-github-runners.md#standard--data-variablesproductprodname_dotcom--hosted-runners-for-public-repositories

Note, too, that our script/cibuild script currently sets the number of
tests run in parallel by the prove(1) command to nine, so using four
parallel build jobs for Git is not excessive by comparison.
Since commit 6285716 in PR git-lfs#4729 we
have installed the gettext library and utilities in our CI and release
GitHub Actions jobs, on both macOS and Ubuntu Linux platforms, while
on Windows we rely on the library and utilities being installed in
advance as part of the Git for Windows environment.

At present, though, the macOS runners provided by GitHub Actions are
always provisioned with a pre-installed version of a recent release of
the gettext library, as packaged by Homebrew, which includes the
msgfmt(1) program used by our Makefile, so we no longer need to install
the "gettext" Homebrew formula ourselves.
Since Go version 1.21, Go supports the designation of a specific version
of the Go toolchain to use when building a module, as described in:

  https://go.dev/doc/toolchain

If the "go" line in a "go.mod" file contains a full version, i.e., one
in the M.N.P format, then this will suffice to define the minimum
required Go toolchain to use when building the module.  Otherwise, Go
will look for a "toolchain" line in the "go.mod" file.

As we do not yet provide either a "toolchain" line or a "go" line with
the full version, running "go get" and other Go utilities may attempt
to insert a "toolchain" line with a recent Go version, such as 1.23.3.
We also receive warnings from CodeQL that our "go.mod" file contains
an invalid toolchain version.

To avoid these issues, we specify a full Go version in the "go" line
of our "go.mod" file, which suffices to define the minimum toolchain
version as well.

We then also run "go mod tidy" to update our module requirements,
which removes a number of entries that have become unnecessary.
@chrisd8088 chrisd8088 merged commit 4a4d330 into git-lfs:main Dec 16, 2024
10 checks passed
@chrisd8088 chrisd8088 deleted the tidy-build-toolchain branch December 16, 2024 01:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants