Do not merge — spike #364: ship the Git the app runs on - #380
Closed
juanmaguitar wants to merge 4 commits into
Closed
Do not merge — spike #364: ship the Git the app runs on#380juanmaguitar wants to merge 4 commits into
juanmaguitar wants to merge 4 commits into
Conversation
Throwaway. It exists to get Buildkite to build a signed, notarised artifact with a bundled Git inside it, and to get the CI matrix to answer on Windows what a macOS laptop cannot. - `dugite@3.2.3` as the vehicle, with the Git Credential Manager and git-lfs it also ships excluded: 148 MB of payload becomes 26 MB, and neither is something this app can use — every fetch it makes is anonymous over public HTTPS. - `asarUnpack` for the Git tree, since binaries cannot execute from inside app.asar. There was no `asarUnpack` in the config before this. - Probes under `scripts/spike-364/`, plus two throwaway suites that ride the existing CI matrix: a `node --test` file for the local questions and a packaged spec that clones wordpress-develop from inside the built app. Part of #364. Nothing here is meant to land. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…stion Every Windows check failed on the harness rather than on the app. The layout differs from macOS in three places at once — the binary is `git/cmd/git.exe`, the exec path is under `git/mingw64/libexec/git-core`, and `PATH` has to carry `mingw64/bin` and `mingw64/usr/bin` — and the first pass hand-joined the macOS shape. `resolveGitDir`/`setupEnvironment` already know all three, including the app.asar to app.asar.unpacked rewrite. Also: - `GIT_CONFIG_NOSYSTEM=1` on top of what dugite sets. Its own system gitconfig `include`s the host's `/etc/gitconfig`, which would let a mentor's machine change what the app does. - The trimming globs now name both layouts. Windows is deliberately narrower: MinGit's DLLs are load-bearing, so only the credential manager and git-lfs come out there. - The packaged probe reports `--exec-path` and counts what is in it, so a trim that dropped a helper shows up there rather than mid-clone. Part of #364. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The packaged probe cleaned up its clone with `rmSync`, which threw EPERM on the Windows runner and took the whole run with it — so the steps that had already passed never printed. The throw is worth keeping as an answer rather than removing. Git marks loose objects read-only and `force` does not clear that attribute on Windows, while `sites:delete` deletes site folders today against repositories isomorphic-git made, which carry no such attribute. Swapping the engine would move that from "works" to "EPERM", so it is one more thing #364 has to absorb. Part of #364. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
It is a finding, and a red check that encodes one hides whatever breaks after it. The step still prints; only the assertion drops it. Part of #364. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Collaborator
Author
|
The spike is done and its findings are written up on #364: #364 (comment) Every question answered, including the one only this pull request could — the signed Buildkite artifact notarises with the bundled Git inside, no afterSign hook needed. Closing as intended; nothing here was ever meant to merge. The branch stays up for now so the probes the comment references remain readable. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Do not merge. This branch exists to make CI answer questions a laptop cannot, and will be closed and deleted once it has. Nothing here is meant to land.
Why
#364 proposes that the app carry a real Git binary instead of reimplementing Git's behaviour in JavaScript. #351 says two of its three spikes stop being questions if #364 lands, and that #364's spike is cheaper and goes first. So this decides the engine before the pieces of #350 are built on the wrong one.
The local questions are already answered on macOS — the findings go on #364, not here. Two are not, and only CI can answer them:
What changes
dugite@3.2.3as the vehicle — the package GitHub Desktop uses to ship Git inside an Electron app.git-lfsthat dugite also ships are excluded throughfiles. That is 148 MB of payload down to 26 MB, and neither is usable here: every fetch this app makes is anonymous over public HTTPS.asarUnpackfor the Git tree, because a binary cannot execute from insideapp.asar. There was noasarUnpackin the build config before this.scripts/spike-364/, and two throwaway suites: anode --testfile for the local questions, and a packaged spec that cloneswordpress-developfrom inside the built app.How to test this
Everything below is what CI is being asked; a reviewer does not need to run any of it.
.dmgfrom this branch's Buildkite build, install it, and open it from Finder. Expected: it opens with no Gatekeeper warning — that is the notarisation answer. What must not have happened: a warning that the app is damaged, which is what an unsigned nested binary inside a notarised bundle produces.codesign -dv --verbose=2 "/Applications/WordPress Contributor Toolkit.app/Contents/Resources/app.asar.unpacked/node_modules/dugite/git/bin/git". Expected:Authority=Developer ID Application: Automattic, Inc.What must not have happened:Signature=adhoc, which is what the binary carries as shipped and what the notary service rejects.Journeys/Packaged smokejobs on this pull request run the twospike364suites onwindows-latest. Read their output rather than reproducing it: the interesting lines are whethergit statusreports a clean tree over a checkoutisomorphic-gitmade (thecore.autocrlfquestion) and whether a clone over HTTPS finds its certificates without being told where they are.trunkbuild. Expected: about 26 MB larger per platform.Platforms: macOS and Windows. Linux is out of scope for the spike and snap confinement is recorded as an open risk.
Cannot be tested by hand here: whether the notary service accepts the bundle. That is a service verdict, and the Buildkite log is the only place it appears.
Risks and limitations
None to the product — this branch is never merged. The one real risk is leaving it open: it holds a
dugitedependency and anasarUnpackrule that nothing else in the repo wants yet.Related