Sync shared package via repository_dispatch and stop Dependabot duplicates#695
Sync shared package via repository_dispatch and stop Dependabot duplicates#695edvilme wants to merge 13 commits into
Conversation
…icates Adds .github/workflows/shared-package-release.yml to handle repository_dispatch (shared-package-release) events from vscode-common-python-lsp: it bumps the npm dep, recompiles the pip pin via uv, pushes a branch, and opens a tracking issue with a manual-PR link (org settings block auto-created PRs). Removes @vscode/common-python-lsp (npm) and vscode-common-python-lsp (pip) from Dependabot via the ignore lists so Dependabot no longer opens duplicate update PRs for the shared package. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Replace the inline Python rewrite of requirements.in with a sed one-liner, keeping uv pip compile --generate-hashes --upgrade to regenerate the hash-locked requirements.txt (matching the command documented in requirements.in). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Main theme: the dispatch workflow is functional but has a correctness gap in pip recompilation (missing --python-version) plus a few robustness gaps (re-dispatch idempotency, empty-tag validation, blanket --upgrade, silent sed no-op). Addressing the pip-version issue is required before merge. |
- Pin uv pip compile to the Python version documented in requirements.in (--python-version) so markers like exceptiongroup are not dropped - Scope the recompile with --upgrade-package so unrelated deps don't move - Assert the pip pin actually changed after sed; fail fast otherwise - Validate release_tag is present and version-shaped before any work - Add concurrency group, --force-with-lease, tracking-issue dedup, and tolerate Issues being disabled (surface compare URL via job summary) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Thanks @rchiodo — addressed the feedback in the latest commit:
|
rchiodo
left a comment
rchiodo
left a comment
There was a problem hiding this comment.
Approved via Review Center.
* Integrate shared package as a git submodule Replaces the external @vscode/common-python-lsp / vscode-common-python-lsp dependency with a git submodule at external/vscode-common-python-lsp (pinned to v0.8.0). npm consumes it via a file: reference, the Python lib is bundled from the submodule, and the release pipeline now bumps the submodule commit instead of a version pin. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Fix CI for git submodule integration - Build the shared package TypeScript dist via a postinstall hook so the bundler and type-checker can resolve the file: dependency. - Regenerate package-lock.json to record the file: link and the submodule dependency tree. - Exclude external/ from the extension tsconfig so the submodule sources are not type-checked against the extension rootDir. - Check out submodules (recursive) in PR and push CI so the submodule and its Python library are present for npm and nox. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Fix VSIX packaging and type-checking for submodule integration - Exclude external/ from the VSIX (.vscodeignore) since webpack already bundles the shared package; avoids case-insensitive path collisions from the submodule node_modules. - Add skipLibCheck to tsconfig where missing so hoisted submodule devDep type definitions do not fail the extension type-check. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Checkout submodules in Azure DevOps pipelines The 1ES/DevDiv checkout template does not initialize submodules, so add an explicit git submodule update --init --recursive step before npm ci and nox in the validation, stable, and pre-release pipelines. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Init submodules manually in isort tests job The tests job checks out into a working directory with spaces and unicode (the special-working-directory path), which breaks actions/checkout submodule initialization. Check out without submodules there and run git submodule update --init --recursive as a separate step instead. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Consolidate the sync steps into a single script, add a retry wrapper for network operations, fail when the released tag is missing instead of silently falling back to main, and surface failures as workflow annotations. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Pin an explicit --force-with-lease baseline so concurrent branch updates are not silently overwritten - Regenerate the lockfile in the sync workflow so produced branches stay npm ci-mergeable - Require a real semver release_tag in the dispatch payload - Guard the postinstall build so a clone without the submodule warns instead of failing npm install Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Overall this looks good and safe to merge. One thing worth confirming before merge: the shared Python lib is now installed from the submodule with |
rchiodo
left a comment
rchiodo
left a comment
There was a problem hiding this comment.
Approved via Review Center.
- Split sync workflow into prepare (validate/normalize tag) and sync jobs; create-once branch semantics (no force-push), normalized concurrency key, pin actions/checkout@v4 - Fix dependabot.yml comments to reference shared-package-submodule-sync - Drop dangling '# via vscode-common-python-lsp' annotations from requirements.txt - Document the shared-package runtime-dep contract in requirements.in Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
| ); | ||
| process.exit(0); | ||
| } | ||
|
|
There was a problem hiding this comment.
📍 build/postinstall.js:18
The guard handles a missing submodule (warn + exit 0) but not a submodule that is present but fails npm run build. execSync throws on any nested tsc/toolchain error → non-zero exit → npm ci/npm install fails for everyone, including unrelated dev work. Previously the published npm package shipped prebuilt JS, so installs never compiled shared code. At minimum emit a clear actionable error on build failure; consider a SKIP_SHARED_BUILD opt-out for install flows that don't need the TS build.
|
Overall this is a solid, well-guarded change. The main non-blocking concern is that sourcing the shared package from the submodule now couples both the npm postinstall build and the |
rchiodo
left a comment
rchiodo
left a comment
There was a problem hiding this comment.
Approved via Review Center.
Add a guard to the submodule-sync workflow that fails the run if the extension's minimum Python (runtime.txt) is older than the shared package's requires-python floor, preventing a shipped bundle that the shared library can't import. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Break the single monolithic shell step into focused steps (guard, create branch, move submodule, verify Python floor, detect change, commit/push, tracking issue). Replace in-shell early-exits with step outputs and if: conditions, and expose the retry helper to every step via BASH_ENV so network operations stay clean one-liners. No behavior change. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Add a Development section explaining that the shared vscode-common-python-lsp library is a git submodule and how to initialize it (clone with --recurse-submodules, or git submodule update --init --recursive for an existing clone) before running npm install. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Add a guard that fails the sync if the extension's pinned Node (.nvmrc) is older than the shared package's pinned Node (submodule .nvmrc), since the extension builds the shared package's TypeScript during install. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
| process.exit(0); | ||
| } | ||
|
|
||
| execSync(`npm --prefix ${pkgDir} run build`, { stdio: "inherit" }); |
There was a problem hiding this comment.
📍 build/postinstall.js:19
The guard only handles a missing submodule (warn + exit 0); a submodule that is present but fails npm run build throws out of execSync → non-zero exit → npm install/npm ci hard-fails for everyone. The Skeptic escalated this: the submodule's tsc is a devDependency of the file:-linked package, so a --omit=dev/production install won't provision the build toolchain and this step fails with tsc: not found. This is a regression from the old prebuilt-npm-package behavior. Wrap execSync in try/catch with an actionable error (and consider a SKIP_SHARED_BUILD opt-out) so a bad pinned commit or production install doesn't hard-block unrelated installs.
[verified]
| "--no-deps", | ||
| "--upgrade", | ||
| "./external/vscode-common-python-lsp/python", | ||
| ) |
There was a problem hiding this comment.
📍 noxfile.py:99
The Advocate flagged that _update_npm_packages rewrites every non-pinned dependency to the latest ^-version from the npm registry, and @vscode/common-python-lsp is not in the pinned set — so running nox -s update_packages would overwrite the file:external/... link back to a published ^0.x version, silently undoing this PR's core mechanism. Add @vscode/common-python-lsp to the pinned set so the submodule wiring is protected.
[verified]
| echo "### Shared package submodule update ready" | ||
| echo '' | ||
| echo "Branch \`${BRANCH}\` has been pushed, moving \`${SUBMODULE_PATH}\` to ${RELEASE_TAG}." | ||
| echo '' |
There was a problem hiding this comment.
📍 .github/workflows/shared-package-submodule-sync.yml:150
The Skeptic notes the branch is pushed and a [Shared Package] Open PR… issue is filed claiming the update is "ready," but the lockfile refresh uses --ignore-scripts, so the shared TS package is never compiled and bundled/libs is never assembled in this job. A non-building submodule commit (or one adding an undeclared Python runtime dep) is reported as ready and only surfaces as broken when a human opens the PR. Consider running npm ci + build (and nox -s install_bundled_libs) on the branch before declaring it ready, or word the issue as "needs validation."
[verified]
|
The submodule migration is well thought out, but the postinstall build path is fragile (it only guards a missing submodule, not a failing build or a dev-toolchain-less install) and a couple of automation contracts around it are unenforced (nox can silently overwrite the file: link; --no-deps requires manual requirements.txt upkeep). Worth tightening before merge. |
| with: | ||
| path: ${{ env.special-working-directory-relative }} | ||
|
|
||
| - name: Checkout submodules |
There was a problem hiding this comment.
📍 .github/workflows/pr-check.yml:60
The Python-floor and Node-floor compatibility gates live only in the dispatch workflow (shared-package-submodule-sync.yml). A maintainer who bumps external/vscode-common-python-lsp by hand in a PR bypasses both floors, and CI stays green even if the extension now claims support for an older Python than the shared lib requires. Mirror the floor checks as a shared step in pr-check.yml/push-check.yml.
[verified]
|
The submodule-based approach is well thought out, but a few robustness gaps around the postinstall build, the |
Address reviewer feedback on the sync workflow: - Retry the branch-existence ls-remote and only treat a definitive 'no such ref' (exit 2) as absent, so a transient network error can no longer bypass the guard and clobber an existing branch. - Run the no-op detection before the compatibility guards so a re-dispatch with nothing to sync exits cleanly instead of failing loudly. - Compare the Python floor against the extension's declared minimum in src/common/constants.ts (not the bundling interpreter in runtime.txt), and the Node floor against the shared package's engines.node build floor (not the exact .nvmrc dev pin). - Treat an unparseable package floor as a warning instead of hard-failing every future sync. - Wrap 'npm install --package-lock-only' in the retry helper. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
| "Run `git submodule update --init --recursive` and reinstall to build it.", | ||
| ); | ||
| process.exit(0); | ||
| } |
There was a problem hiding this comment.
📍 build/postinstall.js:11
The missing-submodule guard is largely dead code: npm resolves the file:external/vscode-common-python-lsp/typescript dependency (package.json:212) during install/link, before the root postinstall lifecycle runs. On a clone without --recurse-submodules npm aborts earlier with a cryptic ENOENT/link:true failure, so this friendly warn-and-exit(0) branch is rarely reached. Either drop/repurpose it, or bootstrap the submodule before the file: link resolves; at minimum don't advertise a graceful path npm preempts.
[verified]
| "./bundled/libs", | ||
| "--no-cache-dir", | ||
| "--no-deps", | ||
| "--upgrade", |
There was a problem hiding this comment.
📍 noxfile.py:99
Partially-addressed prior High. session.install(..., '--no-deps', ...) makes requirements.txt responsible for the submodule's Python runtime deps, and while pygls/packaging/lsprotocol are now documented and pinned in requirements.in, nothing enforces the subset invariant. A future submodule bump that adds a Python runtime dep would ship a bundled/libs missing it and fail at import (fails closed, but latent). Add a sync-time assertion comparing the submodule's pyproject runtime deps against requirements.txt, or drop --no-deps.
[verified]
| exit 1 | ||
| else | ||
| echo "Node versions compatible: extension ${EXT_NODE} >= shared package build floor ${PKG_NODE}." | ||
| fi |
There was a problem hiding this comment.
📍 .github/workflows/shared-package-submodule-sync.yml:186
The commit step runs npm install --package-lock-only --ignore-scripts (no build, no nox install_bundled_libs), then pushes the branch and files a [Shared Package] Open PR… issue claiming the update is "ready." Because scripts are ignored, the shared TS package is never compiled in this job, so a non-building submodule commit is reported as ready and only surfaces as broken when a human opens the PR. Run npm ci + build (and nox -s install_bundled_libs) on the branch before declaring it ready, or word the issue as "needs validation."
[verified]
| echo "branch=shared-package-v${VERSION}" | ||
| } >>"$GITHUB_OUTPUT" | ||
|
|
||
| sync: |
There was a problem hiding this comment.
📍 .github/workflows/shared-package-submodule-sync.yml:33
Residual input-validation bypass: grep -Eq matches per-line, so RELEASE_TAG=$'1.2.3\nmalicious' passes the semver gate and ${RELEASE_TAG#v} strips only a leading v, letting the embedded newline flow into version/branch. It fails closed and the payload comes from a trusted repository_dispatch sender, so this is low-surface, but it defeats the fail-fast intent. Reject any tag containing a newline outright (e.g. validate the raw value has no newline, or use grep -Ezqx).
[verified]
|
The submodule-based bundling approach is sound, but the postinstall build path is fragile: it must not hard-fail unrelated installs when |
Point external/vscode-common-python-lsp at the v0.8.1 release and refresh package-lock.json to keep the branch npm ci-mergeable. Python (>=3.10) and Node (engines.node >=18.0.0) compatibility floors are unchanged. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
| .vscode-test/** | ||
| out/** | ||
| node_modules/** | ||
| external/** |
There was a problem hiding this comment.
📍 .vscodeignore:5
The Skeptic notes .vscodeignore external/** + the file: link is correct only because webpack bundles the built shared JS into dist/extension.js before packaging. This silently depends on postinstall having produced that build first; if postinstall is skipped (--ignore-scripts) before npm run package, webpack resolves an unbuilt main and the VSIX can ship broken with no error. Consider a guard/verification that the shared package built before packaging.
[verified]
|
The submodule approach is sound, but two things undermine its robustness: (1) the postinstall build path can hard-fail non-dev/CI installs and needs error handling + an opt-out, and (2) the pin wiring isn't self-defending against the repo's own |
|
🔒 Automated review in progress — @rchiodo is auto-reviewing this PR. |
3 participants
Summary
Two related changes so the shared package (
@vscode/common-python-lspnpm /vscode-common-python-lsppip) is updated via arepository_dispatchevent fromvscode-common-python-lspinstead of by Dependabot.1. New
.github/workflows/shared-package-release.ymlHandles
repository_dispatch(shared-package-release): branches offmain, bumps the npm dependency, rewrites the pip pin inrequirements.inand recompilesrequirements.txtwithuv, then pushes the branch and opens a tracking issue with a manual compare/PR link (org settings prevent the workflow from opening PRs automatically).2.
.github/dependabot.ymlMoves the shared package from the group
exclude-patterns(which still produced a standalone Dependabot PR) into the npm and pipignorelists, so Dependabot no longer opens duplicate PRs for it.