Allow manual `GIT_BUILT_FROM_COMMIT` specification for installer #8

vdye · 2021-12-02T20:03:55Z

Changes

Allow specification of GIT_BUILT_FROM_COMMIT as make variable when building the MacOS installer package
Add commit hash to installer build & test step for printing git version --build-options

Testing

Test installer build (including corresponding changes to git Makefile): https://github.com/vdye/git_osx_installer/actions/runs/1531273864

derrickstolee · 2021-12-02T20:46:19Z

Related: microsoft/git#472

Changes

Allow specification of GIT_BUILT_FROM_COMMIT as make variable when building the MacOS installer package

Add

I feel like maybe you paused writing here, then forgot to resume?

Testing

Test installer build (including corresponding changes to git Makefile): https://github.com/vdye/git_osx_installer/actions/runs/1531273864

Excellent!

derrickstolee · 2021-12-02T20:48:53Z

.github/workflows/osx-installer.yml

+          sudo installer -verbose -pkg $pkgpath -target /
+
+          # Show build options
+          git version --build-options


We will need to manually verify this output, but that's fine.

couldn't we automate this? Have the build step write an additional artifact with the OID and then test it here.
Might not be worth the trouble (you know my love of all things yml...)

Yes! Should be pretty straightforward to implement, and it makes it clearer when something goes wrong.

Indeed, you can make it an output variable. It is slightly convoluted because you will have to have two output variables (the test-install job can only access the outputs of another job, not of another job's step), see e.g. https://github.com/git/git/blob/v2.34.1/.github/workflows/main.yml#L36, https://github.com/git/git/blob/v2.34.1/.github/workflows/main.yml#L11-L12, and https://github.com/git/git/blob/v2.34.1/.github/workflows/main.yml#L81.

It took a bit of experimentation, but this is now working!

vdye · 2021-12-02T20:50:23Z

Related: microsoft/git#472

Changes

Allow specification of GIT_BUILT_FROM_COMMIT as make variable when building the MacOS installer package

Add

I feel like maybe you paused writing here, then forgot to resume?

Sorry about that, description updated!

jeffhostetler · 2021-12-02T22:00:52Z

Makefile

@@ -63,7 +64,7 @@ XML_CATALOG_FILES=$(shell bin/find-file /usr/local/etc/xml/catalog)
 BUILD_CODE := intel-$(ARCH_CODE)-$(OSX_CODE)
 BUILD_DIR := build/$(BUILD_CODE)
 DESTDIR := $(PWD)/stage/git-$(BUILD_CODE)-$(VERSION)
-SUBMAKE := $(MAKE) C_INCLUDE_PATH="$(C_INCLUDE_PATH)" CPLUS_INCLUDE_PATH="$(CPLUS_INCLUDE_PATH)" LD_LIBRARY_PATH="$(LD_LIBRARY_PATH)" TARGET_FLAGS="$(TARGET_FLAGS)" CFLAGS="$(CFLAGS)" LDFLAGS="$(LDFLAGS)" NO_GETTEXT=1 NO_DARWIN_PORTS=1 prefix=$(GIT_PREFIX) DESTDIR=$(DESTDIR)
+SUBMAKE := $(MAKE) C_INCLUDE_PATH="$(C_INCLUDE_PATH)" CPLUS_INCLUDE_PATH="$(CPLUS_INCLUDE_PATH)" LD_LIBRARY_PATH="$(LD_LIBRARY_PATH)" TARGET_FLAGS="$(TARGET_FLAGS)" CFLAGS="$(CFLAGS)" LDFLAGS="$(LDFLAGS)" NO_GETTEXT=1 NO_DARWIN_PORTS=1 prefix=$(GIT_PREFIX) GIT_BUILT_FROM_COMMIT=$(GIT_BUILT_FROM_COMMIT) DESTDIR=$(DESTDIR)


Wondering if we should have DQuotes around the new value. (and maybe the $(GIT_PREFIX) too.)

I'll try it and, assuming nothing weird happens (like the quotes showing up in git version --build-options), update this.

Didn't see any issues - updated!

Use GIT_BUILT_FROM_COMMIT specified as argument to `make` when building `git`. By default, the value is empty. Signed-off-by: Victoria Dye <[email protected]>

Set `GIT_BUILT_FROM_COMMIT` in the `osx-installer.yml` workflow from the git repository cloned for build. Additionally, install the built installer and verify the built-from commit hash appears properly in `git version --build-options`. Signed-off-by: Victoria Dye <[email protected]>

dscho · 2021-12-05T21:47:00Z

.github/workflows/osx-installer.yml

          if (find ./stage/git-*/usr/local/git/bin/scalar); then
            echo "::set-output name=with_scalar::true"
          else
            echo "::set-output name=with_scalar::false"
          fi
+
+          # Set build commit hash
+          echo "::set-output name=commit_hash::$(git -C git rev-parse -q --verify HEAD 2>/dev/null)"


Are we sure that this is the commit ID we want git config --build-options to show? I had expected something like this appended after line 85:

echo "::set-output name=commit_hash::$GIT_BUILT_FROM_COMMIT"

This is mostly an attempt to keep the sources of the "actual" and "expected" commit hash values as separate as possible. By pulling the "expected" commit hash directly from git rev-parse, the validation later will confirm both that 1) the dist archive was built with the correct commit hash, and 2) that commit hash was properly embedded into the resulting installer.

Fair enough!

dscho · 2021-12-05T21:48:53Z

Makefile

@@ -55,6 +55,7 @@ LDFLAGS := $(TARGET_FLAGS) $(ARCH_FLAGS_${ARCH_CODE})
 BAK_FOLDER := $(shell date +%s)
 PREFIX := /usr/local
 GIT_PREFIX := $(PREFIX)/git
+GIT_BUILT_FROM_COMMIT :=


Does that really work? I thought that := overrides whatever was specified on the command-line, and we would need ?= here. But then, any uninitialized variable VAR would see $(VAR) interpolated to the empty string, no?

It does! According to the documentation, a command line-defined variable overrides all simply (:=) and recursively (=) expanded assignments. As a result, this initializes GIT_BUILT_FROM_COMMIT as an empty string only if it isn't specified on the command line.

vdye mentioned this pull request Dec 2, 2021

Fixes for MacOS release build & build options microsoft/git#472

Merged

derrickstolee approved these changes Dec 2, 2021

View reviewed changes

jeffhostetler reviewed Dec 2, 2021

View reviewed changes

jeffhostetler approved these changes Dec 2, 2021

View reviewed changes

installer: use specified GIT_BUILT_FROM_COMMIT

6e8395d

Use GIT_BUILT_FROM_COMMIT specified as argument to `make` when building `git`. By default, the value is empty. Signed-off-by: Victoria Dye <[email protected]>

vdye force-pushed the bugfix/macos-build-options branch 3 times, most recently from 347fbab to 58b6593 Compare December 3, 2021 16:30

vdye force-pushed the bugfix/macos-build-options branch from 58b6593 to 09b51b4 Compare December 3, 2021 16:40

dscho reviewed Dec 5, 2021

View reviewed changes

dscho approved these changes Dec 6, 2021

View reviewed changes

derrickstolee merged commit 7928044 into derrickstolee:master Dec 6, 2021

vdye deleted the bugfix/macos-build-options branch December 6, 2021 16:05

Allow manual GIT_BUILT_FROM_COMMIT specification for installer #8

Allow manual GIT_BUILT_FROM_COMMIT specification for installer #8

Uh oh!

Conversation

vdye commented Dec 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes

Testing

Uh oh!

derrickstolee commented Dec 2, 2021

Changes

Testing

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vdye commented Dec 2, 2021

Changes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Allow manual `GIT_BUILT_FROM_COMMIT` specification for installer #8

Allow manual `GIT_BUILT_FROM_COMMIT` specification for installer #8

vdye commented Dec 2, 2021 •

edited

Loading