Skip to content

Fixes for MacOS release build & build options #472

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.

Sign up for GitHub

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

Merged
merged 4 commits into from
Dec 6, 2021
Merged

Fixes for MacOS release build & build options #472

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
29c3a39
Makefile: correct the dependency graph of hook-list.h
avar Nov 17, 2021
e7e8d9c
Makefile: allow specifying GIT_BUILT_FROM_COMMIT
vdye Nov 30, 2021
131abfa
dist: archive HEAD instead of HEAD^{tree}
vdye Dec 2, 2021
e64bb71
release: include GIT_BUILT_FROM_COMMIT in MacOS build
vdye Dec 2, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions .github/workflows/build-git-installers.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -354,6 +354,9 @@ jobs:
cp git/git-$VERSION.tar.gz git/git-manpages-$VERSION.tar.gz git_osx_installer/build/ ||
die "Could not copy .tar.gz files"

GIT_BUILT_FROM_COMMIT=$(gunzip -c git/git-$VERSION.tar.gz | git get-tar-commit-id) ||
die "Could not determine commit for build"

# drop the -isysroot `GIT_SDK` hack
sed -i .bak -e 's/ -isysroot .(SDK_PATH)//' git_osx_installer/Makefile || die "Could not drop the -isysroot hack"

Expand All @@ -371,6 +374,7 @@ jobs:

PATH=/usr/local/bin:$PATH \
make -C git_osx_installer \
GIT_BUILT_FROM_COMMIT=$GIT_BUILT_FROM_COMMIT \
OSX_VERSION=10.15 C_INCLUDE_PATH="$C_INCLUDE_PATH" V=1 image ||
die "Build failed"

Expand Down
1 change: 1 addition & 0 deletions .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
/fuzz-pack-headers
/fuzz-pack-idx
/GIT-BUILD-OPTIONS
/GIT-BUILT-FROM-COMMIT
/GIT-CFLAGS
/GIT-LDFLAGS
/GIT-PREFIX
Expand Down
25 changes: 18 additions & 7 deletions Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -390,6 +390,10 @@ all::
# Define GIT_USER_AGENT if you want to change how git identifies itself during
# network interactions. The default is "git/$(GIT_VERSION)".
#
# Define GIT_BUILT_FROM_COMMIT if you want to force the commit hash identified
# in 'git version --build-options' to a specific value. The default is the
# commit hash of the current HEAD.
#
# Define DEFAULT_HELP_FORMAT to "man", "info" or "html"
# (defaults to "man") if you want to have a different default when
# "git help" is called without a parameter specifying the format.
Expand Down Expand Up @@ -2147,6 +2151,15 @@ GIT-USER-AGENT: FORCE
echo '$(GIT_USER_AGENT_SQ)' >GIT-USER-AGENT; \
fi

GIT_BUILT_FROM_COMMIT = $(eval GIT_BUILT_FROM_COMMIT := $$(shell \
Copy link
Member

Choose a reason for hiding this comment

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

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)

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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 😁

GIT_CEILING_DIRECTORIES="$$(CURDIR)/.." \
git rev-parse -q --verify HEAD 2>/dev/null))$(GIT_BUILT_FROM_COMMIT)
GIT-BUILT-FROM-COMMIT: FORCE
@if test x'$(GIT_BUILT_FROM_COMMIT)' != x"`cat GIT-BUILT-FROM-COMMIT 2>/dev/null`" ; then \
Copy link
Member

Choose a reason for hiding this comment

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

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 \
[...]

Copy link
Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

echo >&2 " * new built-from commit"; \
echo '$(GIT_BUILT_FROM_COMMIT)' >GIT-BUILT-FROM-COMMIT; \
fi

ifdef DEFAULT_HELP_FORMAT
BASIC_CFLAGS += -DDEFAULT_HELP_FORMAT='"$(DEFAULT_HELP_FORMAT)"'
endif
Expand Down Expand Up @@ -2262,21 +2275,19 @@ git$X: git.o GIT-LDFLAGS $(BUILTIN_OBJS) $(GITLIBS)
$(filter %.o,$^) $(LIBS)

help.sp help.s help.o: command-list.h
hook.sp hook.s hook.o: hook-list.h
builtin/bugreport.sp builtin/bugreport.s builtin/bugreport.o: hook-list.h
Copy link

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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.


builtin/help.sp builtin/help.s builtin/help.o: config-list.h hook-list.h GIT-PREFIX
builtin/help.sp builtin/help.s builtin/help.o: config-list.h GIT-PREFIX
Copy link

Choose a reason for hiding this comment

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

also the hook-list change is curious.

Copy link
Author

Choose a reason for hiding this comment

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

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).

builtin/help.sp builtin/help.s builtin/help.o: EXTRA_CPPFLAGS = \
'-DGIT_HTML_PATH="$(htmldir_relative_SQ)"' \
'-DGIT_MAN_PATH="$(mandir_relative_SQ)"' \
'-DGIT_INFO_PATH="$(infodir_relative_SQ)"'

version.sp version.s version.o: GIT-VERSION-FILE GIT-USER-AGENT
version.sp version.s version.o: GIT-VERSION-FILE GIT-USER-AGENT GIT-BUILT-FROM-COMMIT
version.sp version.s version.o: EXTRA_CPPFLAGS = \
'-DGIT_VERSION="$(GIT_VERSION)"' \
'-DGIT_USER_AGENT=$(GIT_USER_AGENT_CQ_SQ)' \
'-DGIT_BUILT_FROM_COMMIT="$(shell \
GIT_CEILING_DIRECTORIES="$(CURDIR)/.." \
git rev-parse -q --verify HEAD 2>/dev/null)"'
'-DGIT_BUILT_FROM_COMMIT="$(GIT_BUILT_FROM_COMMIT)"'
Copy link
Member

Choose a reason for hiding this comment

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

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).

Copy link

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Author

@vdye vdye Dec 3, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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


$(BUILT_INS): git$X
$(QUIET_BUILT_IN)$(RM) $@ && \
Expand Down Expand Up @@ -3263,7 +3274,7 @@ dist: git-archive$(X) configure
@$(MAKE) -C git-gui TARDIR=../.dist-tmp-dir/git-gui dist-version
./git-archive --format=tar \
$(GIT_ARCHIVE_EXTRA_FILES) \
--prefix=$(GIT_TARNAME)/ HEAD^{tree} > $(GIT_TARNAME).tar
--prefix=$(GIT_TARNAME)/ HEAD > $(GIT_TARNAME).tar
Copy link

Choose a reason for hiding this comment

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

likewise the ^{tree} change.

Copy link
Author

Choose a reason for hiding this comment

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

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).

@$(RM) -r .dist-tmp-dir
gzip -f -9 $(GIT_TARNAME).tar

Expand Down