Create Makefile & release pipeline, make `git-bundle-server web-server` more resilient #12

vdye · 2023-02-02T23:28:40Z

Summary

This series is made up of a few loosely-connected changes:

The first 5 commits make miscellaneous changes to git-bundle-server web-server, mainly making the services more resilient to configuration issues & allowing more flexible running (e.g., starting the service and having it continue after a user logs off).
The next 3 commits create the Makefile and packaging logic for MacOS .pkg files and binary Debian .deb files.
The last 2 commits create a release workflow, triggered on tag push, that generates MacOS & Debian packages and attaches them to a draft release.

Prior Art

`microsoft/git` - `build-git-installers.yml`

This release pipeline is the source of both the variable calculation in the prereqs step and the create-github-release step. Note that the latter is updated to handle Promises a bit more carefully, uses a new version of github-script (and therefore changes references of github to github.rest¹), and includes an intermediate step to move artifacts into a single directory before upload.

`GitCredentialManager/git-credential-manager` - misc. scripts (1, 2, 3)

The scripts found in build/package here are derived from those linked in GCM. There were a lot of minor tweaks to adjust to this codebase, but some major differences include:

adding a die "unknown option '$i'" catch to fail if unidentified options are passed to the scripts (rather than just ignoring them)
combining the pkgbuild and productbuild steps into a single script (pkg/pack.sh)
simplifying the productbuild, namely leaving out the distribution.xml & localization resources
- this may be useful for the future, but leaving it out for now in the interest of time
adding git-bundle-server web-server stop --remove to the uninstall scripts, run as the logged-in user (not root, despite the uninstall script otherwise executing as root)
adding a prerm script for the Debian package (to stop and clean up the web server, if needed)

Testing

Successful pipeline run

Draft release (screenshot)

Other

I've locally built, installed, and uninstalled the packages on both my (Intel) Mac and an Ubuntu Codespace.

If you get the chance, I'd love it if y'all could try installing the generated artifacts (found here), especially if you use an M1 Mac. To see if the install worked, run git-bundle-server -h and you should get help text.

Future work

I dropped Apple package signing from this PR, since I don't have a signing certificate to test with yet. I'll open another PR with that once I can test it properly.
I found a bug in git-bundle-server init while testing a local setup. It seems pretty minor, but not particularly related to this PR, so I'll leave it out for now.

https://github.com/actions/github-script#breaking-changes-in-v5 ↩

derrickstolee · 2023-02-03T14:57:54Z

(I co-assigned myself to remind me to review this, but I won't have time until next week.)

jeffhostetler · 2023-02-03T16:00:58Z

um, WOW!!! Let me just stop there.

I want to steal so much of this for my thing... :-)

I promise I'll give it a more "critical" review shortly.

Makefile

build/package/deb/pack.sh

jeffhostetler · 2023-02-03T19:39:26Z

build/package/layout-unix.sh

+	echo "$*" >&2
+	exit 1
+}
+


I find that "set -x" is helpful when there is a lot happening that could go wrong....

I ended up not adding set -x (although it did inspire me to add set -e, which helped avoid all the || exit 1s), since it's probably more verbose than typical execution needs. That said, I think it'll be valuable to have that option, so I opened an issue to add it in the future: #14

build/package/pkg/scripts/postinstall

internal/daemon/launchd.go

jeffhostetler · 2023-02-03T20:23:55Z

It installed successfully on my M1 mac. The git-bundle-server -h command works.

jeffhostetler · 2023-02-03T20:26:34Z

However, when running the uninstall script, I got an error. (I'm not sure that the web-server was running.)

m1 ~/work/git.git % ps auwx | grep git-bun
jeff             12392   0.0  0.0 407962240    176 s018  R+    3:24PM   0:00.00 grep git-bun
m1 ~/work/git.git % sudo /usr/local/git-bundle-server/uninstall.sh 
Password:
Stopping the web server daemon for user 'jeff'...
2023/02/03 15:24:47 Failed with error: could not unload launchd service: 'launchctl bootout' exited with status 5
m1 ~/work/git.git %

And the files are still in /usr/local/

vdye · 2023-02-06T16:39:53Z

However, when running the uninstall script, I got an error. (I'm not sure that the web-server was running.)

Welp, that's what I get for not testing the uninstall script properly - I tested running it after starting the web server, so the plist was populated. I need to add a check that the file exists - if it doesn't, bootout fails with error 5 ("Input/Output error"), rather than 113 as I assumed.

The more annoying thing is that you get a different, more informative error code when running launchctl as root (error 2, "No such file or directory"), but I also don't want to run as root if I can avoid it. I'll figure something out, thanks for testing!

Remove "Whether to" from description of the '--force' option for 'git-bundle-server web-server start'. To a user, the '--force' option is not a boolean, so the description should describe what '--force' does when specified. Signed-off-by: Victoria Dye <[email protected]>

vdye · 2023-02-07T00:18:35Z

@jeffhostetler I've made some changes to git-bundle-server web-server stop --remove, so now it shouldn't fail if the plist hasn't been created/the server hasn't been started. If you want to try again, these artifacts should be able to install & uninstall without the error you saw.

jeffhostetler · 2023-02-07T13:50:21Z

This one installed and uninstalled just fine.

(I never started the web-server, so this was a minimal test at best.)

ldennington

Good stuff! I learned some new things, especially in the Makefile realm.

internal/daemon/launchd.go

ldennington · 2023-02-07T21:49:54Z

Makefile

@@ -0,0 +1,22 @@
+# Default target


Nice! This Makefile is really simple and lovely.

ldennington · 2023-02-07T21:52:40Z

.vscode/settings.json

+    "[makefile]": {
+        "editor.detectIndentation": false,
+        "editor.insertSpaces": false,
+        "editor.tabSize": 8,
+        "editor.wordWrap": "off",
+        "files.trimTrailingWhitespace": true,
+    },
+    "[shellscript]": {
+        "editor.detectIndentation": false,
+        "editor.insertSpaces": false,
+        "editor.tabSize": 8,
+        "editor.wordWrap": "off",
+        "files.trimTrailingWhitespace": true,
+    },


Good guardrails

build/package/pkg/pack.sh

ldennington · 2023-02-07T22:50:22Z

.github/workflows/release.yml

+              draft: true,
+              tag_name: tagName,
+              name: tagName,
+              generate_release_notes: true


I'm looking forward to using the generated release notes in a new project. Having all of the releases match this generated format should help, as opposed to other places where an established release note pattern doesn't match the generated notes.

derrickstolee

I enjoyed reading this. I'm glad you were able to synthesize and improve upon our existing release workflows. Did you notice anything you would like to improve in the originals?

derrickstolee · 2023-02-08T17:50:03Z

internal/daemon/launchd.go

 	if !fileExists || force {
+		// TODO: only overwrite file if file contents have changed


Just noticing this TODO and agreeing that this should be handled later.

.gitignore

derrickstolee · 2023-02-08T18:00:28Z

Makefile

+# Platform information
+GOOS := $(shell go env GOOS)
+GOARCH := $(shell go env GOARCH)


These are set from go env here, but you mention they could be modified later. I'll keep an eye out for that.

Revisiting this: I was incorrect. You are not manually overriding these, but using them to decide which kind of installer is necessary. I was expecting this to need to change to build macOS ARM binaries on intel machines, but I don't see that changing in those workflows.

derrickstolee · 2023-02-08T18:07:21Z

Makefile

+$(PKGDIR)/payload: check-arch build
+	@echo
+	@echo "======== Formatting package contents ========"
+	@build/package/layout-unix.sh --bindir="$(BINDIR)" \


Just noticing that you're able to share a lot in this script between Linux and macOS. layout-unix is a good name for that.

derrickstolee · 2023-02-08T18:09:52Z

.github/workflows/release.yml

+              draft: true,
+              tag_name: tagName,
+              name: tagName,
+              generate_release_notes: true


I'm looking forward to using the generated release notes in a new project. Having all of the releases match this generated format should help, as opposed to other places where an established release note pattern doesn't match the generated notes.

If a service unit configuration file doesn't exist on disk, 'systemctl stop' fails with error code 5 [1]. To keep 'git-bundle-server web-server stop' from failing when 'web-server start' was not called before it, do not cause the program to fail when the error code indicates the service isn't installed. Add a test covering this case. [1] https://freedesktop.org/software/systemd/man/systemd.exec.html#Process%20Exit%20Codes Signed-off-by: Victoria Dye <[email protected]>

Change the launch agent domain from 'gui/<uid>' (tied to login session) to 'user/<uid>' (tied to user, but may run when logged out). To allow the agent to be loaded with this new domain, explicitly set the 'LimitLoadToSessionType' value in the generated plist to 'Background' (default value seems to be 'Aqua', corresponding to the logged in session i.e. 'gui/<uid>' domain). Signed-off-by: Victoria Dye <[email protected]>

Change 'bootoutFile()' helper to 'bootout()' to unload a launchd service based on its service target identifier (e.g. 'gui/502/com.example.myservice') rather than the path to its plist file. This use of 'bootout' is both less fragile (since it does not require a plist file to exist to succeed) and easier to use when performing a "gentle" removal (i.e., suppressing "service not found" errors). Update 'Create()' and the 'launchd' tests with the updated bootout behavior. Signed-off-by: Victoria Dye <[email protected]>

Add a '--remove' option to 'git-bundle-server web-server stop' to fully remove the service daemon and its resources on disk after stopping the daemon process. For launchd, this is done by first running 'launchctl bootout' on the appropriate service, then removing the plist file; for systemd, the service unit file is removed then 'systemctl daemon-reload' is run. Neither implementation fails if the web server was never loaded. Although it is unlikely that users will need this option in day-to-day use, this cleanup option will be needed to fully remove bundle server resources in the uninstall scripts implemented in later patches. Signed-off-by: Victoria Dye <[email protected]>

Later patches will create releasable packages of the repository's software with the name 'git-bundle-server'. To keep the web-server daemon process naming consistent, as well as generally provide a bit of specificity to the daemon name, change 'com.github.bundleserver' to 'com.github.gitbundleserver'. Signed-off-by: Victoria Dye <[email protected]>

Popular convention in Go projects seems to be using Makefiles for automating build/release tasks [1]. In anticipation of adding complex packaging scripts in future patches, create an initial Makefile with basic 'build' and 'clean' targets. Note that the 'build' target utilizes the 'GOOS' and 'GOARCH' of the environment to allow generating executables for any supported Go platform, defaulting to the current platform the build is running on. This behavior will be critical in later patches, when we want to create MacOS ARM64 packages on MacOS Intel build machines. [1] https://github.com/golang-standards/project-layout/tree/master#scripts Signed-off-by: Victoria Dye <[email protected]>

vdye · 2023-02-08T23:53:45Z

Update: after testing on an Ubuntu VM, I found that 1) some fixes I had made to the Debian pack.sh & layout-unix.go got reverted at some point 🙃 and 2) I needed to add handling in systemd.go for when the service is stopped before it's ever started (i.e., the service unit doesn't exist on disk). I think the easiest way to show the changes is with a range diff since e971c13:

Range diff

 -:  ------- >  1:  374758a systemd: do not fail 'stop' if service unit not installed
 1:  4e4341c =  2:  02282c4 launchd: enable running after user logs out
 2:  008d5de =  3:  8c6a468 launchd: unload by service target rather than filename
 3:  7645b2f =  4:  c82b8e0 web-server: add '--remove' option to 'stop'
 4:  d656905 =  5:  7660c16 web-server: rename daemon to align with package naming
 5:  3371111 =  6:  be8f1ea Makefile: create basic 'build' and 'clean' targets
 6:  0fb5df8 !  7:  18c1ddc Makefile: add Debian packaging targets
    @@ Makefile: build:
     +	@echo
     +	@echo "======== Creating binary Debian package ========"
     +	@build/package/deb/pack.sh --payload="$(DEBDIR)/root" \
    ++				   --scripts="$(CURDIR)/build/package/deb/scripts" \
     +				   --arch="$(PACKAGE_ARCH)" \
     +				   --version="$(VERSION)" \
     +				   --output="$(DEB_FILENAME)"
    @@ build/package/deb/pack.sh (new)
     +	DEBROOT="${i#*=}"
     +	shift # past argument=value
     +	;;
    ++	--scripts=*)
    ++	SCRIPT_DIR="${i#*=}"
    ++	shift # past argument=value
    ++	;;
     +	--arch=*)
     +	ARCH="${i#*=}"
     +	shift # past argument=value
    @@ build/package/deb/pack.sh (new)
     +mkdir -p "$(dirname "$DEBOUT")"
     +
     +# Build .deb
    -+mkdir -p "$DEBROOT/DEBIAN"
    ++mkdir -m 755 -p "$DEBROOT/DEBIAN"
     +
     +# Create the debian control file
     +cat >"$DEBROOT/DEBIAN/control" <<EOF
    @@ build/package/deb/pack.sh (new)
     +Description: A self-hostable Git bundle server.
     +EOF
     +
    ++# Copy the maintainer scripts, if they exist
    ++if [ -d "$SCRIPT_DIR" ]; then
    ++	cp -R  "$SCRIPT_DIR/." "$DEBROOT/DEBIAN"
    ++fi
    ++
     +dpkg-deb -Zxz --build "$DEBROOT" "$DEBOUT"
     
      ## build/package/deb/scripts/prerm (new) ##
    @@ build/package/deb/scripts/prerm (new)
     +#!/bin/bash
     +set -e
     +
    -+# Stop & cleanup the web server
    -+/usr/local/git-bundle-server/bin/git-bundle-server web-server stop --remove
    ++# Stop & cleanup the web server as the logged-in user
    ++# The XDG_RUNTIME_DIR is required for 'systemctl' to work, so we manually set it
    ++# to that of the logged-in user.
    ++LOGGED_IN_USER="${SUDO_USER:-${USER}}"
    ++LOGGED_IN_UID="$(sudo -u $LOGGED_IN_USER id -u)"
    ++sudo -u $LOGGED_IN_USER XDG_RUNTIME_DIR=/run/user/$LOGGED_IN_UID \
    ++	/usr/local/git-bundle-server/bin/git-bundle-server web-server stop --remove
     +
     +exit 0
     
    @@ build/package/layout-unix.sh (new)
     +fi
     +
     +# Ensure payload directory exists
    -+INSTALL_TO="$PAYLOAD/usr/local/git-bundle-server/"
    ++INSTALL_TO="$PAYLOAD/usr/local/git-bundle-server"
     +mkdir -p "$INSTALL_TO"
     +
     +# Copy built binaries
    @@ build/package/layout-unix.sh (new)
     +
     +# Create symlinks
     +if [ -n "$INCLUDE_SYMLINKS" ]; then
    -+	LINK_TO="$DEBROOT/usr/local/bin/"
    ++	LINK_TO="$PAYLOAD/usr/local/bin"
     +	mkdir -p "$LINK_TO"
     +
     +	echo "Creating symlinks..."
 7:  dfdc338 !  8:  54f27ff Makefile: add MacOS packaging targets
    @@ build/package/layout-unix.sh: mkdir -p "$INSTALL_TO"
     +
      # Create symlinks
      if [ -n "$INCLUDE_SYMLINKS" ]; then
    - 	LINK_TO="$DEBROOT/usr/local/bin/"
    + 	LINK_TO="$PAYLOAD/usr/local/bin"
     
      ## build/package/pkg/entitlements.xml (new) ##
     @@
 8:  91fe6c6 =  9:  5e797e8 release.yml: create release pipeline
 9:  e971c13 = 10:  d9ef2bd release.yml: create draft release with artifacts

The good news is that I've confirmed the user-scoped service in systemd keeps running after logging out, so we don't need to mess with any root-privileged settings there. I've also successfully installed and uninstalled there, checking the behavior both when the web-server is never started, and when it has been started.

EDIT: and here's a new release build

build/package/deb/pack.sh

derrickstolee · 2023-02-09T14:46:02Z

I tested this on my Intel Mac and installation succeeded without issue. I ran into #13, so I added my error message to that description.

After fixing the missing directory issue, I did get this prompt:

I suppose this is inevitable for the macOS ecosystem, but something to be aware of, right?

Add 'package' target to Makefile that, based on the 'GOOS', builds the platform-appropriate package. For now, support only binary .deb packages (corresponding to a 'GOOS' of 'linux'); if the 'GOOS' is unsupported, 'make' will exit with an error. The .deb packaging process is broken into three steps, each corresponding to a target in the Makefile: 1. Build the executables. 2. Copy the executables & any symlinks into the appropriate file structure for running 'dpkg-deb'. 3. Generate the control file, copy the maintainer script(s) [1], and build the package with 'dpkg-deb'. Additionally, the 'check-arch' and 'check-version' targets verify that the 'GOARCH' is supported for packaging on the platform and that a version string is set, respectively. In accordance with the 'project-layout' guidelines for the Makefile [2], most of the packaging logic outlined above lives in dedicated scripts. Best practice for the location of these scripts is somewhat vague, since it appears that both 'scripts/' [3] and 'build/package/' [4] would be valid choices. Go with the latter, putting the scripts in 'build/package' (if they will apply to more than one packaging mechanism) or a 'deb' subdirectory for Debian-specific configuration. [1] https://www.debian.org/doc/debian-policy/ap-flowcharts.html [2] https://github.com/golang-standards/project-layout/tree/master#scripts [3] https://github.com/golang-standards/project-layout/blob/master/scripts/README.md [4] https://github.com/golang-standards/project-layout/blob/master/build/README.md Signed-off-by: Victoria Dye <[email protected]>

Add support for creating MacOS .pkg packages in the Makefile. Similar to the Debian packaging routine, this is broken into four steps: 1. Build the executables. 2. Copy executables & 'uninstall.sh' into the appropriate file structure to build the .pkg file. 3. Build the component package with 'pkgbuild' and the full package with 'productbuild'. Signed-off-by: Victoria Dye <[email protected]>

Create a basic release pipeline that triggers when a tag of the correct format ("v<number>.<number>.<number>...") is pushed. The pipeline generates three artifacts: 1. An x86/64 .deb binary package 2. An x86/64 MacOS .pkg 3. An ARM64 MacOS .pkg At the moment, these packages are simply uploaded as artifacts in the workflow. In the future, the goal is to automatically create and attach these artifacts to a draft release, to allow maintainers to add release note details then publish with the necessary attachments. Note that the tag filtering in GitHub Actions isn't fancy enough to allow for full verification of the version, so a tag like "v1a.2b.3c-xyz" would trigger the pipeline. Better protections will be put in place later, but for now we can rely on maintainers to push only valid tags for the pipeline. Signed-off-by: Victoria Dye <[email protected]>

Add a job, dependent on the success of all packaging steps, to download the generated artifacts and attach them to a newly-created draft release. The created draft release description is populated with auto-generated release notes [1]. Note the intermediate step "Consolidate artifact directory"; because downloading all artifacts with the 'download-artifacts' action creates a wrapper directory around each artifact [2], we remove those wrappers by moving all files into a single consolidated 'artifacts' directory before running the release creation & attachment script. [1] https://docs.github.com/en/repositories/releasing-projects-on-github/automatically-generated-release-notes [2] https://github.com/actions/download-artifact#download-all-artifacts Signed-off-by: Victoria Dye <[email protected]>

vdye · 2023-02-09T17:34:05Z

I suppose this is inevitable for the macOS ecosystem, but something to be aware of, right?

That's interesting, I haven't seen that notification (whether I install from a package built locally or from the one generated by the release pipeline). The only one I've encountered is this one when starting the web server:

My guess would be that it's something cron related, especially if you saw it after running init. I'll keep an eye out for that and other errors as I keep testing.

jeffhostetler · 2023-02-09T18:04:24Z

I've had to add git-telemetry-service manually to my firewall settings. It's well hidden under the "options..." page on the "firewall" page. However, it looks like there might be a way to add an exception/open a port from the command line.
I haven't played with it yet. See man pfctl and
https://www.macworld.com/article/671729/mac-firewall-how-to-open-specific-ports-in-os-x-10-10-firewall.html

jeffhostetler · 2023-02-09T18:32:36Z

Hey, just found this while working on a preview package (and stealing generously from this PR). There's an
option to go build to let you inject a version number into the source. I though you might be interested.

With source looking like this:

% more main_version.go 
package main

// This value will be overwritten during release builds.
var MainVersion string = "v0.0.0"

We can force a new value at compile time using:

go build -ldflags="-X 'main.MainVersion=v1.2.3'"

I'm still experimenting, but it looks promising.

derrickstolee

Thanks for all the details and significant work here. It's a great foundation to expand upon more later.

vdye requested review from derrickstolee, jeffhostetler and ldennington February 2, 2023 23:28

vdye self-assigned this Feb 2, 2023

derrickstolee self-assigned this Feb 3, 2023