diff --git a/.github/workflows/promote-shark-explorer.yml b/.github/workflows/promote-shark-explorer.yml new file mode 100644 index 0000000000..004d37f928 --- /dev/null +++ b/.github/workflows/promote-shark-explorer.yml @@ -0,0 +1,98 @@ +# Tells every running Shark Explorer that a release exists, which is a separate act from publishing it. +# +# The app reads one file — the `latest.properties` asset of the rolling `shark-explorer-latest` release — +# and this workflow is the only thing that writes it. So a release can be published, installed, and tried +# before anybody else is told about it, and a release that turns out to be bad is never announced rather +# than announced and withdrawn. +# +# Run it by hand: `gh workflow run promote-shark-explorer.yml -f version=1.0.0`. +# +# Why a file on the release download CDN rather than the GitHub API, which would need no promotion step at +# all: `releases/latest` answers with the newest release of *either* line, and this repository releases +# LeakCanary on `v*` tags too, so it is usually the wrong one. The unauthenticated API also allows 60 +# requests an hour per IP, which a shared corporate egress can exhaust, while a release asset is an +# ordinary unmetered download. See UpdateCheck.kt. +name: Promote Shark Explorer + +on: + workflow_dispatch: + inputs: + version: + description: 'The released version to start offering, e.g. 1.0.0' + required: true + type: string + +permissions: + contents: read + +jobs: + promote: + runs-on: ubuntu-latest + if: github.repository == 'square/leakcanary' + permissions: + contents: write + steps: + - uses: actions/checkout@v7 + + # A release nobody can download is not one to point at, so this is checked before the tag moves. + - name: Check that the release exists and has its macOS builds + env: + GH_TOKEN: ${{ github.token }} + VERSION: ${{ inputs.version }} + run: | + set -euo pipefail + assets="$(gh release view "shark-explorer-$VERSION" --json assets --jq '.assets[].name')" + echo "$assets" + for arch in arm64 x64; do + echo "$assets" | grep -qx "Shark-Explorer-$VERSION-macos-$arch.dmg" || { + echo "::error::shark-explorer-$VERSION has no macOS $arch DMG, so there is nothing to promote." + exit 1 + } + done + + - name: Write the manifest and move the rolling release onto it + env: + GH_TOKEN: ${{ github.token }} + VERSION: ${{ inputs.version }} + ROLLING_TAG: shark-explorer-latest + run: | + set -euo pipefail + # Read by shark.explorer.app.parseReleaseManifest. Properties rather than JSON so that the app + # parses it with java.util.Properties and needs no JSON dependency for this one file. + cat > latest.properties </dev/null 2>&1; then + gh release create "$ROLLING_TAG" \ + --title "Shark Explorer update manifest" \ + --notes "Not a release. Holds the one file running copies of Shark Explorer read to find out whether a newer release exists. Written by promote-shark-explorer.yml." \ + --prerelease + fi + gh release upload "$ROLLING_TAG" latest.properties --clobber + + # The app fetches this exact URL, so fetching it here is the check that promotion worked. The CDN + # serves the previous asset for a moment after an upload, hence retrying rather than asserting once. + - name: Check that the app's URL now serves the new version + env: + VERSION: ${{ inputs.version }} + run: | + set -euo pipefail + url="https://github.com/${GITHUB_REPOSITORY}/releases/download/shark-explorer-latest/latest.properties" + for attempt in $(seq 1 10); do + served="$(curl -fsSL "$url" | sed -n 's/^version=//p' || true)" + if [[ "$served" == "$VERSION" ]]; then + echo "$url serves version=$VERSION" + exit 0 + fi + echo "Attempt $attempt: $url serves '${served:-nothing}', waiting" + sleep 15 + done + echo "::error::$url did not serve version=$VERSION" + exit 1 diff --git a/.github/workflows/release-shark-explorer.yml b/.github/workflows/release-shark-explorer.yml new file mode 100644 index 0000000000..e755b5505c --- /dev/null +++ b/.github/workflows/release-shark-explorer.yml @@ -0,0 +1,264 @@ +# Releases Shark Explorer, which is released separately from LeakCanary itself. +# +# LeakCanary's libraries go out on `v*` tags through publish-release.yml, to Maven Central. This app goes +# out on `shark-explorer-*` tags, to a GitHub release, on its own version line and its own schedule. The +# two share nothing but the repository. See docs/releasing-shark-explorer.md. +# +# Publishing a release does NOT tell anyone about it. The in-app update check reads the manifest that +# promote-shark-explorer.yml writes, which is a separate, deliberate step. +name: Release Shark Explorer + +on: + push: + tags: + - 'shark-explorer-*' + +permissions: + contents: read + +env: + EXPLORER_MODULE: ':shark:shark-explorer:shark-explorer-app' + +jobs: + # The tag names the version, gradle.properties holds it, and nothing keeps the two the same. Checked + # first and on its own so that a mismatch costs one quick job rather than four packaging jobs and a + # release built out of the wrong version. + version: + runs-on: ubuntu-latest + if: github.repository == 'square/leakcanary' + outputs: + version: ${{ steps.version.outputs.version }} + steps: + - uses: actions/checkout@v7 + - name: Check the tag against SHARK_EXPLORER_VERSION + id: version + run: | + set -euo pipefail + tag_version="${GITHUB_REF_NAME#shark-explorer-}" + built_version="$(sed -n 's/^SHARK_EXPLORER_VERSION=//p' gradle.properties)" + if [[ "$tag_version" != "$built_version" ]]; then + echo "::error::Tag $GITHUB_REF_NAME would build $built_version. Set SHARK_EXPLORER_VERSION=$tag_version, or tag shark-explorer-$built_version." + exit 1 + fi + echo "version=$tag_version" >> "$GITHUB_OUTPUT" + + # macOS is the platform this app is for, and the only one whose artifact is signed. Two jobs rather than + # one universal binary: jpackage builds a thin binary for the architecture it runs on and has no + # universal option, so an Apple Silicon DMG and an Intel DMG are two builds on two runners. + # + # macos-15-intel is the last x86_64 macOS image GitHub will offer, and it goes away in August 2027. When + # it does, either this matrix entry goes or Intel users stay on whatever the last release was. + macos: + needs: version + strategy: + fail-fast: false + matrix: + include: + - runner: macos-15 + arch: arm64 + - runner: macos-15-intel + arch: x64 + runs-on: ${{ matrix.runner }} + permissions: + contents: read + # The signing service authenticates the workflow through OIDC rather than through a secret, which is + # what makes signing from a public repository safe: there is no key here to leak. + id-token: write + steps: + - uses: actions/checkout@v7 + - uses: actions/setup-java@v5 + with: + java-version: 17 + distribution: 'zulu' + - uses: gradle/actions/setup-gradle@0723195856401067f7a2779048b490ace7a47d7c # v5 + with: + # An Actions cache is writable from any branch of the repository and restorable by a tag build, + # so on a workflow whose output people download — and, on macOS, download signed as Block — a + # cache entry is an untrusted input. A release runs a few times a year, so caching buys nothing + # worth that. https://docs.zizmor.sh/audits/#cache-poisoning + cache-disabled: true + + # Not packageReleaseDmg, which runs the artifact through R8. Shark's object inspectors read fields + # by name, so minification is a change that needs testing on its own rather than one to take on + # along with a first release. + - name: Build the DMG + run: ./gradlew ${{ env.EXPLORER_MODULE }}:packageDmg --stacktrace + + - name: Find the unsigned DMG + id: unsigned + run: | + set -euo pipefail + # Found rather than named: jpackage names it after packageName and the version, and the DMG + # this uploads is named below instead, after the architecture the runner built it for. + dmg="$(find shark/shark-explorer/shark-explorer-app/build/compose/binaries/main/dmg -name '*.dmg' -print -quit)" + test -n "$dmg" + echo "path=$dmg" >> "$GITHUB_OUTPUT" + + # Signs and notarizes with `Developer ID Application: Block, Inc.` through Block's internal signing + # service. Ask #mdx-ios to provision OSX_CODESIGN_ROLE and CODESIGN_S3_BUCKET for the repository. + # + # It signs the .app inside the DMG and rebuilds the DMG around it, so the app is notarized and the + # DMG container itself is not signed. Gatekeeper checks the app, which is what has to pass. + - name: Codesign and notarize + id: codesign + uses: block/apple-codesign-action@679535d1ab7c5a7c18e6f9afcba3464512cc3dde # v1.1.0 + with: + osx-codesign-role: ${{ secrets.OSX_CODESIGN_ROLE }} + codesign-s3-bucket: ${{ secrets.CODESIGN_S3_BUCKET }} + unsigned-artifact-path: ${{ steps.unsigned.outputs.path }} + entitlements-plist-path: shark/shark-explorer/shark-explorer-app/entitlements.plist + artifact-name: shark-explorer-${{ needs.version.outputs.version }}-${{ matrix.arch }}-${{ github.run_id }} + + # Says what was actually produced rather than trusting that it was signed. + # + # `stapler validate` failing is fatal, because an app the signing service signed but Apple did not + # notarize does not launch: measured on macOS 26.5, it hangs in dyld with no output and no log file + # rather than being refused. `stapler` is also the only one of these that fails on it — `codesign` + # is happy, and `spctl --assess` still answers "accepted" for a build Apple has no notarization + # record of, because nothing here carries the quarantine attribute that makes Gatekeeper insist on + # a ticket. Its `source=` line is the tell, though: `Notarized Developer ID` against a plain + # `Developer ID`, which is why this prints it. So this is the check standing between a green + # release and a DMG that opens into a hang. + - name: Verify the signature and the notarization + env: + SIGNED_DMG: ${{ steps.codesign.outputs.signed-dmg-path }} + run: | + set -euo pipefail + mount_point="$(hdiutil attach "$SIGNED_DMG" -nobrowse -readonly | grep -o '/Volumes/.*')" + app="$(find "$mount_point" -maxdepth 1 -name '*.app' -print -quit)" + codesign --verify --deep --strict --verbose=2 "$app" + codesign -dvv "$app" 2>&1 | grep -E 'Authority|TeamIdentifier|flags' + spctl --assess --type execute --verbose=4 "$app" || echo "::warning::Gatekeeper did not accept the app" + if ! xcrun stapler validate "$app"; then + echo "::error::No notarization ticket is stapled to the app, so it will not launch. It came back signed, and Apple has no notarization record of it." + hdiutil detach "$mount_point" + exit 1 + fi + hdiutil detach "$mount_point" + + # Renamed here rather than in the build script: the architecture is a property of the runner that + # built it rather than of the project, so jpackage has no way to put it in the name. + - name: Name the DMG after its version and architecture + id: named + env: + SIGNED_DMG: ${{ steps.codesign.outputs.signed-dmg-path }} + VERSION: ${{ needs.version.outputs.version }} + run: | + set -euo pipefail + named="$RUNNER_TEMP/Shark-Explorer-$VERSION-macos-${{ matrix.arch }}.dmg" + mv "$SIGNED_DMG" "$named" + echo "path=$named" >> "$GITHUB_OUTPUT" + + - uses: actions/upload-artifact@v7 + with: + name: shark-explorer-macos-${{ matrix.arch }} + path: ${{ steps.named.outputs.path }} + if-no-files-found: error + + # Unsigned, deliberately. Windows would need Azure Trusted Signing, which is a separate ask and a + # separate service from the Apple one above, and a .deb needs no signature at all. Both are built + # because the build script already targets them and an unsigned build beats no build. + other-platforms: + needs: version + # Windows runners default to pwsh, where `./gradlew` does nothing at all and the step still reports + # success — so the MSI never got built and the first sign of it was a later step not finding the file. + # Git bash is on the image, and it runs the same script the Linux runner does. + defaults: + run: + shell: bash + strategy: + fail-fast: false + matrix: + include: + - runner: windows-latest + task: packageMsi + artifact: windows-x64 + extension: msi + - runner: ubuntu-latest + task: packageDeb + artifact: linux-x64 + extension: deb + runs-on: ${{ matrix.runner }} + steps: + - uses: actions/checkout@v7 + - uses: actions/setup-java@v5 + with: + java-version: 17 + distribution: 'zulu' + - uses: gradle/actions/setup-gradle@0723195856401067f7a2779048b490ace7a47d7c # v5 + with: + # An Actions cache is writable from any branch of the repository and restorable by a tag build, + # so on a workflow whose output people download — and, on macOS, download signed as Block — a + # cache entry is an untrusted input. A release runs a few times a year, so caching buys nothing + # worth that. https://docs.zizmor.sh/audits/#cache-poisoning + cache-disabled: true + + - name: Build the installer + run: ./gradlew ${{ env.EXPLORER_MODULE }}:${{ matrix.task }} --stacktrace + + - name: Name it after its version and platform + id: named + shell: bash + env: + VERSION: ${{ needs.version.outputs.version }} + run: | + set -euo pipefail + built="$(find shark/shark-explorer/shark-explorer-app/build/compose/binaries/main -name "*.${{ matrix.extension }}" -print -quit)" + test -n "$built" + named="$RUNNER_TEMP/Shark-Explorer-$VERSION-${{ matrix.artifact }}.${{ matrix.extension }}" + mv "$built" "$named" + echo "path=$named" >> "$GITHUB_OUTPUT" + + - uses: actions/upload-artifact@v7 + with: + name: shark-explorer-${{ matrix.artifact }} + path: ${{ steps.named.outputs.path }} + if-no-files-found: error + + release: + needs: [ version, macos, other-platforms ] + runs-on: ubuntu-latest + permissions: + contents: write + steps: + - uses: actions/checkout@v7 + - uses: actions/download-artifact@v7 + with: + path: artifacts + merge-multiple: true + + # A prerelease, and titled so. The version number cannot say "alpha" — every installer format + # validates it down to three integers with a non-zero major, see gradle.properties — so this is + # where that gets said instead. + # The notes are a YAML block scalar rather than a shell heredoc because YAML strips this + # indentation and a heredoc would keep it, and four spaces of kept indentation is a Markdown code + # block: the whole release page would render as one. + - name: Create the GitHub release + env: + GH_TOKEN: ${{ github.token }} + VERSION: ${{ needs.version.outputs.version }} + NOTES: | + Shark Explorer ${{ needs.version.outputs.version }}, an alpha release. + + A desktop app that opens an Android heap dump and shows what is holding its memory, as a + navigable treemap. Download the build for your platform below, open it, and drag the app to + Applications. + + | Platform | Download | Signed | + | --- | --- | --- | + | macOS, Apple Silicon | `Shark-Explorer-${{ needs.version.outputs.version }}-macos-arm64.dmg` | Yes | + | macOS, Intel | `Shark-Explorer-${{ needs.version.outputs.version }}-macos-x64.dmg` | Yes | + | Windows | `Shark-Explorer-${{ needs.version.outputs.version }}-windows-x64.msi` | No | + | Linux | `Shark-Explorer-${{ needs.version.outputs.version }}-linux-x64.deb` | No | + + The macOS builds are signed and notarized by Block. The Windows and Linux builds are not + signed, so their installers will warn. + + What changed: https://square.github.io/leakcanary/shark-explorer-changelog/ + run: | + set -euo pipefail + gh release create "$GITHUB_REF_NAME" \ + --title "Shark Explorer $VERSION (alpha)" \ + --notes "$NOTES" \ + --prerelease \ + artifacts/* diff --git a/docs/changelog.md b/docs/changelog.md index 9d8fad0313..c97fa07082 100644 --- a/docs/changelog.md +++ b/docs/changelog.md @@ -3,6 +3,9 @@ Please thank our [contributors](https://github.com/square/leakcanary/graphs/contributors) 🙏 🙏 🙏. +This covers the LeakCanary and Shark libraries. Shark Explorer, the desktop app, is released on its own +schedule and has its own [Shark Explorer change log](shark-explorer-changelog.md). + Each entry starts with a marker for the kind of change it is: | | Means | diff --git a/docs/releasing-shark-explorer.md b/docs/releasing-shark-explorer.md new file mode 100644 index 0000000000..3c798d32fb --- /dev/null +++ b/docs/releasing-shark-explorer.md @@ -0,0 +1,207 @@ +# Releasing Shark Explorer + +Shark Explorer is a desktop app, not a library, and it is released **separately from LeakCanary**. This +page is that process. [Releasing LeakCanary](releasing.md) is the other one, and the two share nothing but +the repository. + +| | LeakCanary | Shark Explorer | +| --- | --- | --- | +| Tags | `v3.0-alpha-10` | `shark-explorer-1.0.0` | +| Version in | `VERSION_NAME` | `SHARK_EXPLORER_VERSION` | +| Goes to | Maven Central | a GitHub release | +| Workflow | `publish-release.yml` | `release-shark-explorer.yml` | +| Change log | [changelog.md](changelog.md) | [shark-explorer-changelog.md](shark-explorer-changelog.md) | + +Two release schedules means two change logs. **Shark Explorer changes never go in the LeakCanary change +log**, and the reverse: a reader of either one is asking about one release line. + +## The version can't say "alpha", so the release does + +`SHARK_EXPLORER_VERSION` is three integers and nothing else. Every installer format validates it, and +between them they leave only `MAJOR.MINOR.PATCH` with **MAJOR between 1 and 255**, MINOR up to 255 and +PATCH up to 65535 — macOS rejects a MAJOR of 0, and MSI rejects anything over 255. So there is no number +that means "before 1.0": not `0.1.0`, not a calendar version, and no qualifier like `-alpha-1`. + +What follows is that **the release says it instead**. `release-shark-explorer.yml` marks every release as a +prerelease and titles it `Shark Explorer (alpha)`. Drop that when the app is no longer alpha. + +## Cutting a release + +Set the version, tag it, and let CI do the rest. The workflow refuses to run if the tag and +`SHARK_EXPLORER_VERSION` disagree, so this order matters. + +```bash +printf "Version being released (e.g. 1.0.1): " && read NEW_VERSION +git checkout main && git pull && \ +git checkout -b shark_explorer_$NEW_VERSION && \ +sed -i '' "s/SHARK_EXPLORER_VERSION=.*/SHARK_EXPLORER_VERSION=$NEW_VERSION/" gradle.properties +``` + +Rename the `## Unreleased` heading in +[`docs/shark-explorer-changelog.md`](shark-explorer-changelog.md) to `## Version $NEW_VERSION ()`, +check it lists everything that landed since the last one, and commit: + +```bash +"${EDITOR:-vi}" docs/shark-explorer-changelog.md && \ +git commit -am "Release Shark Explorer $NEW_VERSION" +``` + +Merge that to `main`, then tag it: + +```bash +git tag shark-explorer-$NEW_VERSION && \ +git push origin shark-explorer-$NEW_VERSION && \ +gh run watch $(gh run list --workflow=release-shark-explorer.yml --limit 1 --json databaseId --jq '.[].databaseId') --exit-status +``` + +There is no `-SNAPSHOT` dance here, unlike LeakCanary: nothing consumes this version as a dependency, so +`main` carrying the last released number between releases costs nothing. + +What the workflow builds: + +* **macOS arm64 and x64**, signed and notarized (see below). Two builds because jpackage produces a thin + binary for the architecture it runs on and has no universal option. +* **Windows `.msi` and Linux `.deb`**, unsigned. + +## Telling people about it is a separate step + +Publishing a release does **not** offer it to anyone. The app checks one file — the `latest.properties` +asset of the rolling `shark-explorer-latest` release — and only `promote-shark-explorer.yml` writes it: + +```bash +gh workflow run promote-shark-explorer.yml -f version=$NEW_VERSION +``` + +So install the release and open a heap dump with it before running that. A release that turns out to be +broken is then one nobody was told about, rather than one that has to be withdrawn. + +The release notes link to the change log page, which is only live once the site is deployed: + +```bash +rm -rf docs/api && ./gradlew siteDokka && mkdocs gh-deploy +``` + +Two things this shares with [releasing LeakCanary](releasing.md), for the same reasons. `siteDokka` is +not optional even though nothing about an explorer release touches the API reference: `docs/api` is +generated and git ignored, so `gh-deploy` without it publishes a site whose API pages 404. And this +deploys **the whole site from your checkout**, so run it from `main` rather than from a branch carrying +unrelated documentation work. + +Two things about that mechanism that look like accidents and aren't: + +* **It is not the GitHub API.** `releases/latest` returns the newest release of *either* line, and this + repository publishes LeakCanary on `v*` tags, so that endpoint usually answers with the wrong release + entirely. The unauthenticated API is also 60 requests an hour **per IP**, which a shared corporate + egress can exhaust; a release asset is an ordinary unmetered CDN download. +* **The app only ever reports.** It shows a bar naming the new version with a link, and nothing downloads + or installs itself. See `UpdateCheck.kt`. + +## macOS signing + +Handled by [`block/apple-codesign-action`](https://github.com/block/apple-codesign-action), which signs and +notarizes with `Developer ID Application: Block, Inc.` through Block's internal signing service. Nothing +in this repository holds a certificate or an Apple credential: the workflow authenticates over OIDC and the +signing happens elsewhere, which is what makes this safe to do from a public repository. + +It needs two repository secrets, `OSX_CODESIGN_ROLE` and `CODESIGN_S3_BUCKET`. **Ask `#mdx-ios` to +provision them** — that team owns Apple codesigning at Block for macOS desktop apps as well as iOS ones, +which is how `block/qrgo` and `block/buzz` are signed. + +Two things to know about the result: + +* The service signs the `.app` and rebuilds the DMG around it, so **the DMG container itself is unsigned**. + Gatekeeper assesses the app, which is the part that has to pass, and the workflow runs `codesign + --verify`, `stapler validate` and `spctl --assess` on the app inside the DMG and prints what they say. + Read those, especially on a first release. +* `entitlements.plist` next to the app module is not optional. Every key in it is something the JVM does + and the hardened runtime forbids by default, so a notarized build without them launches and immediately + dies. + +Windows is unsigned, and signing it would mean Azure Trusted Signing — a different service and a separate +ask. Linux `.deb` needs no signature. + +## Managed Software Center, for Block employees + +Optional, and worth doing only for discoverability: the in-app check already covers updates. File a ticket +at `go/cpeticket` with the repository, the bundle ID (`com.squareup.leakcanary.shark-explorer`), a release +asset URL and install type "optional". CPE build the AutoPkg pipeline and Munki recipes themselves and pick +up each new GitHub release daily. + +Note that Munki compares versions using `CFBundleShortVersionString`, which is `SHARK_EXPLORER_VERSION` — +another reason that field can't be pinned to something the releases don't move. + +## What the first signing runs found + +Measured against the real service on 2026-08-04, from tags whose releases were forced to draft and then +deleted. Three separate faults, and they hid each other: Apple refused the app, the service reported the +refusal as success, and a space in the app's name broke the reply that would have said so. + +### Apple refuses the app over a dylib no signer can see + +Fixed here, by `shark-explorer-app/build.gradle.kts`, which deletes them from the app image. Worth +knowing anyway, because nothing about the failure points at the cause. + +`skiko-awt-runtime-macos-arm64` ships both architectures' dylibs. Compose extracts the one it is +packaging for into the app directory, where `-Dskiko.library.path=$APPDIR` makes it the copy that loads, +and leaves the other architecture's dylib inside the jar. Nothing loads that one, and nothing signing the +bundle reaches it either: a signer walks files, and this is an entry in a zip. **Apple's notary service +opens jars.** So it refused the whole app over +`skiko-awt-runtime-macos-arm64-*.jar/libskiko-macos-x64.dylib` — *"The binary is not signed with a valid +Developer ID certificate"* — while every one of the 32 files a signer can see was signed correctly, each +with a secure timestamp, and the nested `Contents/runtime` bundle was sealed. + +Which is why no local check finds this. `codesign --verify --deep --strict` passes on the returned DMG, +`spctl --assess` accepts it, and all four entitlements are there. What it costs is the whole app: one +Apple has no notarization record of does not launch, and it does not fail either. It hangs in `dyld` with +no output and no session log, where the same bundle re-signed ad hoc starts in two seconds. So a signed +DMG that opens into a hang means notarization, not entitlements. + +### A refusal came back as success + +`notarize()` in `apple-codesign/lib/notarize.sh` (`squareup/mdx-ios-codesign-helper`) read +`xcrun notarytool submit --wait`'s exit status rather than the `status` field of the JSON it asked for, so +a refusal logged "Notarization complete" and the pipeline handed back a signed, un-notarized DMG. It also +discarded that JSON, which is where the submission id was — and the id is the only handle on Apple's +reason, since the artifact carries no trace of it. Both halves are fixed in +squareup/mdx-ios-codesign-helper#20. A run after that merge failed where the same bundle had previously +"passed", which is that fix working. + +**A signing failure's reason is legible only in Buildkite.** The lambda collapses any failed build into +`Poll request failed with status 400`, so the `notarytool log` output that PR added never reaches the +GitHub Actions log. Reading it means opening the build at `buildkite.com/runway/mdx-ios-codesign-helper`, +which is Block-internal, so expect to need someone with that access. + +### A space in the app's name broke the reply, not the signing + +The service signed `Shark Explorer.app` correctly — the Buildkite job passed and uploaded the signed zip — +and then the lambda failed working out where it had put it: `bad URI(is not URI?): "s3://…/Shark +Explorer.app.zip"`, after a mac worker had done all the work. `destination_url` in +`global/lambdas/codesign_helper.rb` parses that S3 URL with Ruby's `URI()` to insert `-signed` before the +extension, and a space is not a legal URI character. squareup/tf-mobuild-workers#1365 fixed it, so the name +has a space in it again. + +**Merging that was not the same as shipping it**, unlike the `notarize.sh` fix above, and that is the part +worth remembering: Buildkite reads its scripts out of a git checkout at build time, so the other fix went +live on merge, whereas this lambda is Ruby that terraform packages and kept serving the deployed zip +afterwards. Every check on the pull request is a *plan*. A tagged build after the merge failed with the +same `bad URI`, and what actually shipped it was the `mobuild-workers-rollout` CodePipeline: staging +applies itself, production stops at an approval in the AWS console that only the +`mobuild-workers-human-role` group can give. So for anything in that repository, **the rollout is what to +wait for, not the merge**, and it needs someone in that group. + +### Where that leaves it + +Both macOS builds come back signed as Block, notarized and stapled: `spctl` says +`source=Notarized Developer ID` and `stapler validate` says *"The validate action worked!"*, first +measured on builds 1622 and 1623. Those two lines are what a release has to show, and the workflow fails +rather than publishing anything that cannot. + +That was carried through to what someone downloading it gets: the DMG off a release, marked with the +`com.apple.quarantine` attribute a browser download applies — which is what makes Gatekeeper insist on a +ticket at all — is accepted, and the app opens a heap dump. So macOS is releasable. + +One thing to know before repeating that check, because it looks exactly like the notarization hang above: +**a quarantined app's first launch waits for the screen to be unlocked**, with no output, no log file and no +CPU. The same bundle, the same command and the same quarantine attribute took over five minutes and +produced nothing against a locked screen, and four seconds unlocked. Gatekeeper wants a human on that first +launch, so verify a release at the keyboard. diff --git a/docs/releasing.md b/docs/releasing.md index b904dc9359..59da948b1e 100644 --- a/docs/releasing.md +++ b/docs/releasing.md @@ -1,5 +1,11 @@ # Releasing LeakCanary +!!! note + This is the process for the LeakCanary and Shark **libraries**, on `v*` tags. **Shark Explorer, the + desktop app, is released separately** on `shark-explorer-*` tags and is not part of anything below — + see [Releasing Shark Explorer](releasing-shark-explorer.md). The `shark-cli` zip attached in this + process is the command line tool, not the app. + ## Prerequisites Publishing to Maven Central is fully automated: the diff --git a/docs/shark-explorer-changelog.md b/docs/shark-explorer-changelog.md new file mode 100644 index 0000000000..fe2deaccde --- /dev/null +++ b/docs/shark-explorer-changelog.md @@ -0,0 +1,21 @@ +# Shark Explorer Change Log + +Shark Explorer is a desktop app released on its own schedule, so it has its own change log. The +[LeakCanary change log](changelog.md) covers the libraries and never mentions this app. See +[Releasing Shark Explorer](releasing-shark-explorer.md) for how a version gets cut. + +Each entry starts with a marker for the kind of change it is, the same markers the LeakCanary change log +uses, without the one for a newly recognized library leak: + +| | Means | +| --- | --- | +| ⚠️ | **Breaking change**: something you relied on works differently or is gone. | +| 🔀 | **Behavior change**: the app now does something different. | +| 💥 | **Crash fix**: something used to crash, and no longer does. | +| 🐛 | **Bug fix**: the app did the wrong thing, without crashing. | +| ✨ | **New**: a capability that didn't exist before. | +| 🔨 | **Improvement**: something that already worked, now works better. | + +## Unreleased + +* ✨ Initial release. diff --git a/gradle.properties b/gradle.properties index 5d0dccee35..9effb217e1 100644 --- a/gradle.properties +++ b/gradle.properties @@ -1,6 +1,17 @@ GROUP=com.squareup.leakcanary VERSION_NAME=3.0-alpha-10-SNAPSHOT +# Shark Explorer is a desktop app rather than a library, so it ships on its own version line, released +# independently of VERSION_NAME. It also has to: the installer formats validate this and reject anything +# but three integers, so `3.0-alpha-10` is not a version a DMG can be built for. +# +# What those formats accept, measured by building each: MAJOR between 1 and 255, MINOR up to 255, PATCH up +# to 65535. macOS refuses a MAJOR of 0 ("The first number in an app-version cannot be zero or negative") +# and MSI refuses anything over 255. So no number here can mean "before 1.0" — not 0.x, and not a +# calendar version either. **The alpha lives in the release, not in the version**: the GitHub release is +# marked as a prerelease and says so. See docs/releasing-shark-explorer.md. +SHARK_EXPLORER_VERSION=1.0.0 + POM_DESCRIPTION=LeakCanary POM_INCEPTION_YEAR=2015 POM_URL=https://github.com/square/leakcanary/ diff --git a/mkdocs.yml b/mkdocs.yml index 828208f75e..13a08a92d0 100644 --- a/mkdocs.yml +++ b/mkdocs.yml @@ -89,9 +89,12 @@ nav: - 'Code of Conduct': code_of_conduct.md - 'Dev Environment': dev-env.md - 'Releasing': releasing.md + - 'Releasing Shark Explorer': releasing-shark-explorer.md - 'How to help': how_to_help.md - 'Shark': - 'Overview': shark.md - 'Shark API': api/shark/index.md + - 'Shark Explorer': + - 'Change Log': shark-explorer-changelog.md - 'LeakCanary API': api/index.md - 'Change Log': changelog.md diff --git a/shark/shark-explorer/AGENTS.md b/shark/shark-explorer/AGENTS.md index 707e2b10be..4516838104 100644 --- a/shark/shark-explorer/AGENTS.md +++ b/shark/shark-explorer/AGENTS.md @@ -89,6 +89,54 @@ what was being read and how long it took. What that makes readable: Which is also the rule for new code here: **anything the UI swallows or falls back from gets a `SharkLog.d` line saying so.** The file is only worth reading if it's complete. +## This app has its own version line, and no number of it can mean "alpha" + +`SHARK_EXPLORER_VERSION` in `gradle.properties`, not the repo wide `VERSION_NAME`, and the two are released +independently — `shark-explorer-*` tags against `v*` tags. See `docs/releasing-shark-explorer.md`. + +It isn't only a preference. Every installer format validates the version, and what they leave between them, +each measured by building it, is **`MAJOR.MINOR.PATCH` with MAJOR from 1 to 255**, MINOR up to 255, PATCH up +to 65535: + +- `3.0-alpha-10` — `Illegal version for 'Dmg'`, and for `Msi`. No qualifiers, in any format. +- `0.1.0` — fails `createDistributable`, and **not through the version validation the other two trip**: + the Compose plugin accepts it and jpackage then reports `Bundler Mac Application Image skipped because + of a configuration problem: The first number in an app-version cannot be zero or negative`. So the + message says "skipped", is only in `build/compose/logs/createDistributable/jpackage-*-err.txt`, and the + Gradle failure above it names no version at all. +- `2026.8.0` and `256.0.0` — `Illegal version for 'Msi'`, whose fields cap at 255.255.65535. + +So `0.x` is not available and neither is a calendar version, and "this is an alpha" is said by the release +being a prerelease titled that way. Don't try to encode it in the number, and don't split it across +`macOS.packageVersion` and `macOS.packageBuildVersion` either: the user visible one is +`CFBundleShortVersionString`, which is also the field Munki compares for Managed Software Center updates, +so pinning it would freeze updates for everyone who installed from there. + +**The version reaches the app as a generated resource**, `shark-explorer-version.properties`, written by +`writeVersionResource` and read by `SharkExplorerVersion`. Not the jar manifest: `run` and the tests put +class directories on the classpath rather than the jar, so `Package.getImplementationVersion()` is null for +every way this app is launched while being worked on, and the update check would only be exercisable from a +packaged build. `SharkExplorerVersionTest` exists because a broken wiring here fails silently — the version +becomes `unknown`, the check declines to run, and no window ever mentions an update. + +## The update check reports and nothing else + +`UpdateCheck` fetches one file and, if it names a later version, `UpdateNotice` puts a bar in every window +of the run. Nothing downloads or installs. Three things about it that reading the code won't tell you: + +- **`releases/latest` is the wrong release.** This repository publishes LeakCanary libraries on `v*` tags + and the explorer on `shark-explorer-*` tags, and GitHub has one "latest" pointer per repository, so that + endpoint answers with whichever line released last. Verified: it returns `v3.0-alpha-9`. +- **The unauthenticated GitHub API allows 60 requests an hour per IP**, and Block's shared egress can + exhaust that on other people's runs. A release asset download is unmetered, which is why the manifest is + one. +- **The manifest is written by `promote-shark-explorer.yml` and nothing else**, so publishing a release and + offering it to everyone are two separate acts. `UpdateCheckTest` pins the manifest format from this side, + because the workflow writes it in bash and nothing else keeps the two agreeing. + +`UpdateNotice` is one per run rather than per window, so dismissing the bar in one window clears it in all +of them. + ## Gradle facts that aren't visible from these build scripts - **`shark-explorer-app` is excluded by name** from the repo-wide Java 8 target in the root @@ -155,9 +203,9 @@ variable AWT reads at that same moment: a run given `-Xdock:name=X` and a run th time. So the run task passes no name and an IDE run configuration needs none either, and `java.awt.Taskbar` is no help — its API is icon, badge, menu and progress, and no name. -**A packaged app ignores the property and keeps its bundle's name.** `Shark Explorer.app` launched with -`--title="Packaged with a title"` logs that title and is still called `Shark Explorer` by macOS, because -jpackage gives it a real bundle. A run from Gradle has no bundle of its own — it is `/…/bin/java`, +**A packaged app ignores the property and keeps its bundle's name.** The `.app` launched with +`--title="Packaged with a title"` logs that title and is still called whatever `packageName` made the +bundle, because jpackage gives it a real bundle. A run from Gradle has no bundle of its own — it is `/…/bin/java`, bundle id `net.java.openjdk.java` — which is why it is called after whatever launched it until something names it. diff --git a/shark/shark-explorer/shark-explorer-app/build.gradle.kts b/shark/shark-explorer/shark-explorer-app/build.gradle.kts index fda1c979d0..26b434156a 100644 --- a/shark/shark-explorer/shark-explorer-app/build.gradle.kts +++ b/shark/shark-explorer/shark-explorer-app/build.gradle.kts @@ -1,3 +1,8 @@ +import java.nio.file.Files +import java.nio.file.StandardCopyOption.REPLACE_EXISTING +import java.util.zip.ZipEntry +import java.util.zip.ZipFile +import java.util.zip.ZipOutputStream import javax.inject.Inject import org.gradle.api.tasks.options.Option import org.gradle.process.ExecOperations @@ -21,6 +26,18 @@ kotlin { compilerOptions.jvmTarget = JVM_17 } +/** + * The app's own version, from `SHARK_EXPLORER_VERSION` rather than the repo wide `VERSION_NAME`: the + * explorer is released on its own tags, and jpackage would reject `VERSION_NAME` anyway. See + * `gradle.properties`. + */ +val explorerVersion = property("SHARK_EXPLORER_VERSION").toString() + +// Overrides the repo wide version the root build script sets from VERSION_NAME, so that the jar inside a +// packaged app is named after the app's version rather than after LeakCanary's. Same reasoning the root +// script gives for shark-cli, whose zip is named after the version too. +version = explorerVersion + dependencies { implementation(projects.shark.sharkExplorer.sharkExplorerCore) // Reads the bitmaps of a live process off the Android versions whose heap dumps can't carry them. @@ -44,6 +61,23 @@ tasks.withType().matching { it.name == "run" }.configureEach { workingDir = rootProject.projectDir } +/** + * Writes the version onto the classpath, which is how [shark.explorer.app.SharkExplorerVersion] reads it. + * + * A generated resource rather than a jar manifest attribute, because `run` and the tests put class + * directories on the classpath rather than the jar, so `Package.getImplementationVersion()` is null for + * every way this app is launched while being worked on — and the update check would then only be + * exercisable from a packaged build. + */ +val writeVersionResource by tasks.registering(WriteProperties::class) { + destinationFile = layout.buildDirectory.file("generated/version/shark-explorer-version.properties") + property("version", explorerVersion) +} + +sourceSets.main { + resources.srcDir(writeVersionResource.map { it.destinationFile.get().asFile.parentFile }) +} + /** Shared by the Compose plugin's `run` and by `runNamed`, which launches the same classes itself. */ val explorerMainClass = "shark.explorer.app.MainKt" @@ -72,8 +106,15 @@ compose.desktop { nativeDistributions { targetFormats(TargetFormat.Dmg, TargetFormat.Msi, TargetFormat.Deb) + // The app's visible name, and also the name of the `.app`, and therefore the name of the zip + // Block's signing service is handed. A space in it used to break the reply that service sends + // back, which is why this was one word until squareup/tf-mobuild-workers#1365 shipped. See + // docs/releasing-shark-explorer.md. packageName = "Shark Explorer" - packageVersion = "1.0.0" + // Each format validates this against rules of its own, and `gradle.properties` records which ones + // and what they leave possible. `3.0-alpha-10` satisfies none of them, which is the whole reason + // this app has a version line of its own. + packageVersion = explorerVersion // Each platform takes a different container, all three rendered from the one SVG by // icons/render-icons.sh. @@ -83,6 +124,10 @@ compose.desktop { // it the process shows the default Java icon. macOS { iconFile.set(macOsIconFile) + // Set here rather than left to default, which is the main class's package. Notarization history + // and the Managed Software Center entry are both keyed on this, so it has to be a name Square + // owns, and changing it after the first release is a migration for everyone who installed one. + bundleID = "com.squareup.leakcanary.shark-explorer" } windows { iconFile.set(project.file("icons/shark-explorer-icon.ico")) @@ -92,9 +137,85 @@ compose.desktop { iconFile.set(project.file("src/main/resources/shark-explorer-icon.png")) } - // What `shark-explorer-jdwp` attaches to a live app with. jlink leaves out every module it - // doesn't detect a use of, and it detects no use of one reached through `Bootstrap`. - modules("jdk.jdi") + // The JDK modules jlink puts in the packaged runtime, which are only the ones listed here: the + // plugin detects nothing by itself. `suggestRuntimeModules` is where this list came from and the + // task to re-run when the dependencies change. + // + // A module missing here is a `NoClassDefFoundError` that only a packaged build hits, because `run` + // has the whole JDK on hand — which is how `java.net.http` went missing for as long as it did. The + // update check is the only thing that fetches anything, so a packaged app logged "Could not fetch + // …/latest.properties" once at startup and then never mentioned a new version again. `jdk.jdi` is + // what `shark-explorer-jdwp` attaches to a live app with. + modules("java.instrument", "java.net.http", "jdk.jdi", "jdk.unsupported") + } + } +} + +/* + * Deletes the dylibs left inside the packaged app's own jars, which is what makes a macOS build + * notarizable. + * + * `skiko-awt-runtime-macos-arm64` ships both architectures' dylibs, 21 and 22 MB. Compose extracts the one + * it is packaging for into the app directory — where the launcher's `-Dskiko.library.path=$APPDIR` makes + * it the copy that loads — and leaves the other architecture's dylib inside the jar. Nothing loads that + * one. Nothing signing the bundle reaches it either: a signer walks files, and this is an entry in a zip. + * + * Apple's notary service does open jars, so it is the one Mach-O in the bundle that arrives unsigned, and + * one is enough. It refused this app over + * `skiko-awt-runtime-macos-arm64-*.jar/libskiko-macos-x64.dylib` — "The binary is not signed with a valid + * Developer ID certificate" — while every file a signer can see was signed correctly, which is why no + * local check found it. See docs/releasing-shark-explorer.md. + * + * Done to the app image and not to the jar this module resolves, because `run` and the tests load skiko + * out of that jar, and the image is the first point where only one architecture is still in play. Every + * package format is rendered from this image, so stripping it here covers the DMG. + */ +// Matched by name rather than with tasks.named(), because the Compose plugin registers this one late too. +tasks.matching { it.name == "createDistributable" }.configureEach { + // Read at configuration time: a task action reaching back into the project is what the configuration + // cache forbids, and this needs nothing else from it. + val appImage = layout.buildDirectory.dir("compose/binaries/main/app").get().asFile + doLast { + appImage.walkTopDown().filter { it.isFile && it.extension == "jar" }.forEach { jar -> + // The `.sha256` beside each one goes too: skiko checks the hash of the copy it loads, which is the + // extracted one, and a hash of a file that is gone is not worth carrying. + val bundledDylibs = ZipFile(jar).use { zip -> + zip.entries().asSequence() + .map { it.name } + .filter { it.endsWith(".dylib") || it.endsWith(".dylib.sha256") } + .toSet() + } + if (bundledDylibs.isEmpty()) return@forEach + + // The extracted copy, which has to be there for deleting the rest to be safe. `$APPDIR` is this + // directory, so beside the jar is the only place it counts as extracted to. + val extracted = jar.parentFile.walk().maxDepth(1).filter { it.extension == "dylib" }.toList() + if (extracted.isEmpty()) { + throw GradleException( + "${jar.name} holds ${bundledDylibs.joinToString()} and no dylib sits beside it, so these " + + "are the only copies and deleting them would leave nothing to load. Compose extracting " + + "the packaged architecture's dylib into the app directory is what makes this safe, and it " + + "has stopped doing that. Check what createDistributable produces before touching this." + ) + } + + val stripped = File(jar.parentFile, "${jar.name}.stripped") + ZipFile(jar).use { zip -> + ZipOutputStream(stripped.outputStream().buffered()).use { out -> + // In the order they were in, so the manifest stays the first entry. + zip.entries().asSequence().filter { it.name !in bundledDylibs }.forEach { entry -> + out.putNextEntry(ZipEntry(entry.name)) + zip.getInputStream(entry).use { it.copyTo(out) } + out.closeEntry() + } + } + } + val freed = jar.length() - stripped.length() + Files.move(stripped.toPath(), jar.toPath(), REPLACE_EXISTING) + logger.lifecycle( + "Stripped ${bundledDylibs.joinToString()} out of ${jar.name}, ${freed / 1024 / 1024} MB, " + + "leaving ${extracted.joinToString { it.name }} to load." + ) } } } diff --git a/shark/shark-explorer/shark-explorer-app/entitlements.plist b/shark/shark-explorer/shark-explorer-app/entitlements.plist new file mode 100644 index 0000000000..529cdc79f0 --- /dev/null +++ b/shark/shark-explorer/shark-explorer-app/entitlements.plist @@ -0,0 +1,27 @@ + + + + + + com.apple.security.cs.allow-jit + + com.apple.security.cs.allow-unsigned-executable-memory + + com.apple.security.cs.allow-dyld-environment-variables + + com.apple.security.cs.disable-library-validation + + + diff --git a/shark/shark-explorer/shark-explorer-app/src/main/java/shark/explorer/app/ExplorerLogging.kt b/shark/shark-explorer/shark-explorer-app/src/main/java/shark/explorer/app/ExplorerLogging.kt index e8aabe0033..b62fd41ebd 100644 --- a/shark/shark-explorer/shark-explorer-app/src/main/java/shark/explorer/app/ExplorerLogging.kt +++ b/shark/shark-explorer/shark-explorer-app/src/main/java/shark/explorer/app/ExplorerLogging.kt @@ -44,7 +44,11 @@ internal fun installLogging(): Closeable { private fun logEnvironment(sessionLog: SessionLog?) { val runtime = Runtime.getRuntime() SharkLog.d { - "Shark Explorer starting" + if (sessionLog == null) "" else ", logging to ${sessionLog.file}" + // Which version, because a report is about a build rather than about the app in general, and because + // this is the only line that says so: the update check names it only when it found a manifest to + // compare against, so a run that couldn't reach GitHub would otherwise name no version at all. + "Shark Explorer ${SharkExplorerVersion.current} starting" + + if (sessionLog == null) "" else ", logging to ${sessionLog.file}" } SharkLog.d { "Java ${System.getProperty("java.version")} (${System.getProperty("java.vm.name")}) on " + diff --git a/shark/shark-explorer/shark-explorer-app/src/main/java/shark/explorer/app/Main.kt b/shark/shark-explorer/shark-explorer-app/src/main/java/shark/explorer/app/Main.kt index 0c887f812e..0a444d09df 100644 --- a/shark/shark-explorer/shark-explorer-app/src/main/java/shark/explorer/app/Main.kt +++ b/shark/shark-explorer/shark-explorer-app/src/main/java/shark/explorer/app/Main.kt @@ -12,6 +12,7 @@ import androidx.compose.material3.CircularProgressIndicator import androidx.compose.material3.MaterialTheme import androidx.compose.material3.Surface import androidx.compose.material3.Text +import androidx.compose.material3.TextButton import androidx.compose.runtime.Composable import androidx.compose.runtime.DisposableEffect import androidx.compose.runtime.LaunchedEffect @@ -33,6 +34,8 @@ import java.awt.Frame import java.awt.GraphicsEnvironment import java.io.File import kotlinx.coroutines.CancellationException +import kotlinx.coroutines.Dispatchers +import kotlinx.coroutines.withContext import shark.SharkLog import shark.explorer.CommandLineAdb import shark.explorer.DeviceHeapDumps @@ -93,6 +96,12 @@ private fun nameThisRun(name: String) { /** One window per heap dump open, which is what [openHeapDump] keeps true as more are opened. */ private fun explorerApplication(arguments: ExplorerArguments) = application { val windows = remember { explorerWindows(arguments) } + val updateNotice = remember { UpdateNotice() } + // Once per run, not once per window, and off the UI thread: this is a network request, and a window that + // waits for GitHub to answer before it draws is a window that hangs when GitHub is unreachable. + LaunchedEffect(updateNotice) { + withContext(Dispatchers.IO) { UpdateCheck().check() }?.let { updateNotice.offer(it) } + } windows.forEach { window -> // Keyed on the window, so that closing one doesn't hand its size and position to the next one along. key(window) { @@ -120,7 +129,8 @@ private fun explorerApplication(arguments: ExplorerArguments) = application { bitmapPixels = window.bitmapPixels, onHeapDumpChosen = { file, fetchedPixels -> windows.openHeapDump(window, file, fetchedPixels) - } + }, + updateNotice = updateNotice ) } } @@ -128,8 +138,10 @@ private fun explorerApplication(arguments: ExplorerArguments) = application { } } +// Internal, like the rest of this app: nothing outside the module composes it, the module is published +// nowhere, and it takes internal types. @Composable -fun ExplorerApp( +internal fun ExplorerApp( /** The one heap dump this window shows, null until one has been chosen for it. */ heapDumpFile: File?, /** @@ -139,6 +151,11 @@ fun ExplorerApp( bitmapPixels: NativeBitmapPixels? = null, /** Where a heap dump chosen from the bar goes, which is a window: see [openHeapDump]. */ onHeapDumpChosen: (File, NativeBitmapPixels?) -> Unit, + /** + * Whether a newer release has been found, shared with every other window of this run. Empty by default so + * that a test only gets the bar when it is what the test is about. + */ + updateNotice: UpdateNotice = remember { UpdateNotice() }, /** Overridden by tests, which have no display to put a file dialog on. */ chooseHeapDumpFile: () -> File? = ::showHeapDumpFileDialog, /** Overridden by tests, which have no device to go back to and no `adb` to ask. */ @@ -200,6 +217,9 @@ fun ExplorerApp( } Column(Modifier.fillMaxSize()) { + // Above the heap dump bar, because it is about the app rather than about what is open in it, and + // because a bar that pushes the map down is one nobody can miss and nobody has to act on. + UpdateBar(updateNotice) HeapDumpBar( state = currentState, onOpenClick = { @@ -264,6 +284,35 @@ private sealed interface HeapDumpState { ) : HeapDumpState } +/** + * Says that a newer release exists, and nothing more: a link to it and a way to stop being told. + * + * Deliberately not a dialog. Someone opens this app to look at a heap dump, and a modal in front of that + * to announce a version number would be in the way of the reason they launched it. + */ +@Composable +private fun UpdateBar(updateNotice: UpdateNotice) { + val update = updateNotice.availableUpdate ?: return + Surface(color = MaterialTheme.colorScheme.secondaryContainer) { + Row( + Modifier.fillMaxWidth().padding(horizontal = 8.dp, vertical = 4.dp), + horizontalArrangement = Arrangement.spacedBy(12.dp), + verticalAlignment = Alignment.CenterVertically + ) { + Text( + updateAvailableText(update.version, SharkExplorerVersion.current), + style = MaterialTheme.typography.bodyMedium + ) + TextButton(onClick = { openInBrowser(update.releaseUrl) }) { + Text(DOWNLOAD_UPDATE) + } + TextButton(onClick = { updateNotice.dismiss() }) { + Text(DISMISS_UPDATE) + } + } + } +} + /** Which heap dump is open, and how to open another. Everything else belongs to whatever is showing it. */ @Composable private fun HeapDumpBar( @@ -365,3 +414,12 @@ private val CASCADE_STEP = 28.dp internal const val OPEN_HEAP_DUMP = "Open heap dump…" internal const val NO_HEAP_DUMP = "Open an Android heap dump to see what retains its memory." + +/** A function rather than a constant because the versions are in the middle of it, and a test wants all of it. */ +internal fun updateAvailableText( + version: String, + currentVersion: String +) = "Shark Explorer $version is available. This run is $currentVersion." + +internal const val DOWNLOAD_UPDATE = "Download" +internal const val DISMISS_UPDATE = "Not now" diff --git a/shark/shark-explorer/shark-explorer-app/src/main/java/shark/explorer/app/SharkExplorerVersion.kt b/shark/shark-explorer/shark-explorer-app/src/main/java/shark/explorer/app/SharkExplorerVersion.kt new file mode 100644 index 0000000000..5f55fe8dbf --- /dev/null +++ b/shark/shark-explorer/shark-explorer-app/src/main/java/shark/explorer/app/SharkExplorerVersion.kt @@ -0,0 +1,42 @@ +package shark.explorer.app + +import java.util.Properties +import shark.SharkLog + +/** + * Which version of the explorer this is, which is what [UpdateCheck] compares a release against. + * + * Read off the classpath from a file the build script generates out of `SHARK_EXPLORER_VERSION` — see + * `writeVersionResource` — rather than from the jar manifest, so that `./gradlew run` and the tests + * report the same version a packaged build does. + */ +internal object SharkExplorerVersion { + + /** [UNKNOWN_VERSION] when the resource is missing, which is a classpath built by hand. */ + val current: String by lazy { readVersion() } + + private fun readVersion(): String { + val stream = javaClass.getResourceAsStream("/$VERSION_RESOURCE") + if (stream == null) { + // Not fatal: the app runs, and the update check declines to compare against a version it can't + // read. Worth a line, because "no updates ever offered" otherwise looks like the check is broken. + SharkLog.d { "No $VERSION_RESOURCE on the classpath, so this run has no version" } + return UNKNOWN_VERSION + } + val version = stream.use { Properties().apply { load(it) }.getProperty("version") } + return if (version.isNullOrBlank()) { + SharkLog.d { "$VERSION_RESOURCE has no version in it" } + UNKNOWN_VERSION + } else { + version + } + } + + /** + * What a run whose version can't be read is called. Deliberately not a number: an unknown version must + * not compare as older than a release and start offering an update to a build we know nothing about. + */ + const val UNKNOWN_VERSION = "unknown" + + private const val VERSION_RESOURCE = "shark-explorer-version.properties" +} diff --git a/shark/shark-explorer/shark-explorer-app/src/main/java/shark/explorer/app/UpdateCheck.kt b/shark/shark-explorer/shark-explorer-app/src/main/java/shark/explorer/app/UpdateCheck.kt new file mode 100644 index 0000000000..c1c581ea8b --- /dev/null +++ b/shark/shark-explorer/shark-explorer-app/src/main/java/shark/explorer/app/UpdateCheck.kt @@ -0,0 +1,172 @@ +package shark.explorer.app + +import java.io.StringReader +import java.net.URI +import java.net.http.HttpClient +import java.net.http.HttpRequest +import java.net.http.HttpResponse.BodyHandlers +import java.time.Duration +import java.util.Properties +import shark.SharkLog + +/** A release newer than this run, as [UpdateCheck] found it. */ +internal data class AvailableUpdate( + val version: String, + /** The release page, which is where the download buttons and the release notes are. */ + val releaseUrl: String +) + +/** + * Asks whether a newer Shark Explorer has been released, and answers with [AvailableUpdate] or null. + * + * **It only ever reports. Nothing here downloads or installs anything** — the release page opens in a + * browser and the rest is the person's to do. A JVM app can't replace itself while it runs without a + * native helper, and an app that rewrites its own signed bundle is a much bigger thing to get right than + * a link is. + * + * Plain logic behind a `fetchManifest` function so that all of it is unit testable: the comparison, the + * parsing and every way the manifest can be unusable are what go wrong here, and none of them needs a + * network. + */ +internal class UpdateCheck( + private val currentVersion: String = SharkExplorerVersion.current, + private val fetchManifest: () -> String? = ::fetchLatestManifest +) { + + fun check(): AvailableUpdate? { + if (currentVersion == SharkExplorerVersion.UNKNOWN_VERSION) { + // A run that doesn't know its own version can't tell a newer release from the one it is, and + // offering an update to every run of a development build would be worse than offering none. + SharkLog.d { "Not checking for updates: this run has no version" } + return null + } + val manifest = try { + fetchManifest() + } catch (throwable: Throwable) { + // Offline, behind a proxy, or GitHub is down. None of that is the app's problem to solve, and none + // of it should reach a window, so it goes in the log and the check is simply over for this run. + SharkLog.d(throwable) { "Could not fetch $LATEST_MANIFEST_URL" } + return null + } + if (manifest == null) { + SharkLog.d { "No release manifest at $LATEST_MANIFEST_URL" } + return null + } + val update = parseReleaseManifest(manifest) + if (update == null) { + SharkLog.d { "Could not read a version and a release URL out of the manifest" } + return null + } + val newer = isNewerVersion(update.version, currentVersion) + SharkLog.d { + "Latest release is ${update.version}, this run is $currentVersion" + + if (newer) ", so there is an update" else ", which is up to date" + } + return if (newer) update else null + } +} + +/** + * Reads the manifest, which is a `.properties` file holding the released version and its release page. + * + * Properties rather than the JSON every comparable updater uses, because `java.util.Properties` parses + * it and JSON would mean a dependency this app has no other use for. The release workflow writes it, so + * both ends of the format are in this repo. + * + * Null for anything that isn't a manifest, which includes the HTML GitHub serves when the rolling tag + * doesn't exist yet: a `Properties` load of a web page succeeds and simply has no `version` in it. + */ +internal fun parseReleaseManifest(manifest: String): AvailableUpdate? { + val properties = try { + Properties().apply { StringReader(manifest).use { load(it) } } + } catch (throwable: Throwable) { + SharkLog.d(throwable) { "Release manifest is not a properties file" } + return null + } + val version = properties.getProperty("version")?.trim() + val releaseUrl = properties.getProperty("releaseUrl")?.trim() + return if (version.isNullOrEmpty() || releaseUrl.isNullOrEmpty()) { + null + } else { + AvailableUpdate(version, releaseUrl) + } +} + +/** + * Whether [candidate] is a later version than [current], both `MAJOR.MINOR.PATCH` — which is the only + * shape either can be, since jpackage builds no other. + * + * A missing component counts as 0, so `0.2` is later than `0.1.9` and the same as `0.2.0`. Anything + * non-numeric makes this false rather than throwing: a manifest we can't read is not grounds for telling + * someone their app is out of date. + */ +internal fun isNewerVersion( + candidate: String, + current: String +): Boolean { + val candidateParts = versionParts(candidate) ?: return false + val currentParts = versionParts(current) ?: return false + val componentCount = maxOf(candidateParts.size, currentParts.size) + for (index in 0 until componentCount) { + val candidatePart = candidateParts.getOrElse(index) { 0 } + val currentPart = currentParts.getOrElse(index) { 0 } + if (candidatePart != currentPart) { + return candidatePart > currentPart + } + } + return false +} + +/** Null for anything that isn't dot separated non-negative integers, which is every version we build. */ +private fun versionParts(version: String): List? { + val parts = version.trim().split(".") + return parts.map { part -> part.toIntOrNull()?.takeIf { it >= 0 } ?: return null } +} + +/** + * Fetches the manifest, or null for any response that isn't one. + * + * A file off the release download CDN rather than the GitHub API, for two reasons. The API's + * `releases/latest` is **the wrong release** — this repo publishes LeakCanary library releases on `v*` + * tags and the explorer on `shark-explorer-*` tags, and GitHub has one "latest" per repository, so that + * endpoint answers with whichever came last overall. And the unauthenticated API allows 60 requests an + * hour *per IP*, which is a shared corporate egress away from being exhausted by other people's runs, + * while a release asset is an ordinary unmetered CDN download. + * + * The other half of that choice is the [LATEST_MANIFEST_URL] tag being moved deliberately, so publishing + * a build and telling everyone about it stay two separate acts. See docs/releasing-shark-explorer.md. + */ +private fun fetchLatestManifest(): String? { + val client = HttpClient.newBuilder() + .connectTimeout(REQUEST_TIMEOUT) + // GitHub answers a release download with a redirect to the CDN it is actually served from. + .followRedirects(HttpClient.Redirect.NORMAL) + .build() + val request = HttpRequest.newBuilder(URI.create(LATEST_MANIFEST_URL)) + .timeout(REQUEST_TIMEOUT) + .header("Accept", "text/plain") + .GET() + .build() + // Not closed, because HttpClient only became AutoCloseable in Java 21 and this module targets 17. One + // client per check, and the check runs once a session. + val response = client.send(request, BodyHandlers.ofString()) + return if (response.statusCode() == HTTP_OK) { + response.body() + } else { + // 404 until the first release has moved the rolling tag, which is the expected answer for a while. + SharkLog.d { "$LATEST_MANIFEST_URL answered ${response.statusCode()}" } + null + } +} + +/** + * The one asset of the rolling `shark-explorer-latest` release, rewritten by whichever release is being + * offered to everyone. Not necessarily the newest one that exists — that is the point of it. + */ +private const val LATEST_MANIFEST_URL = + "https://github.com/square/leakcanary/releases/download/shark-explorer-latest/latest.properties" + +private const val HTTP_OK = 200 + +/** Short: this runs while someone waits for a window, and a check that never finishes just never fires. */ +private val REQUEST_TIMEOUT = Duration.ofSeconds(10) diff --git a/shark/shark-explorer/shark-explorer-app/src/main/java/shark/explorer/app/UpdateNotice.kt b/shark/shark-explorer/shark-explorer-app/src/main/java/shark/explorer/app/UpdateNotice.kt new file mode 100644 index 0000000000..c20c7f2827 --- /dev/null +++ b/shark/shark-explorer/shark-explorer-app/src/main/java/shark/explorer/app/UpdateNotice.kt @@ -0,0 +1,55 @@ +package shark.explorer.app + +import androidx.compose.runtime.getValue +import androidx.compose.runtime.mutableStateOf +import androidx.compose.runtime.setValue +import java.awt.Desktop +import java.net.URI +import shark.SharkLog + +/** + * Whether this run has an update to tell someone about, shared by every window of it. + * + * One per run rather than one per window, because the check is about the app and not about a heap dump: + * asking once is enough, and dismissing the bar in one window should not leave it up in the three others. + * + * Plain state rather than a composable's, for the same reason [ExplorerWindow] is: a `Window` needs a + * display, so anything only reachable from inside one is untestable here. See AGENTS.md. + */ +internal class UpdateNotice { + + /** Null until the check has answered, and again once [dismiss] has been called. */ + var availableUpdate: AvailableUpdate? by mutableStateOf(null) + private set + + fun offer(update: AvailableUpdate) { + availableUpdate = update + } + + /** For this run only. The next one asks again, which is what makes ignoring it once harmless. */ + fun dismiss() { + SharkLog.d { "Dismissed the update to ${availableUpdate?.version}" } + availableUpdate = null + } +} + +/** + * Opens the release page in whatever the machine calls a browser. + * + * [Desktop] is not available on every JVM and every desktop session, and a button that silently does + * nothing is the worst version of this, so a failure says the URL in the log — where someone can at least + * read it back out. + */ +internal fun openInBrowser(url: String) { + try { + val desktop = Desktop.getDesktop().takeIf { Desktop.isDesktopSupported() && it.isSupported(Desktop.Action.BROWSE) } + if (desktop == null) { + SharkLog.d { "No browser to open, so $url was not opened" } + return + } + SharkLog.d { "Opening $url" } + desktop.browse(URI.create(url)) + } catch (throwable: Throwable) { + SharkLog.d(throwable) { "Could not open $url" } + } +} diff --git a/shark/shark-explorer/shark-explorer-app/src/test/java/shark/explorer/app/SharkExplorerVersionTest.kt b/shark/shark-explorer/shark-explorer-app/src/test/java/shark/explorer/app/SharkExplorerVersionTest.kt new file mode 100644 index 0000000000..7502f7e52f --- /dev/null +++ b/shark/shark-explorer/shark-explorer-app/src/test/java/shark/explorer/app/SharkExplorerVersionTest.kt @@ -0,0 +1,28 @@ +package shark.explorer.app + +import org.assertj.core.api.Assertions.assertThat +import org.junit.Rule +import org.junit.Test + +/** + * That the build script's generated version resource is on the classpath and readable. + * + * Worth a test of its own because everything else about the update check keeps working when it isn't: the + * version silently becomes [SharkExplorerVersion.UNKNOWN_VERSION], the check declines to run, and no + * window ever mentions an update. A wiring mistake would therefore look exactly like "nothing to update + * to" until a release went out and nobody heard about it. + */ +class SharkExplorerVersionTest { + + @get:Rule val logged = RecordedLog() + + @Test fun `this build knows its own version`() { + assertThat(SharkExplorerVersion.current).isNotEqualTo(SharkExplorerVersion.UNKNOWN_VERSION) + } + + /** Which is what jpackage accepts and what [isNewerVersion] can compare — see `gradle.properties`. */ + @Test fun `the version is one jpackage can build and this app can compare`() { + assertThat(SharkExplorerVersion.current).matches("""\d+\.\d+\.\d+""") + assertThat(isNewerVersion(candidate = SharkExplorerVersion.current, current = "0.0.0")).isTrue() + } +} diff --git a/shark/shark-explorer/shark-explorer-app/src/test/java/shark/explorer/app/UpdateBarTest.kt b/shark/shark-explorer/shark-explorer-app/src/test/java/shark/explorer/app/UpdateBarTest.kt new file mode 100644 index 0000000000..51064af4fe --- /dev/null +++ b/shark/shark-explorer/shark-explorer-app/src/test/java/shark/explorer/app/UpdateBarTest.kt @@ -0,0 +1,62 @@ +package shark.explorer.app + +import androidx.compose.ui.test.ExperimentalTestApi +import androidx.compose.ui.test.assertIsDisplayed +import androidx.compose.ui.test.onAllNodesWithText +import androidx.compose.ui.test.onNodeWithText +import androidx.compose.ui.test.performClick +import org.assertj.core.api.Assertions.assertThat +import org.junit.Rule +import org.junit.Test + +/** + * The bar that says a newer release exists. The check itself is [UpdateCheckTest]; this is only about the + * window saying so, and about it going away. + */ +@OptIn(ExperimentalTestApi::class) +class UpdateBarTest { + + @get:Rule val logged = RecordedLog() + + @Test fun `a window with no update found says nothing about updates`() = explorerUiTest { + setContent { ExplorerApp(heapDumpFile = null, onHeapDumpChosen = { _, _ -> }) } + + assertThat(onAllNodesWithText(DOWNLOAD_UPDATE).fetchSemanticsNodes()).isEmpty() + } + + @Test fun `a window with an update found names the version`() = explorerUiTest { + setContent { + ExplorerApp( + heapDumpFile = null, + onHeapDumpChosen = { _, _ -> }, + updateNotice = UpdateNotice().apply { offer(AN_UPDATE) } + ) + } + + onNodeWithText(updateAvailableText(AN_UPDATE.version, SharkExplorerVersion.current)).assertIsDisplayed() + onNodeWithText(DOWNLOAD_UPDATE).assertIsDisplayed() + } + + /** + * One notice per run, so a window is not the thing that has been told: dismissing in one has to take the + * bar out of every window of that run, which is what sharing the [UpdateNotice] gets us. + */ + @Test fun `dismissing takes the bar out of every window sharing the notice`() = explorerUiTest { + val notice = UpdateNotice().apply { offer(AN_UPDATE) } + setContent { + ExplorerApp(heapDumpFile = null, onHeapDumpChosen = { _, _ -> }, updateNotice = notice) + } + + onNodeWithText(DISMISS_UPDATE).performClick() + + assertThat(notice.availableUpdate).isNull() + assertThat(onAllNodesWithText(DOWNLOAD_UPDATE).fetchSemanticsNodes()).isEmpty() + } + + private companion object { + val AN_UPDATE = AvailableUpdate( + version = "99.0.0", + releaseUrl = "https://github.com/square/leakcanary/releases/tag/shark-explorer-99.0.0" + ) + } +} diff --git a/shark/shark-explorer/shark-explorer-app/src/test/java/shark/explorer/app/UpdateCheckTest.kt b/shark/shark-explorer/shark-explorer-app/src/test/java/shark/explorer/app/UpdateCheckTest.kt new file mode 100644 index 0000000000..376a0acc66 --- /dev/null +++ b/shark/shark-explorer/shark-explorer-app/src/test/java/shark/explorer/app/UpdateCheckTest.kt @@ -0,0 +1,143 @@ +package shark.explorer.app + +import java.io.IOException +import org.assertj.core.api.Assertions.assertThat +import org.junit.Rule +import org.junit.Test + +/** + * What the app concludes from a release manifest. No network: the whole point of [UpdateCheck] taking a + * `fetchManifest` function is that everything that can go wrong here is reachable without one. + */ +class UpdateCheckTest { + + /** So that a log line built from the wrong state fails here rather than in a session nobody reads. */ + @get:Rule val logged = RecordedLog() + + @Test fun `a later release is an update`() { + val update = checkAgainst(released = "0.2.0", current = "0.1.0") + + assertThat(update?.version).isEqualTo("0.2.0") + assertThat(update?.releaseUrl).isEqualTo(RELEASE_URL) + } + + @Test fun `the released version being the running one is not an update`() { + assertThat(checkAgainst(released = "0.1.0", current = "0.1.0")).isNull() + } + + @Test fun `a release older than this run is not an update`() { + assertThat(checkAgainst(released = "0.1.0", current = "0.2.0")).isNull() + } + + /** + * The case that matters for a promotion gate: the rolling manifest points at an older release than the + * one someone happens to be running, and that must not read as an update. + */ + @Test fun `a run ahead of the promoted release is left alone`() { + assertThat(checkAgainst(released = "0.1.0", current = "0.1.3")).isNull() + } + + @Test fun `a run that does not know its version is never offered an update`() { + val update = UpdateCheck( + currentVersion = SharkExplorerVersion.UNKNOWN_VERSION, + fetchManifest = { manifest("99.0.0") } + ).check() + + assertThat(update).isNull() + assertThat(logged).anyMatch { it.contains("no version") } + } + + @Test fun `being offline is not a failure`() { + val update = UpdateCheck( + currentVersion = "0.1.0", + fetchManifest = { throw IOException("No route to host") } + ).check() + + assertThat(update).isNull() + assertThat(logged).anyMatch { it.contains("Could not fetch") } + } + + @Test fun `no manifest yet is not a failure`() { + val update = UpdateCheck(currentVersion = "0.1.0", fetchManifest = { null }).check() + + assertThat(update).isNull() + } + + /** What GitHub serves for a tag that doesn't exist: a page, which `Properties` reads without complaint. */ + @Test fun `a page where the manifest should be is not an update`() { + val update = UpdateCheck( + currentVersion = "0.1.0", + fetchManifest = { "Not Found" } + ).check() + + assertThat(update).isNull() + } + + @Test fun `a manifest missing the release url is not an update`() { + assertThat(parseReleaseManifest("version=0.2.0")).isNull() + } + + @Test fun `a manifest missing the version is not an update`() { + assertThat(parseReleaseManifest("releaseUrl=$RELEASE_URL")).isNull() + } + + @Test fun `a released version that is not a version is not an update`() { + val update = UpdateCheck( + currentVersion = "0.1.0", + fetchManifest = { manifest("0.2.0-alpha-1") } + ).check() + + assertThat(update).isNull() + } + + /** + * The manifest as `.github/workflows/promote-shark-explorer.yml` actually writes it, comments and all. + * + * The workflow writes this file and this code reads it, in two languages with nothing between them to + * keep them agreeing, so the format is worth pinning from this side. Change one and this fails. + */ + @Test fun `the manifest the promote workflow writes is one this reads`() { + val written = """ + # Which Shark Explorer release is currently being offered to running copies of the app. + # Written by .github/workflows/promote-shark-explorer.yml. Read by the app on startup. + version=1.2.0 + releaseUrl=https://github.com/square/leakcanary/releases/tag/shark-explorer-1.2.0 + + """.trimIndent() + + val update = parseReleaseManifest(written) + + assertThat(update).isEqualTo( + AvailableUpdate( + version = "1.2.0", + releaseUrl = "https://github.com/square/leakcanary/releases/tag/shark-explorer-1.2.0" + ) + ) + } + + @Test fun `a shorter version compares by the components it has`() { + assertThat(isNewerVersion(candidate = "0.2", current = "0.1.9")).isTrue() + assertThat(isNewerVersion(candidate = "0.2", current = "0.2.0")).isFalse() + assertThat(isNewerVersion(candidate = "1", current = "0.9.9")).isTrue() + } + + /** Component by component, not by string order, which is where `0.10.0` and `0.9.0` part company. */ + @Test fun `a two digit component is later than a one digit one`() { + assertThat(isNewerVersion(candidate = "0.10.0", current = "0.9.0")).isTrue() + assertThat(isNewerVersion(candidate = "0.9.0", current = "0.10.0")).isFalse() + } + + private fun checkAgainst( + released: String, + current: String + ): AvailableUpdate? = UpdateCheck( + currentVersion = current, + fetchManifest = { manifest(released) } + ).check() + + private fun manifest(version: String) = "version=$version\nreleaseUrl=$RELEASE_URL\n" + + private companion object { + const val RELEASE_URL = "https://github.com/square/leakcanary/releases/tag/shark-explorer-0.2.0" + } +}