Build from source some extensions that were previously bundled. - #775
Build from source some extensions that were previously bundled.#775rgrunber wants to merge 1 commit into
Conversation
|
Click here to review and test in web IDE: |
&message=Dev%20cluster%20(for%20maintainers)&logo=eclipseche&color=525C86&labelColor=FDB940)
📝 WalkthroughWalkthroughThe change adds four Git submodules for external extensions, enables submodule checkout, and introduces scripts to copy and execute their build tooling. Gulp and npm build directories now include the extensions, while install hooks prepare their scripts and package metadata. Built-in extension metadata is removed, and locked artifacts are replaced with architecture-specific ripgrep binaries. Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Checkout
participant NpmInstall
participant BuildScripts
participant ExtensionRepositories
Checkout->>ExtensionRepositories: fetch submodules
NpmInstall->>BuildScripts: run preinstall helpers
BuildScripts->>ExtensionRepositories: copy build scripts and update metadata
BuildScripts->>ExtensionRepositories: invoke extension builds
ExtensionRepositories-->>BuildScripts: generate packaged outputs
Suggested reviewers: Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
code/build/npm/dirs.ts (1)
31-46: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winEnsure the npm cache contract matches these added dirs.
extensions/devfileis a TypeScript build target but has nopackage.json/package-lock.json, so it is exposed to the cache logic without the files it expects; either add a no-op package contract or move it out of this npm dir list. Similarly,extensions/js-profile-visualizercompiles nestedpackages/vscode-js-profile-table; if that package depends on its own install artifacts, add entries/lockfile handling for the nested package too.
[mantainability_and_code_quality]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@code/build/npm/dirs.ts` around lines 31 - 46, Update the npm directory configuration containing the listed extension paths so every cached build target has the package artifacts expected by the cache logic. For extensions/devfile, either add the required no-op package contract or remove it from the npm directory list; for extensions/js-profile-visualizer, include handling for the nested packages/vscode-js-profile-table package and its lockfile when required.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/image-publish.yml:
- Around line 39-40: Update the actions/checkout configuration by adding
persist-credentials: false alongside submodules: true, ensuring credentials are
not retained in the workspace before the subsequent Docker build.
In `@code/build/copy-extension-build-scripts.js`:
- Around line 34-42: Update the missing-input checks in the copy flow, including
the checks for src, destDir, and jsDebugPkgPath, to throw an error immediately
instead of warning and continuing. Preserve the existing path-specific error
context while ensuring any absent required submodule directory, build script, or
js-debug package metadata causes the script to fail.
In `@code/build/extension-build-scripts/js-profile-visualizer.esbuild.ts`:
- Around line 14-23: Update the two tsc invocations in the build script so
catches do not unconditionally continue after compiler failures. Allow only
explicitly tolerated type-check errors, or validate that all expected out/esm
outputs were freshly generated before proceeding; otherwise propagate the
failure and prevent packaging an invalid extension.
---
Outside diff comments:
In `@code/build/npm/dirs.ts`:
- Around line 31-46: Update the npm directory configuration containing the
listed extension paths so every cached build target has the package artifacts
expected by the cache logic. For extensions/devfile, either add the required
no-op package contract or remove it from the npm directory list; for
extensions/js-profile-visualizer, include handling for the nested
packages/vscode-js-profile-table package and its lockfile when required.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 71a74af2-86ee-4e51-8b8e-a09ae7d4786e
📒 Files selected for processing (18)
.github/workflows/image-publish.yml.gitmodulesbuild/artifacts/artifacts.lock.yamlcode/build/copy-extension-build-scripts.jscode/build/extension-build-scripts/devfile.esbuild.tscode/build/extension-build-scripts/js-debug-companion.esbuild.tscode/build/extension-build-scripts/js-debug.esbuild.tscode/build/extension-build-scripts/js-profile-visualizer.esbuild.tscode/build/extension-build-scripts/js-profile-visualizer.webpack.override.jscode/build/gulpfile.extensions.tscode/build/npm/dirs.tscode/extensions/devfilecode/extensions/js-debugcode/extensions/js-debug-companioncode/extensions/js-profile-visualizercode/package.jsoncode/product.jsonpackage.json
💤 Files with no reviewable changes (2)
- code/product.json
- build/artifacts/artifacts.lock.yaml
| with: | ||
| submodules: true |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Do not persist the checkout token into the build workspace.
actions/checkout persists credentials by default. With submodules enabled, the token can remain in Git metadata available to the subsequent docker build ... . context. Set persist-credentials: false after checkout has fetched the submodules.
Proposed fix
with:
submodules: true
+ persist-credentials: false📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| with: | |
| submodules: true | |
| with: | |
| submodules: true | |
| persist-credentials: false |
🧰 Tools
🪛 zizmor (1.28.0)
[warning] 37-40: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/image-publish.yml around lines 39 - 40, Update the
actions/checkout configuration by adding persist-credentials: false alongside
submodules: true, ensuring credentials are not retained in the workspace before
the subsequent Docker build.
Source: Linters/SAST tools
| if (!fs.existsSync(src)) { | ||
| console.warn(`Warning: ${src} not found, skipping`); | ||
| continue; | ||
| } | ||
|
|
||
| if (!fs.existsSync(destDir)) { | ||
| console.warn(`Warning: ${destDir} not found, skipping`); | ||
| continue; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Fail closed when required extension inputs are missing.
The script currently exits successfully when a required submodule directory, build script, or js-debug/package.json is absent. CI can therefore continue with stale scripts or unmodified metadata and produce a non-reproducible package. Throw an error instead of warning and continuing.
Proposed fix
if (!fs.existsSync(src)) {
- console.warn(`Warning: ${src} not found, skipping`);
- continue;
+ throw new Error(`Required build script not found: ${src}`);
}
if (!fs.existsSync(destDir)) {
- console.warn(`Warning: ${destDir} not found, skipping`);
- continue;
+ throw new Error(`Required extension directory not found: ${destDir}`);
}Apply the same fail-fast behavior when jsDebugPkgPath is missing.
Also applies to: 49-60
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@code/build/copy-extension-build-scripts.js` around lines 34 - 42, Update the
missing-input checks in the copy flow, including the checks for src, destDir,
and jsDebugPkgPath, to throw an error immediately instead of warning and
continuing. Preserve the existing path-specific error context while ensuring any
absent required submodule directory, build script, or js-debug package metadata
causes the script to fail.
| try { | ||
| cp.execFileSync(tsc, ['-p', 'tsconfig.json'], { cwd: coreDir, stdio: 'inherit' }); | ||
| } catch (e) { | ||
| // Ignore tsc errors - output files are still generated | ||
| } | ||
| try { | ||
| cp.execFileSync(tsc, ['-p', 'tsconfig.browser.json'], { cwd: coreDir, stdio: 'inherit' }); | ||
| } catch (e) { | ||
| // Ignore tsc errors - output files are still generated | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Do not treat every tsc failure as a successful build.
These catches suppress configuration, parse, module-resolution, and other compiler failures—not just tolerated type errors. The subsequent steps can then consume missing or stale out/esm output and package an invalid extension. Fail on non-tolerable compiler failures, or validate all expected emitted files before continuing.
🧰 Tools
🪛 ast-grep (0.45.0)
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: require('child_process')
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@code/build/extension-build-scripts/js-profile-visualizer.esbuild.ts` around
lines 14 - 23, Update the two tsc invocations in the build script so catches do
not unconditionally continue after compiler failures. Allow only explicitly
tolerated type-check errors, or validate that all expected out/esm outputs were
freshly generated before proceeding; otherwise propagate the failure and prevent
packaging an invalid extension.
- devfile.vscode-devfile, ms-vscode.js-debug, js-debug-companion, js-profile-table - Create git submodules under extensions/ - Add extensions to code/build/npm/dirs.ts to trigger npm-install - Create .esbuild.ts to wrap compilation process of each extension Assisted-By: claude-opus-4-6 Signed-off-by: Roland Grunberg <rgrunber@redhat.com>
21dd277 to
871ed3b
Compare
rgrunber
changed the title
|
Pull Request images published ✨ Editor amd64: quay.io/che-incubator-pull-requests/che-code:pr-775-amd64 |
1 participant
Assisted-By: claude-opus-4-6
I tested this by publishing to https://quay.io/repository/rgrunber/che-code?tab=tags and then running that editor image. The extensions seem to be bundled as part of the editor.
The only thing I need to verify is that the contents of the published vsix in the marketplace match the contents (roughly) of what the build generates. I think the
.vscodeignoreincludes too many things which would result in a larger installation.Summary by CodeRabbit