Improve libgit2 build reliability by hbmartin · Pull Request #97 · gitx/objective-git

hbmartin · 2026-07-14T14:11:59Z

Summary

declare the libgit2 build script inputs and archive output so Xcode can skip unchanged rebuilds
skip the expensive libgit2 rebuild when no source file is newer than the archive
use POSIX-compatible shell syntax and restrict the current build configurations to the supported architecture

Why

GitX rebuilds libgit2 on every application and test invocation, substantially increasing local and CI turnaround time. The build phase did not declare its dependencies, and its script had no freshness check.

Validation

sh -n script/update_libgit2
GitX Debug application build
GitX unit and repository-integration suite: 34 passed, 0 failed

Copilot

🟡 Changes recommended

Unresolved cache invalidation, dependency tracking, error handling, and architecture issues block approval.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

Improves libgit2 build reliability through incremental rebuilds and Xcode dependency tracking.

Changes:

Adds archive freshness checks and POSIX shell syntax.
Declares Xcode script inputs and output.
Restricts supported build architectures.

File summaries

File	Review summary
`script/update_libgit2`	Adds caching, but revision/deletion changes, architecture differences, build recipe changes, and masked `find` failures can leave stale or incompatible archives.
`ObjectiveGitFramework.xcodeproj/project.pbxproj`	Adds dependency declarations, but the source set is incomplete and excluding `x86_64` conflicts with the Intel CI job.

Review details

Suppressed comments (4)

ObjectiveGitFramework.xcodeproj/project.pbxproj:1790

The Release project configuration is used by the archive job, but excluding x86_64 here leaves no valid architecture on the workflow's macos-15-intel job (.github/workflows/BuildPR.yml:17-22). The Mac archive therefore fails on Intel. Either keep x86_64 enabled or update/remove that CI job as part of this change.

				EXCLUDED_ARCHS = x86_64;

ObjectiveGitFramework.xcodeproj/project.pbxproj:1926

This project-level Test setting excludes x86_64, so the repository's Intel test/build configuration cannot produce a valid architecture. This conflicts with the Intel job still declared in .github/workflows/BuildPR.yml:17-22; retain that architecture or update the job and supported-architecture contract together.

				EXCLUDED_ARCHS = x86_64;

ObjectiveGitFramework.xcodeproj/project.pbxproj:2145

This project-level Profile setting excludes x86_64, making Profile builds on the repository's Intel environment unable to select a valid architecture. It is inconsistent with the Intel build matrix in .github/workflows/BuildPR.yml:17-22; retain that architecture or update the supported-architecture contract and CI together.

				EXCLUDED_ARCHS = x86_64;

script/update_libgit2:8

This mtime scan only considers files that still exist. If the libgit2 submodule is switched to a revision that deletes or renames a source file, find can return no newer file while the old archive remains present and newer than every remaining file, causing the script to exit with stale code. Track the submodule revision or another manifest/sentinel so deletions invalidate the archive.

newer_source=$(find External/libgit2 \
  \( -path 'External/libgit2/.git' -o -path 'External/libgit2/build' \) -prune -o \
  -type f -newer "$product" -print -quit 2>/dev/null || true)

Files reviewed: 2/2 changed files
Comments generated: 5
Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

hannesa2 · 2026-09-16T04:23:36Z

Sorry for the late review, I miss this pull request. My bad

goneng

One blocker from a quick read: the four EXCLUDED_ARCHS = x86_64; additions in the build configurations.

That makes the project unbuildable on Intel Macs, and this repo still runs a macos-15-intel job in CI.

Dropping those four lines should be all it needs. The rest of the PR looks like a real improvement.

EXCLUDED_ARCHS = x86_64 was added to work around archiving on an Apple Silicon host: ARCHS defaults to arm64 + x86_64 there, while script/update_libgit2 builds a single-arch libgit2.a for the host, so the x86_64 slice fails to link. Excluding x86_64 project-wide was the wrong lever. All four settings sat on project-level configurations, so they cascaded to every target; they contradicted ObjectiveGit-Mac's own VALID_ARCHS = "x86_64 arm64"; and they left the macos-15-intel CI job with no buildable architecture. Architecture selection belongs at the invocation instead - ARCHS=, as the workflow already passes, or ONLY_ACTIVE_ARCH, which Debug and Release already set. Building a universal libgit2.a is not an option here because libgit2 links Homebrew OpenSSL, which is native-arch only. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EeDNs6jJrjDDdpo2T1YUDF

The mtime scan only considered files that still existed. Switching the libgit2 submodule to a revision that deletes or renames a source could leave every surviving file older than External/libgit2.a, so the script reported "libgit2 is up to date." and the build linked stale code. Replace it with a stamp file recording a key built from the submodule revision, the submodule working tree state, the host architecture and a hash of this script. That covers revision switches including ones that only delete files, uncommitted edits and deletions, an archive built for the other architecture, and changes to the cmake flags. The submodule's build directory is excluded so its own output does not force a rebuild. Failures are no longer masked: git errors go to stderr instead of 2>/dev/null, and any part of the key that cannot be computed yields an empty key, which rebuilds rather than trusting an archive we cannot account for. The stamp is removed before building and written only after the archive is installed, so an interrupted build cannot look up to date. The run script phase now sets alwaysOutOfDate instead of declaring inputPaths. libgit2's sources cannot be enumerated statically, so listing only CMakeLists.txt let Xcode skip the phase when sources changed - the same staleness, one level up. The script's own check is now the single source of truth, and it is cheap. Also anchor the script to the repository root so the CI invocation and Xcode's $SRCROOT invocation behave identically, stop the product variable shadowing itself, and teach clean_externals and .gitignore about the stamp. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EeDNs6jJrjDDdpo2T1YUDF

The workflow interpolated matrix.arch, but the matrix defines abi, so both jobs passed an empty ARCHS and neither pinned the architecture it claims to test. Use matrix.abi, in the archive step and in the commented-out test step so it is correct if it is re-enabled. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EeDNs6jJrjDDdpo2T1YUDF

Resolves the freshness rewrite against master's switch from BUILD_CLAR to BUILD_TESTS, and accepts master's removal of script/clean_externals, which this branch had only touched to teach it about the build stamp. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EeDNs6jJrjDDdpo2T1YUDF

Copilot

Copilot review overview

🟡 Changes recommended

The freshness check can miss source-content changes, and Xcode dependency optimization is not enabled.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 1 · 1

Open (2)

Git status key misses content changes to modified files · New
Dependency analysis remains disabled for the build phase · New

Resolved since last review (5)

This cache decision keys only on source mtimes, but External/libgit2.a is built for the current…
This freshness guard only compares mtimes of currently existing submodule files with the archive.…
This project-level Debug setting excludes x86_64, while the repository still defines an Intel…
|| true masks every find failure. If the submodule is missing or unreadable while an ignored…
Xcode uses inputPaths for dependency analysis, so with only the script and CMakeLists.txt…

hannesa2 · 2026-09-21T16:37:56Z

+  git -C "$submodule" status --porcelain --untracked-files=all \
+    -- . ':(exclude)build' || return 1


As it's a High finding, I'll give Copilot a try ...

Ok, I don't do it with Copilot 😁

This could be an alternative: copy&past from google

BUILD_KEY=$( ( git -C "$submodule" ls-files -s -- . ':(exclude)build' git -C "$submodule" ls-files -o --exclude-standard -- . ':(exclude)build' ) | sort -u | git -C "$submodule" hash-object --stdin ) || return 1

 		};
 		D0A330F116027F2300A616FA /* libgit2 */ = {
 			isa = PBXShellScriptBuildPhase;
+			alwaysOutOfDate = 1;


hannesa2 · 2026-09-21T16:29:01Z

+#        run: xcodebuild -workspace ObjectiveGitFramework.xcworkspace -scheme "ObjectiveGit Mac" test ARCHS="${{ matrix.abi }}"
      - name: Archive project
-        run: xcodebuild -workspace ObjectiveGitFramework.xcworkspace -scheme "ObjectiveGit Mac" archive ARCHS="${{ matrix.arch }}"
+        run: xcodebuild -workspace ObjectiveGitFramework.xcworkspace -scheme "ObjectiveGit Mac" archive ARCHS="${{ matrix.abi }}"


Indeed there was a bug !

hannesa2

As it works, we could solve it later too. But this is just my opinion

Improve libgit2 build reliability

3fb831e

hbmartin mentioned this pull request Jul 14, 2026

Add native history and harden Git workflows gitx/gitx#558

Closed

hannesa2 requested a lite review from Copilot September 16, 2026 04:17

Copilot started reviewing on behalf of hannesa2 September 16, 2026 04:18 View session

Copilot AI reviewed Sep 16, 2026

View reviewed changes

Comment thread ObjectiveGitFramework.xcodeproj/project.pbxproj Outdated

Comment thread script/update_libgit2 Outdated

Comment thread script/update_libgit2 Outdated

Comment thread ObjectiveGitFramework.xcodeproj/project.pbxproj Outdated

Comment thread script/update_libgit2 Outdated

hannesa2 requested changes Sep 16, 2026

View reviewed changes

Comment thread ObjectiveGitFramework.xcodeproj/project.pbxproj Outdated

Comment thread ObjectiveGitFramework.xcodeproj/project.pbxproj Outdated

goneng mentioned this pull request Sep 21, 2026

Stop building the libgit2 test suite on every build #108

Merged

goneng reviewed Sep 21, 2026

View reviewed changes

claude added 4 commits September 21, 2026 16:15

hannesa2 requested a lite review from Copilot September 21, 2026 16:24

Copilot started reviewing on behalf of hannesa2 September 21, 2026 16:25 View session

Copilot AI reviewed Sep 21, 2026

View reviewed changes

hannesa2 reviewed Sep 21, 2026

View reviewed changes

hannesa2 approved these changes Sep 21, 2026

View reviewed changes

hannesa2 merged commit 61140b4 into gitx:master Sep 22, 2026
2 checks passed

Navigation Menu

Improve libgit2 build reliability - #97

Improve libgit2 build reliability#97
hannesa2 merged 5 commits into
gitx:masterfrom
hbmartin:codex/libgit2-build-reliability

hbmartin commented Jul 14, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

hannesa2 commented Sep 16, 2026

Uh oh!

Uh oh!

Uh oh!

goneng left a comment

Uh oh!

Copilot AI left a comment

Uh oh!

hannesa2 Sep 21, 2026

Uh oh!

hannesa2 Sep 21, 2026

Uh oh!

hannesa2 Sep 21, 2026

Uh oh!

hannesa2 Sep 21, 2026

Uh oh!

hannesa2 left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

		git -C "$submodule" status --porcelain --untracked-files=all \
		-- . ':(exclude)build' \|\| return 1

Conversation

hbmartin commented Jul 14, 2026

Summary

Why

Validation

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Suppressed comments (4)

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

hannesa2 commented Sep 16, 2026

Uh oh!

Uh oh!

Uh oh!

goneng left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🟡 Changes recommended

Uh oh!

hannesa2 Sep 21, 2026

Choose a reason for hiding this comment

Uh oh!

hannesa2 Sep 21, 2026

Choose a reason for hiding this comment

Uh oh!

hannesa2 Sep 21, 2026

Choose a reason for hiding this comment

Uh oh!

hannesa2 Sep 21, 2026

Choose a reason for hiding this comment

Uh oh!

hannesa2 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants