Skip to content

Bundle the ggsql-jupyter kernel into the VS Code extension - #523

Open
samclark2015 wants to merge 15 commits into
mainfrom
sclark/bundle-kernel-in-vsix
Open

Bundle the ggsql-jupyter kernel into the VS Code extension#523
samclark2015 wants to merge 15 commits into
mainfrom
sclark/bundle-kernel-in-vsix

Conversation

@samclark2015

@samclark2015 samclark2015 commented Aug 18, 2026

Copy link
Copy Markdown

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)

  • Discovers a kernel at <extensionPath>/bundled/bin/ggsql-jupyter[.exe], which the per-platform VSIXes now carry.
  • Every kernel found is offered, and the user picks in the New Console Session picker. There is one order, not a set of strategies: ggsql.kernelPath, then the bundled kernel, then Jupyter kernelspecs, native install locations, and PATH. 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.
  • Fixes the phantom runtime. Discovery appended the bare binary name whether or not it was on 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.
  • A kernel is run before it is offered, and what it says is what the picker shows. probeKernel() runs ggsql-jupyter --version (a flag the kernel gains in this PR; there was no way to ask before) and names the runtime ggsql 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.
  • Kernels older than the flag are still offered. They reject --version and 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 plain ggsql (<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.
  • A probe success is cached in 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.
  • The Jupyter kernel spec is written only for the leading kernel that passed — it outlives the window, is what Quarto resolves, and has no fallback. Only the dead end — nothing runnable anywhere, whether the bundled kernel failed or the build carries none — raises one non-modal notice per extension version; a kernel merely skipped is reported in the log.
  • The bundled kernel gets a fixed runtimeId rather 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 plain ggsql, since it is the default and has nothing to distinguish itself from.

Release (.github/workflows/release-packages.yml)

  • Each platform job uploads its ggsql-jupyter as an artifact, taken after signing so the extension ships the same binary the installer does.
  • build-vsix stages that artifact and packages six VSIXes: five platform targets plus a kernel-less universal build for everything else. publish-openvsx publishes them.
  • This has to live in 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.yaml is retired.
  • create-release's globs are now scoped per artifact directory. artifacts/**/*.exe would also have matched the raw ggsql-jupyter.exe, publishing one platform's bare kernel next to the installers.

Testssrc/test/kernelDiscovery.test.ts is 44 tests, plus a 3-test Positron integration suite

  • A Positron integration suite (src/test/integration/) that launches the bundled kernel via @posit-dev/positron-test-electron: asserts one registered ggsql runtime with the ggsql-bundled id under bundled/bin/, its name matching ggsql <version> — the only check that the version really comes back from the binary — and that executeCode starts a session and returns a result. Everything else about bundling can pass while the binary fails to start; only this catches that.
  • discoverAllRuntimes had no coverage, so "no kernel means no runtime" was only asserted 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, so GgsqlRuntimeManager takes an optional kernelSpecDir.
  • The probe needed a seam too: a fixture file cannot be a runnable kernel, so the manager takes an optional probe (and a globalState stand-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 separate kernel probe suite exercises the real probeKernel against actual binaries: exit zero, exit zero without a version, a kernel that predates --version, one that rejects every argument, not executable, missing.
  • Host discovery and the symlink dedupe are covered by redirecting HOME/PATH instead 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.yaml gains a three-OS matrix — discovery branches on the OS for the binary name, the PATH lookup, 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 restores publish.yaml's with save-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)

  • Removes the !**/*.d.ts re-inclusion. Both test caches are excluded, but that later rule pulled 269 stray .d.ts files back out of them and into the VSIX, because the last matching rule wins. Pre-existing on main (134 of them from .vscode-test/); nothing reads a .d.ts at runtime. The universal build drops from 149 files to 15.
  • Excludes .positron-test/, or vsce package walks the 2.9 GB Positron download and dies in its secret scanner on a directory symlink inside the app bundle.

Verified, not assumed

  • Open VSX takes the platform from the TargetPlatform attribute vsce package --target writes into extension.vsixmanifest, defaulting to universal when absent (ExtensionProcessor.getTargetPlatform()), and ovsx discards a target option for an already-packaged vsix. So the packaged file is published as-is, with no target passed to the publish action.
  • Packaged locally with a real kernel: 46.3 MiB binary, 16.21 MiB VSIX over 16 files (the plan projected ~15.6 MiB), and 106 KB for the kernel-less universal build.
  • extension/bundled/bin/ggsql-jupyter ships without adding a .vscodeignore rule, the zip preserves -rwxr-xr-x, and the universal build has neither the attribute nor the binary. build-vsix asserts all of this per target so a targeted-but-empty VSIX cannot be published.
  • The probe's handling of an older kernel was checked against two real pre---version installs in an Extension Development Host, which are now offered as ggsql (Jupyter) and ggsql (Path) alongside the bundled ggsql 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 on refs/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-vsix

Not in this PR

  • win32-arm64 is 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 in build/lib/extensions.ts.
  • Docs: ggsql-vscode/README.md, doc/get_started/tooling.qmd, and the stale dylibbundler note in INSTALLERS.md.
  • The VSIX version comes from ggsql-vscode/package.json, not the tag. Tagging v0.5.0 without bumping it would republish 0.4.1 and, with skipDuplicate, silently skip all six publishes.

🤖 Generated with Claude Code

samclark2015 and others added 5 commits August 18, 2026 10:07
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>
samclark2015 and others added 3 commits August 24, 2026 10:09
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>
@samclark2015
samclark2015 marked this pull request as ready for review August 24, 2026 16:45
@samclark2015
samclark2015 requested a review from thomasp85 August 24, 2026 16:45

@georgestagg georgestagg left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, this looks good overall! Just a few things before we merge:

  1. 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.

  1. With this, we now inspect --version with every kernel. It would be good if the returned version was interpolated into the title c.f. R/Python kernels in this screenshot:
Image

...

  1. 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.

  1. 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's ggsql.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 fixed ggsql-bundled runtime ID deliberately survives extension updates, but its runtimePath still points inside the versioned extension directory and the manager does not implement validateMetadata. 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: KernelSelection exposes one eager candidate list plus a special fallback callback, which requires a second discovery branch and makes the reverse environment precedence 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.

Comment on lines +1 to +50
/*
* 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);
});

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@samclark2015 samclark2015 Sep 1, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

georgestagg and others added 4 commits September 1, 2026 10:01
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>
@samclark2015
samclark2015 force-pushed the sclark/bundle-kernel-in-vsix branch from 28d8fca to 8a74d46 Compare September 1, 2026 15:56
samclark2015 and others added 2 commits September 1, 2026 11:03
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants