Fixes for MacOS release build & build options

[vdye]

Depends on: derrickstolee/git_osx_installer#8

Changes

• Update make dist to archive the HEAD commit, rather than the HEAD tree (preserves commit hash in tar header)
• Allow manual specification of GIT_BUILT_FROM_COMMIT (like e.g. GIT_USER_AGENT)
• Use make dist archive commit hash as GIT_BUILT_FROM_COMMIT in build-git-installers.yml
• Fix dependency issue with hook-list.h
  • Pulled (early) from upstream
  • Dependency issue only seems to appear consistently on macos-latest - for some reason, macos-10.15 doesn't show the same issue. Regardless, this is a good fix to have in case of a mid-cycle release

Testing

• Test installer build (including corresponding changes to git_osx_installer Makefile): https://github.com/vdye/git_osx_installer/actions/runs/1531273864
• build-git-installers execution: https://github.com/vdye/git/actions/runs/1532263438
  • Note: this will NOT include the value in git version --build-options, as it relies on the corresponding change in git_osx_installer to be merged

[derrickstolee]

Commits 2 & 3 seem like they are upstreamable, if you are interested.

[jeffhostetler]

I'm not sure what these hook vs bugreport changes are doing here.

[vdye]

This was intentionally pulled in from an upstream commit because of a build error that was (for some reason) only manifesting in make dist on MacOS 11: https://github.com/vdye/git_osx_installer/runs/4387194585?check_suite_focus=true

I included it "early" (rather than letting it naturally show up as part of next release's rebase) because the error caused by incorrect dependencies is blocking to any mid-cycle releases we may want to do. It does have some things not explicitly needed (e.g., the removal of hook-list.h as a dependency of help), but including the full commit seemed clearer.

[jeffhostetler]

also the hook-list change is curious.

[vdye]

This came with the other hook-list.h in the upstream patch - it's not directly related to the dependency bugfix I wanted out of this commit, but I also didn't want to split the commit (especially because I imagine it'll make its way into git/git master by next release, at which point this should disappear in the rebase).

[jeffhostetler]

likewise the ^{tree} change.

[vdye]

Removing the ^{tree} makes git archive evaluate with a commit object rather than a tree, which ultimately lets it set the commit hash as a header in the generated .tar file (see the second paragraph here). That header can then be used to set GIT_BUILT_FROM_COMMIT when building from an extracted archive (e.g., in the MacOS build).

[jeffhostetler]

very minor nit: using := rather than plain = would avoid multiple evals of the sub-shell expression.
not sure it is worth the bother though.

[dscho]

Wouldn't := force the evaluation, even if the variable is not even used?

[vdye]

I think both of you are right. In this case, maybe a deferred single expansion would work (won't evaluate until needed, and only evaluates once)?

GIT_BUILT_FROM_COMMIT = $(eval GIT_BUILT_FROM_COMMIT := $$(shell \
	GIT_CEILING_DIRECTORIES="$$(CURDIR)/.." \
	git rev-parse -q --verify HEAD 2>/dev/null))$(GIT_BUILT_FROM_COMMIT)

CURL_LDFLAGS and CURL_CFLAGS do something similar (context).

[dscho]

But a ?= would only evaluate unless set, right? So if you specify via the command-line, it won't be evaluated. And once evaluated, it is set, and won't evaluate again either.

[dscho]

Since we already have a shell script snippet here, how about moving the assignment of GIT_BUILT_FROM_COMMIT into the GIT-BUILT-FROM-COMMIT rule? Something like:

@GIT_BUILT_FROM_COMMIT="$(GIT_CEILING_DIRECTORIES="$(CURDIR)/.." \
    git rev-parse -q --verify HEAD 2>/dev/null)" && \
if test x"$GIT_BUILT_FROM_COMMIT" != x"`cat GIT-BUILT-FROM-COMMIT 2>/dev/null`" ; then \
[...]

[vdye]

Would this set GIT_BUILT_FROM_COMMIT as a Makefile variable, or would it only exist local to that target?

[dscho]

That would be a shell variable, local to this target.

[dscho]

Since we take pains to write that file, the value should probably be read from the file instead of relying on the variable, right?

-DGIT_BUILT_FROM_COMMIT="$(cat GIT-BUILT-FROM-COMMIT)"

But if we do that, we need to change patch 4 to write that file instead of specifying the value in the make invocation.

But maybe the idea was to make GIT_BUILT_FROM_COMMIT overrideable from the command-line? In that case, I would suggest not to write GIT-BUILT-FROM-COMMIT at all, but instead use ?= when defining the variable (which should avoid multiple evaluations, just like the := idea Jeff pointed out).

[jeffhostetler]

I think we need the file to be written when the OID changes as she in the Makefile to cause Make to later detect that the dependencies are stale and cause version.o to be recompiled, right?

[dscho]

Oh, but of course! The file is there only to force recompilation if GIT_BUILT_FROM_COMMIT changed...

[vdye]

The idea was, as you suggested, to make GIT_BUILT_FROM_COMMIT overrideable on the command line. It mimics some of the other overrideable variables (GIT_USER_AGENT, PERL_DEFINES, etc.) in the Makefile, which also a) write to a file, and b) do not read from that file when using the value. My interpretation was that the file is used identify whether the value changed on subsequent builds (the "new built-from commit"), and that the Makefile variable is what's actually used.

Also, this reminded me that I missed something (documentation at the top of the file indicating that GIT_BUILT_FROM_COMMIT is overrideable via the command line).

EDIT: whoops! submitted this without refreshing my page first

[dscho]

Do we need a ?= here, to avoid this eval from happening in case the user specified it on the command-line? I.e. like this:

GIT_BUILT_FROM_COMMIT ?= $(shell GIT_CEILING_DIRECTORIES="$(CURDIR)/.." \
    git rev-parse -q --verify HEAD 2>/dev/null)

[vdye]

The command line-specified variable will override any = or := assignment for that variable (documentation). I can't infer any "rule" or pattern for when ?= vs. = is used in this Makefile, but the variables most similar to GIT_BUILT_FROM_COMMIT (e.g. GIT_USER_AGENT) use non-conditional assignments.

[dscho]

Okay, thank you! I thought ?= was the thing to use if one wanted to define some variable that had not yet been defined, but that was not based on reading the documentation 😁