Address review feedback for PR #359#361
Merged
data-douser merged 1 commit intodd/cds-extractor/cds-indexer-fixesfrom Apr 18, 2026
Merged
Address review feedback for PR #359#361data-douser merged 1 commit intodd/cds-extractor/cds-indexer-fixesfrom
data-douser merged 1 commit intodd/cds-extractor/cds-indexer-fixesfrom
Conversation
data-douser
added
bug
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates the CDS extractor tooling to better support cross-platform execution (notably Windows) and incorporate review-driven hardening of dependency installation behavior.
Changes:
- Add
npmExecutable()/npxExecutable()helpers and switchnpm/npxinvocations to use them while removing unnecessaryshell: true. - Harden
npm installin full-project dependency installation by adding--ignore-scripts. - Update TypeScript config and tests to support the new environment helpers.
Show a summary per file
| File | Description |
|---|---|
| extractors/cds/tools/tsconfig.json | Adds explicit jest/node types for TypeScript compilation. |
| extractors/cds/tools/test/src/packageManager/projectInstaller.test.ts | Updates expected npm install args (adds --ignore-scripts) and adjusts exec options. |
| extractors/cds/tools/test/src/environment.test.ts | Adds unit tests for npmExecutable() and npxExecutable(). |
| extractors/cds/tools/src/packageManager/projectInstaller.ts | Uses npmExecutable() and adds --ignore-scripts to full dependency installation. |
| extractors/cds/tools/src/packageManager/cacheInstaller.ts | Uses npmExecutable() for cache installs and removes shell: true. |
| extractors/cds/tools/src/environment.ts | Introduces npmExecutable() and npxExecutable() helpers. |
| extractors/cds/tools/src/cds/indexer.ts | Uses npxExecutable() and removes shell: true when running @sap/cds-indexer. |
| extractors/cds/tools/src/cds/compiler/compile.ts | Refines shell usage to be Windows-only and only for non-direct-binary commands. |
Copilot's findings
- Files reviewed: 8/10 changed files
- Comments generated: 3
data-douser
merged commit 6921c2b
into
dd/cds-extractor/cds-indexer-fixes
4 checks passed
data-douser
added a commit
that referenced
this pull request
Apr 22, 2026
…lation (#359) * Fix CDS extractor cds-indexer deps Attempt to fix the CDS extractor's support for private "cds-indexer" package installation by ensuring that the associated project's entire node_modules directory is copied instead of just the cachable subset of the CAP projects NodeJS dependencies. * CDS extractor fix dist & test * fix: CDS extractor support for Windows OS (#360) * CDS extractor windows fixes - 1 * Fix Windows ENOENT: add shell:true to npm/npx spawn calls On Windows, npm and npx are .cmd files. execFileSync('npm', ...) and spawnSync('npx', ...) fail with ENOENT because they bypass the shell and cannot resolve .cmd extensions. Adding shell: true fixes this on Windows while being a no-op on Linux/macOS. Affected files: - cacheInstaller.ts: execFileSync('npm', ['install', ...]) - projectInstaller.ts: execFileSync('npm', ['install', ...]) - indexer.ts: spawnSync('npx', ['--yes', ...]) * Fix Windows: shell:true for npx spawn, posix paths in LGTM_INDEX_FILTERS - compile.ts: change shell:false to shell:true so npx (a .cmd on Windows) can be resolved during CDS compilation - environment.ts: use explicit forward-slash paths ('exclude:**/*.*') instead of join() which produces backslashes on Windows, causing 'Illegal use of **' errors in the JS extractor * Address review feedback for PR #359 (#361)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What This PR Contributes
Updates #359 with fixes and improvements based on PR feedback.
Future Works