Bundle the ggsql-jupyter kernel into the VS Code extension - #523
Bundle the ggsql-jupyter kernel into the VS Code extension#523samclark2015 wants to merge 15 commits into
Conversation
The per-platform VSIXes will carry ggsql-jupyter at bundled/bin/, so installing the extension is enough. Add a ggsql.kernelStrategy setting (bundled | environment | path, modelled on air.executableStrategy) that decides where manager.ts looks; a ggsql.kernelPath configured before the strategy existed still means "use that path". Also fix the phantom runtime: discovery appended the bare binary name whether or not it was on PATH, and the accessibility check accepted any non-absolute path, so a machine with no kernel got a registered runtime that failed at session start with KS-19. With no kernel and no bundle, discovery now yields nothing. The bundled kernel gets a fixed runtimeId rather than one hashed from its path, which contains the versioned extension directory: hashing it would mint a new runtime on every update and lose runtime affinity and restorable sessions. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Each platform job now uploads its ggsql-jupyter binary as an artifact, taken after signing so the extension ships the same binary the installer does. A build-vsix job stages that artifact into ggsql-vscode/bundled/bin and packages one VSIX per target, plus a kernel-less universal build for platforms without one; publish-openvsx then publishes the packaged files. The VSIX build has to live in release-packages.yml rather than its own workflow: Actions artifacts are scoped to a workflow run, and two workflows triggered by the same tag run in parallel, so a separate workflow could not reach the kernels. Building in one run also keeps the kernel and the extension on the same commit. Open VSX takes the platform from the TargetPlatform attribute that vsce package --target writes into extension.vsixmanifest, so the packaged file is published as-is with no target passed to the publish action. create-release now names an artifact directory per glob instead of matching an extension anywhere under artifacts/, which would have swept the raw ggsql-jupyter.exe onto the release page alongside the installers. win32-arm64 is not built: no runner produces that kernel yet. release-vscode.yaml loses its tag trigger, which would otherwise race the new publish, and is left as a manual path for the universal VSIX alone. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Adds the layer that was missing: a Positron integration suite that launches the bundled kernel. The unit suites cover discovery precedence and the release workflow proves the binary is inside the VSIX, but nothing until now started the binary, which is the failure the bundling work is about. It asserts one registered ggsql runtime with the ggsql-bundled id, and that executeCode starts a session and returns a result. Session creation needs positron.positron-supervisor, so the harness runs with disableExtensions: false; the suite drives mocha itself because extensionTestsPath must export run(). .vscode-test.mjs now globs one level so the Positron suite does not also run under stock VS Code, where it cannot pass. discoverAllRuntimes was untested, so the "no kernel means no runtime" requirement was only checked one level down at selectKernelCandidates, which returns candidates rather than runtimes. Testing it needed a seam: discovery writes a Jupyter kernel spec as a side effect, and with the default directory a test run would repoint the real kernelspec at a fixture. GgsqlRuntimeManager therefore takes an optional kernelSpecDir. Also covered: host discovery and the symlink dedupe, by redirecting HOME and PATH rather than depending on what the developer has installed; the strategy settings read through the real configuration service, since a stubbed inspect() cannot prove the migration; and resolveConfiguredPath. The old fallback test passed vacuously on any machine without a kernel installed, which was every machine including CI. test-extension.yaml gains a three-OS matrix, because discovery branches on the OS for the binary name, the PATH lookup, the executable-bit repair and the locations it searches, and a packaging job asserting the universal VSIX stays kernel-less on every PR rather than only at release time. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Packaging from a working tree that had run the tests swept 269 stray .d.ts files out of .vscode-test/ and .positron-test/ into the VSIX: both directories are excluded, but the later `!**/*.d.ts` re-included every .d.ts anywhere, and in .vscodeignore the last matching rule wins. Nothing reads a .d.ts at runtime — esbuild bundles the one dependency — so the negation only ever shipped junk, and it goes. The Positron download cache also needed excluding outright. Without it `vsce package` walked all 2.9 GB of it and died in the secret scanner on a directory symlink inside the app bundle. A clean checkout was unaffected, which is why CI never saw it, so the packaging check moves into the job that has just run the tests and therefore has a populated .vscode-test/. That also drops a job rather than adding one, and it now asserts the absence of both caches. Measured on darwin-arm64 with a real kernel: 46.3 MiB binary, 16.21 MiB VSIX across 16 files, against the plan's ~15.6 MiB projection. The kernel-less universal build is 106 KB. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A bundled kernel is built for a platform, not for every system it can be installed on. One linked against newer shared libraries than the host provides is exec'd successfully and then rejected by the dynamic linker, so it passes every filesystem check discovery makes and still cannot serve a session. On the Linux builds this is the common case rather than an edge one: the kernels are built on Ubuntu 24.04 and need GLIBC_2.39, while Positron supports back to Ubuntu 20.04 and RHEL 9. Run the bundled kernel before offering it, and put the host locations behind it as a fallback tier. selectKernelCandidates() now returns a KernelSelection carrying that tier as a callback, so the common case -- a bundled kernel that runs -- never pays for the PATH lookup. Only the bundled kernel is probed; a kernel the user installed is taken at its word. A success is cached against the extension version, keeping it to one spawn per update; a failure is not, so a host that gains the missing libraries starts working without waiting for an update. The Jupyter kernel spec is written only for a kernel that passed, because it outlives the window, is what Quarto resolves, and has no fallback. A fallback that succeeds stays silent: the runtime's name in the picker already discloses where it came from. Only the dead end interrupts -- nothing runnable anywhere, whether the bundled kernel failed or the build carries none -- with one non-modal notice per extension version. ggsql-jupyter gains --version, which the probe uses and which had no way to be asked before. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Uses a const array with `(typeof)[number]` to derive the type and a type guard function, so the compiler enforces that KERNEL_STRATEGIES and KernelStrategy stay in sync instead of relying on manual `as` casts.
On Windows a file that is not a valid executable fails the CreateProcess call, which Node surfaces as a synchronous throw from execFile rather than a callback error. probeKernel only handled the callback path, so the promise rejected and discovery crashed instead of treating the kernel as unrunnable. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Thanks, this looks good overall! Just a few things before we merge:
- I pushed a commit to reduce Claude's trademark over-commenting and prose.
1a. I also pushed a commit to fix a latent CI issue.
- With this, we now inspect
--versionwith every kernel. It would be good if the returned version was interpolated into the title c.f. R/Python kernels in this screenshot:
...
- Previously, if you had multiple versions of ggsql installed (e.g. System install, one on path, installed via kernelspec), they would all be shown in the New Console Session picker. With this, by default only the bundled version is shown in the picker.
I think we should find all versions available and show all of them. I am happy for the bundled "ggsql" entry to be implied to be the default by omitting "(Bundled)" or similar, but if we find others we should still offer them by default and add "(System)" etc. to disambiguate.
- Related to 3, if I set the
"ggsql.kernelStrategy": "environment", it is closer to what I'd like. As such, do we really need this setting at all? If we just search everywhere, including the user'sggsql.kernelPath, and agree to show everything to the user, then we can remove this setting, right? As above, I think I'd prefer that.
Finally, here are some LLM review items. Most weren't important but here are two that might be worth looking into:
-
important (correctness) —
ggsql-vscode/src/manager.ts:502,ggsql-vscode/src/manager.ts:524: the fixedggsql-bundledruntime ID deliberately survives extension updates, but itsruntimePathstill points inside the versioned extension directory and the manager does not implementvalidateMetadata. Stored workspace/session metadata can therefore retain a path removed by the next update, undermining the stated goal of preserving affinity and restorable sessions. Add metadata validation that regenerates bundled metadata from the current extension path, and cover an old-version path in a regression test. -
minor (simplification) —
ggsql-vscode/src/manager.ts:221,ggsql-vscode/src/manager.ts:782:KernelSelectionexposes one eager candidate list plus a special fallback callback, which requires a second discovery branch and makes the reverseenvironmentprecedence awkward. Represent discovery as ordered lazy tiers and run one loop that stops after the first tier yielding a runnable kernel; this removes the asymmetric fallback handling and gives both strategies one precedence rule.
| /* | ||
| * Downloads Positron and runs the integration suite against it. | ||
| * | ||
| * Invoked by `npm run test:integration`. The stock VS Code suites go through | ||
| * @vscode/test-cli instead; only this suite needs a real Positron, because only | ||
| * it touches the language runtime API. | ||
| */ | ||
|
|
||
| import * as fs from 'fs'; | ||
| import * as path from 'path'; | ||
| import { runTests } from '@posit-dev/positron-test-electron'; | ||
|
|
||
| async function main(): Promise<void> { | ||
| // out-test/test/runIntegration.js -> the extension root | ||
| const extensionDevelopmentPath = path.resolve(__dirname, '..', '..'); | ||
| const extensionTestsPath = path.resolve(__dirname, 'integration', 'index'); | ||
|
|
||
| const binaryName = process.platform === 'win32' ? 'ggsql-jupyter.exe' : 'ggsql-jupyter'; | ||
| const bundledKernel = path.join(extensionDevelopmentPath, 'bundled', 'bin', binaryName); | ||
| if (!fs.existsSync(bundledKernel)) { | ||
| // Failing here names the missing fixture, rather than letting the suite | ||
| // fail later on an absent runtime. | ||
| throw new Error( | ||
| `no bundled kernel at ${bundledKernel}\n` + | ||
| 'Build one first:\n' + | ||
| ' cargo build --release --bin ggsql-jupyter\n' + | ||
| ` mkdir -p ${path.dirname(bundledKernel)} && cp target/release/${binaryName} ${bundledKernel}`, | ||
| ); | ||
| } | ||
|
|
||
| const code = await runTests({ | ||
| extensionDevelopmentPath, | ||
| extensionTestsPath, | ||
| // Positron's stable channel is not published for every platform, and the | ||
| // daily build is what the extension is developed against. | ||
| channel: 'daily', | ||
| // The runtime needs positron.positron-supervisor, one of Positron's | ||
| // bundled extensions, to start a session at all. With the default | ||
| // --disable-extensions there would be no supervisor and every session | ||
| // start would fail. | ||
| disableExtensions: false, | ||
| }); | ||
|
|
||
| process.exit(code); | ||
| } | ||
|
|
||
| main().catch(err => { | ||
| console.error(err); | ||
| process.exit(1); | ||
| }); |
There was a problem hiding this comment.
Will this re-download Positron for every rest run?
I think having full integration tests is a good thing, but we should try to keep an eye on how long this actually takes in CI. I'm nervous because we have had problems with both CI cache size and very long CI run times in this repo before.
There was a problem hiding this comment.
Yeah, it'll re-download, but it only takes about 75s for that download + running the tests. I think leaving this uncached is best as it's a minimal time penalty, and the daily build is published often.
The biggest time sink here is building the kernel (16 mins from scratch, it seems). I added a cache restore to slurp up the deps from main to trim that time way down; but that's already written, so no cost there.
Its rust-cache step had no options, so unlike every other workflow in the repo it saved an entry of its own for every branch it ran on -- 456MB, scoped to that ref and readable by nothing else, against a 10GB repo limit already ~69% used. Give it a shared key and let only main save it, so PRs restore that one entry instead of adding to it. Building the kernel is what the job costs; the Positron download is under a minute and stays uncached. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Discovery registered only one tier of kernels, so a machine with ggsql already installed was shown just the bundled one, and every runtime was labelled from the extension version rather than the kernel's. Drop the ggsql.kernelStrategy setting — it was never released — and register each kernel found, in one order: ggsql.kernelPath, bundled, kernelspecs, native install, PATH. The probe now reads the version out of `--version`, so a runtime is named `ggsql 0.4.1 (System)` the way Positron's own runtimes are. Only the bundled kernel has to pass the probe. Kernels released before `--version` existed exit non-zero on it, and dropping them would take away the install the user already had; they are offered without a version instead. Successful probes are cached per path with the file's mtime and size, so an install upgraded in place is probed again. validateMetadata regenerates the metadata Positron stored for a workspace. The bundled runtime id deliberately survives an extension update, but the path stored beside it points into the directory that update removed, which is what would have broken runtime affinity and restorable sessions. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The probe read any non-zero exit from `ggsql-jupyter --version` as proof the binary would not run. A kernel released before the flag existed rejects the argument and exits non-zero, so every one of them was logged as a probe failure — several lines of clap usage text per kernel, per window — when it was in fact perfectly able to serve a session. Exit status alone cannot settle it: a bundled kernel killed by the dynamic linker for want of a shared library, which is the failure the probe exists to catch, exits non-zero just the same. So ask a question every version answers instead. When `--version` is rejected by a binary that did start, the probe now falls back to `--help`, and only a kernel that fails that too is treated as unable to run. `runKernel()` separates the two outcomes execFile conflates: a numeric `code` is an exit status, so the binary was exec'd, while the errno strings and the synchronous throw mean it never started at all. Also isolate the host environment in the two discovery tests that asserted on every runtime without it. Both broke on any machine with a ggsql install, which includes any machine the extension has been run on: discovery writes the user kernelspec, so a session in the Extension Development Host leaves a kernel behind for the suite to find. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
28d8fca to
8a74d46
Compare
The cargo cache saved about 14 minutes a run, taking the job from 18 minutes to 4, but it cost another ~456MB entry against a 10GB repo-wide limit already ~67% spent -- and over 5GB of that is build.yaml's own cargo cache. The budget is shared across every workflow, this job is not on the critical path for a PR, and a cache that competes with the ones doing more work is not worth its place. Pay the build in wall-clock instead. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
publish.yaml runs on every PR and already builds this exact binary -- same command, same 1.86 toolchain, same lockfile -- so its cache key is an exact match for what this job needs. Point at its shared-key and take the restore. save-if: false is what makes this worth doing: the job reads that entry and never writes one, so the kernel build comes back warm (~16 min cold, under 3 warm) at no cost against a 10GB repo-wide limit already ~67% spent, over 5GB of which is build.yaml's cache alone. The coupling is to publish.yaml's shared-key and toolchain; change either and this goes cold rather than red. Building the kernel once instead of twice would be better still, but the two jobs are in separate workflows and Actions artifacts are run-scoped, so nothing can pass between them. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Installing the ggsql extension is now enough to run queries in Positron — no separate native installer. Groundwork for posit-dev/positron#14954.
Today the extension registers a runtime for a kernel that may not exist, and starting a session fails with
KS-19: Kernel path not found: ggsql-jupyter.What changes
Extension (
ggsql-vscode/src/manager.ts,package.json)<extensionPath>/bundled/bin/ggsql-jupyter[.exe], which the per-platform VSIXes now carry.ggsql.kernelPath, then the bundled kernel, then Jupyter kernelspecs, native install locations, andPATH. Order decides which the picker lists first — hence the default — and which occurrence survives deduplication when two paths name one file. The bundled kernel leads unless the user named one, so a machine with nothing installed gets a working runtime and a machine with an install keeps being offered it.PATH, and the accessibility check waved through any non-absolute path. With no kernel and no bundle, discovery now yields zero runtimes instead of one that fails at session start.probeKernel()runsggsql-jupyter --version(a flag the kernel gains in this PR; there was no way to ask before) and names the runtimeggsql 0.4.1, the way Positron's own runtimes are. Filesystem checks cannot tell whether a binary starts: on Linux a bundled kernel rejected by the dynamic linker is the common case, not an edge one — the kernels are built on Ubuntu 24.04 and need GLIBC_2.39, while Positron supports back to Ubuntu 20.04 and RHEL 9.--versionand exit non-zero, which by exit status alone is the same answer the loader-killed binary gives. The probe separates them by asking a question every version answers —--help— and only a kernel that fails that counts as unable to run; one that passes is offered as plainggsql (<source>)with no version. Dropping them would take away the install the user already had. Beyond that, only the bundled kernel has to pass at all: a kernel the user installed is their own business.globalState, keyed by kernel path with the file's mtime and size in the entry, so an install upgraded in place is probed again rather than reporting the version it used to have, and a pass costs one spawn per kernel per install rather than one per window. A failure is not cached, so a host that gains the missing libraries starts working without waiting for an update.runtimeIdrather than one hashed from its path: that path contains the versioned extension directory, so hashing it would mint a new runtime on every update and lose runtime affinity and the session restore added in 56174ec.validateMetadata()is what makes that safe — it regenerates the metadata Positron stored for the workspace, so the id survives an update while the stale path in the stored copy does not, and it rejects metadata with no matching candidate, which is how Positron learns to drop a runtime whose kernel has been uninstalled. The bundled runtime also shows as plainggsql, since it is the default and has nothing to distinguish itself from.Release (
.github/workflows/release-packages.yml)ggsql-jupyteras an artifact, taken after signing so the extension ships the same binary the installer does.build-vsixstages that artifact and packages six VSIXes: five platform targets plus a kernel-less universal build for everything else.publish-openvsxpublishes them.release-packages.yml: Actions artifacts are run-scoped, and two workflows triggered by the same tag run in parallel, so a separate workflow could not reach the kernels.release-vscode.yamlis retired.create-release's globs are now scoped per artifact directory.artifacts/**/*.exewould also have matched the rawggsql-jupyter.exe, publishing one platform's bare kernel next to the installers.Tests —
src/test/kernelDiscovery.test.tsis 44 tests, plus a 3-test Positron integration suitesrc/test/integration/) that launches the bundled kernel via@posit-dev/positron-test-electron: asserts one registered ggsql runtime with theggsql-bundledid underbundled/bin/, its name matchingggsql <version>— the only check that the version really comes back from the binary — and thatexecuteCodestarts a session and returns a result. Everything else about bundling can pass while the binary fails to start; only this catches that.discoverAllRuntimeshad no coverage, so "no kernel means no runtime" was only asserted atselectKernelCandidates, which returns candidates rather than runtimes. Testing it needed a seam: discovery writes a Jupyter kernel spec as a side effect, and with the default directory a test run would repoint the real kernelspec at a fixture, soGgsqlRuntimeManagertakes an optionalkernelSpecDir.probe(and aglobalStatestand-in for the cache), and the registration tests inject the verdict — every install being offered, the once-per-version warning, the cache surviving a new window, metadata revalidation. A separatekernel probesuite exercises the realprobeKernelagainst actual binaries: exit zero, exit zero without a version, a kernel that predates--version, one that rejects every argument, not executable, missing.HOME/PATHinstead of depending on what the developer has installed. That isolation now also covers the tests that assert on every runtime without staging a host kernel: running the extension writes the user kernelspec, so a session in the Extension Development Host left a kernel behind that the suite then discovered, and two tests failed on any machine the extension had been used on.test-extension.yamlgains a three-OS matrix — discovery branches on the OS for the binary name, thePATHlookup, the executable-bit repair and the locations it searches — and a packaging check on every PR rather than only at release time, run after the tests so that.vscode-test/is populated and an ignore rule letting a cache into the package actually fails. The integration job adds no cache of its own: it restorespublish.yaml's withsave-if: false. That workflow runs on every PR and builds this same binary on the same toolchain and lockfile, so the key is an exact match and the kernel build comes back warm (~16 min cold, under 3 warm) without spending a byte against a 10 GB repo limit other workflows have already mostly used.Packaging (
ggsql-vscode/.vscodeignore)!**/*.d.tsre-inclusion. Both test caches are excluded, but that later rule pulled 269 stray.d.tsfiles back out of them and into the VSIX, because the last matching rule wins. Pre-existing onmain(134 of them from.vscode-test/); nothing reads a.d.tsat runtime. The universal build drops from 149 files to 15..positron-test/, orvsce packagewalks the 2.9 GB Positron download and dies in its secret scanner on a directory symlink inside the app bundle.Verified, not assumed
TargetPlatformattributevsce package --targetwrites intoextension.vsixmanifest, defaulting to universal when absent (ExtensionProcessor.getTargetPlatform()), andovsxdiscards atargetoption for an already-packaged vsix. So the packaged file is published as-is, with notargetpassed to the publish action.extension/bundled/bin/ggsql-jupyterships without adding a.vscodeignorerule, the zip preserves-rwxr-xr-x, and the universal build has neither the attribute nor the binary.build-vsixasserts all of this per target so a targeted-but-empty VSIX cannot be published.--versioninstalls in an Extension Development Host, which are now offered asggsql (Jupyter)andggsql (Path)alongside the bundledggsql 0.4.1.Testing
CI covers the extension code on three platforms plus the kernel launch. Nothing on a PR exercises
release-packages.yml, so it needs one manual dry run before merge — publishing is gated onrefs/tags/v*, so a branch dispatch builds all five kernels and all six VSIXes and publishes nothing:gh workflow run "Release Cargo and Installer Packages" --ref sclark/bundle-kernel-in-vsixNot in this PR
win32-arm64is not built (no runner produces that kernel). The universal VSIX is not a fallback: Positron's bootstrap appends?targetPlatform=<target>and gets HTTP 403 for a target that was never published. The Positron-side entry needs either this target or a universal fallback inbuild/lib/extensions.ts.ggsql-vscode/README.md,doc/get_started/tooling.qmd, and the staledylibbundlernote inINSTALLERS.md.ggsql-vscode/package.json, not the tag. Taggingv0.5.0without bumping it would republish0.4.1and, withskipDuplicate, silently skip all six publishes.🤖 Generated with Claude Code