Skip to content

Fix CDS extractor for cds-indexer private package dependency installation#359

Draft
data-douser wants to merge 6 commits intomainfrom
dd/cds-extractor/cds-indexer-fixes
Draft

Fix CDS extractor for cds-indexer private package dependency installation#359
data-douser wants to merge 6 commits intomainfrom
dd/cds-extractor/cds-indexer-fixes

Conversation

@data-douser
Copy link
Copy Markdown
Collaborator

What This PR Contributes

This pull request adds logic to install full project dependencies after a successful cds-indexer run, ensuring that all required CDS model imports can be resolved during compilation. The change is also thoroughly tested, including cases where dependency installation fails or is unnecessary.

Outline of Changes

Dependency installation improvements:

  • After a successful cds-indexer run, the code now calls projectInstallDependencies to install all required project dependencies, ensuring the CDS compiler can resolve all imports. If the installation fails, a warning is logged, but the process continues. [1] [2]

Testing enhancements:

  • Mocks for projectInstallDependencies and related functions were added to the test suite to simulate dependency installation outcomes. [1] [2]
  • New tests verify that:
    • Dependencies are installed only after successful indexer runs.
    • Dependency installation is skipped if the indexer fails or is not present.
    • The workflow continues even if dependency installation fails after a successful indexer run.

Future Works

N/A

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.
@data-douser data-douser self-assigned this Apr 16, 2026
@data-douser data-douser added bug Something isn't working javascript Pull requests that update javascript code labels Apr 16, 2026
data-douser and others added 4 commits April 16, 2026 09:49
* 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
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR updates the CDS extractor workflow to better handle CAP projects that depend on @sap/cds-indexer and additional (including private) project dependencies, with several Windows-specific robustness fixes.

Changes:

  • After a successful cds-indexer run, trigger a full project dependency installation to improve CDS import resolution during compilation.
  • Adjust process execution and scripts for Windows compatibility (batch escaping; shell usage for npm/npx).
  • Improve Windows path/glob handling (LGTM_INDEX_FILTERS patterns and glob options).
Show a summary per file
File Description
extractors/javascript/tools/pre-finalize.cmd Escapes parentheses in echo statements inside batch blocks.
extractors/cds/tools/index-files.cmd Escapes parentheses in error message echo for batch compatibility.
extractors/cds/tools/src/cds/indexer.ts Runs cds-indexer with updated spawn options and installs full dependencies after successful runs.
extractors/cds/tools/src/packageManager/projectInstaller.ts Updates npm install execution options used for full project installs.
extractors/cds/tools/src/packageManager/cacheInstaller.ts Updates cached dependency installation command execution options.
extractors/cds/tools/src/cds/compiler/compile.ts Changes spawn options for CDS compilation processes.
extractors/cds/tools/src/environment.ts Uses forward-slash LGTM index filter patterns to avoid Windows glob issues.
extractors/cds/tools/src/cds/parser/functions.ts Sets glob options to improve Windows path behavior.
extractors/cds/tools/cds-extractor.ts Sets glob options to improve Windows path behavior.
extractors/cds/tools/test/src/cds/indexer.test.ts Adds mocks/tests covering post-indexer dependency installation behavior.
extractors/cds/tools/test/src/packageManager/projectInstaller.test.ts Updates expectations for the changed npm install spawn options.
extractors/cds/tools/dist/cds-extractor.bundle.js Regenerated bundle reflecting the TypeScript source changes.

Copilot's findings

Comments suppressed due to low confidence (1)

extractors/cds/tools/src/cds/indexer.ts:210

  • This unconditionally triggers a full npm install for every successful cds-indexer run. That can significantly increase extractor time/network usage and largely defeats the cached-deps approach. Consider gating this (e.g., only when full deps are actually needed / not yet installed), and if installation succeeds, update the project’s retryStatus.fullDependenciesInstalled so the retry pipeline doesn’t reinstall again later.
      // resolve all CDS model imports (e.g. `@sap/cds-shim`) during
      // compilation.  The cache directory only contains `@sap/cds`,
      // `@sap/cds-dk`, and `@sap/cds-indexer`, which is not enough for
      // projects that reference additional CDS packages.
      const installResult = projectInstallDependencies(project, sourceRoot);
  • Files reviewed: 11/14 changed files
  • Comments generated: 4

Comment on lines 251 to 255
const spawnOptions: SpawnSyncOptions = {
cwd: projectBaseDir, // CRITICAL: Always use project base directory as cwd to ensure correct path generation
shell: false, // Use shell=false to ensure proper argument handling for paths with spaces
shell: true, // Required on Windows where npm/npx are .cmd files
stdio: 'pipe',
env: { ...process.env },
Copy link

Copilot AI Apr 18, 2026

Choose a reason for hiding this comment

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

shell was switched from false to true for all CDS compilation spawns. This changes argument handling semantics (especially for paths with spaces) and increases exposure to shell-quoting edge cases. Prefer keeping shell: false and using platform-specific executables (npx.cmd/cds.cmd) or only enabling shell on Windows when needed.

See below for a potential fix:

  // Check if we're using a direct binary path (contains node_modules/.bin/ or node_modules\.bin\) or npx-style command
  // Check both platform-native separator and forward slash for cross-platform compatibility
  const binPathNative = `node_modules${sep}.bin${sep}`;
  const binPathPosix = 'node_modules/.bin/';
  const isDirectBinary = cdsCommand.includes(binPathNative) || cdsCommand.includes(binPathPosix);
  const useShell = process.platform === 'win32' && !isDirectBinary;

  const spawnOptions: SpawnSyncOptions = {
    cwd: projectBaseDir, // CRITICAL: Always use project base directory as cwd to ensure correct path generation
    shell: useShell, // Only enable shell on Windows for command resolution such as npm/npx .cmd files
    stdio: 'pipe',
    env: { ...process.env },
  };

Copilot uses AI. Check for mistakes.
Comment thread extractors/cds/tools/src/cds/indexer.ts Outdated
Comment on lines +114 to +118
@@ -114,6 +115,7 @@ export function runCdsIndexer(
env,
stdio: 'pipe',
timeout: CDS_INDEXER_TIMEOUT_MS,
shell: true,
Copy link

Copilot AI Apr 18, 2026

Choose a reason for hiding this comment

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

spawnSync is invoked with shell: true, which routes execution through a command shell. That’s usually unnecessary for npx and can introduce quoting/escaping differences across platforms. Prefer keeping shell: false and selecting the correct executable on Windows (e.g., npx.cmd), or make shell conditional on process.platform === 'win32'.

This issue also appears on line 206 of the same file.

See below for a potential fix:

    const npxExecutable = process.platform === 'win32' ? 'npx.cmd' : 'npx';
    const spawnResult = spawnSync(
      npxExecutable,
      ['--yes', CDS_INDEXER_PACKAGE],
      {
        cwd: projectAbsPath,
        env,
        stdio: 'pipe',
        timeout: CDS_INDEXER_TIMEOUT_MS,
        shell: false,
      },
    );

Copilot uses AI. Check for mistakes.
Comment on lines +74 to +78
@@ -75,6 +75,7 @@ export function projectInstallDependencies(
cwd: projectPath,
stdio: 'inherit',
timeout: 120000, // 2-minute timeout
shell: true,
Copy link

Copilot AI Apr 18, 2026

Choose a reason for hiding this comment

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

Running npm install with shell: true executes through a shell and is more error-prone for argument quoting; it also increases the risk surface compared to using the platform-specific executable (e.g., npm.cmd on Windows) with shell: false. Since this installs full project dependencies, also consider passing --ignore-scripts to avoid running arbitrary lifecycle scripts from untrusted dependencies during extraction.

See below for a potential fix:

    const npmExecutable = process.platform === 'win32' ? 'npm.cmd' : 'npm';

    try {
      execFileSync(
        npmExecutable,
        ['install', '--ignore-scripts', '--quiet', '--no-audit', '--no-fund'],
        {
          cwd: projectPath,
          stdio: 'inherit',
          timeout: 120000, // 2-minute timeout
          shell: false,
        },
      );

Copilot uses AI. Check for mistakes.
Comment on lines +497 to +500
execFileSync('npm', ['install', '--quiet', '--no-audit', '--no-fund'], {
cwd: cacheDir,
stdio: 'inherit',
shell: true,
Copy link

Copilot AI Apr 18, 2026

Choose a reason for hiding this comment

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

execFileSync('npm', ...) is now run with shell: true. Prefer using npm.cmd on Windows (or otherwise making shell conditional) and keeping shell: false to avoid shell-specific quoting/escaping behavior and reduce security risk.

Suggested change
execFileSync('npm', ['install', '--quiet', '--no-audit', '--no-fund'], {
cwd: cacheDir,
stdio: 'inherit',
shell: true,
const npmExecutable = process.platform === 'win32' ? 'npm.cmd' : 'npm';
execFileSync(npmExecutable, ['install', '--quiet', '--no-audit', '--no-fund'], {
cwd: cacheDir,
stdio: 'inherit',

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working javascript Pull requests that update javascript code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants