-
Notifications
You must be signed in to change notification settings - Fork 2
CDS extractor : Implement retry for CDS compilation tasks #209
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
CDS extractor : Implement retry for CDS compilation tasks #209
Conversation
This commit updates the implementation of the CDS extractor in order to attempt to retry failed compilation tasks using the full set of node dependencies specified by the `package.json` config file of the associated CDS project. The new implementation continues to use a minimal cache of `@sap/cds` and `@sap/cds-dk` dependencies as the default/initial installation behavior as a performance optimization. When this does not work, the CDS extractor should now attempt to install all node dependencies for the associated (CAP) project before re-attempting the failed compilation tasks(s) for that project. This commit also updates the `package.json` config for the CDS extractor itself in order to ensure that we build test code before running unit tests, which addresses a bug in the previous testing implementation. The test code is now automatically compiled as part of running commands like `npm run test` or `npm run test:coverage`.
There was a problem hiding this 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 implements a comprehensive retry mechanism for CDS compilation tasks that fail initially. The retry system includes dependency installation, output validation, and centralized status tracking to improve compilation success rates.
- Introduces retry orchestration with systematic validation and recovery for failed compilation tasks
- Adds full dependency installation capabilities for projects requiring broader dependency access during retries
- Implements centralized status tracking and validation to ensure accurate reporting across compilation phases
Reviewed Changes
Copilot reviewed 26 out of 28 changed files in this pull request and generated 4 comments.
Show a summary per file
File | Description |
---|---|
cds-extractor.ts | Updates to use new cacheInstallDependencies function |
packageManager/ | Refactors dependency installation with separate cache and project installation utilities |
cds/compiler/ | Adds comprehensive retry orchestration, validation, and status tracking infrastructure |
test/ | Extensive test coverage for new retry mechanisms and validation utilities |
package.json | Adds TypeScript validation to test scripts |
logging/statusReport.ts | Enhances status reporting with retry-specific metrics |
Comments suppressed due to low confidence (2)
extractors/cds/tools/src/cds/compiler/compile.ts:21
- The function name
parseCommandForSpawn
is misleading. It returnsbaseArgs
which suggests arguments that come after the executable, but the function actually splits a command string. Consider renaming toparseCommandString
and the return property toargs
for clarity.
return { executable, baseArgs };
extractors/cds/tools/src/cds/compiler/retry.ts:1
- The
substr()
method is deprecated. Usesubstring()
orslice()
instead:Math.random().toString(36).substring(2, 7)
.
/** Main retry orchestration logic for CDS compilation failures. */
Reworks the CDS extractor such that it only does project-level compilation, which allows for consistently generating a single model.cds.json file in the base/root directory of its associated project, which allows for matching file paths relative to the project base directory. Updates the `CDL.qll` library file to use location/path values that are relative to the project base directory rather than relative to the source root directory.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @data-douser! I've provided a suggestion on the .qll
which I believe should fix the query side. The other comments are suggestions, and could be completed in a separate PR.
javascript/frameworks/cap/lib/advanced_security/javascript/frameworks/cap/CDL.qll
Outdated
Show resolved
Hide resolved
Implements special handling in the CDS extractor for the use case where an `index.cds` file exists in the base directory of a given CAP project, in which case we just compile the `index.cds` file instead of all the subdirectories of the project.
For CDS compilation tasks that need to be retried because the initial attempt failed to produce a `model.cds.json` file, this commit enforces that the `npx cds compile` command will require a compatible, non-deprecated version of the `@sap/cds-dk` dependency by using the `--yes --package @sap/cds-dk@^8` arguments with the `npx cds compile` command. This addresses a persistent challenge with reliably performing CDS compilation tasks via some form of the `npx cds` command when run from the base directory of a given project. If the `npx` command does not enforce the use of a compatible `--package @sap/cds-dk@*`, then we sometimes encounter errors in environments where the project inherits/uses a non-compatible version of `@sap/cds-dk` from the `node_modules` of some parent directory of the project.
Updates the `CodeQL - Run Unit Tests (javascript` actions workflow to align with recent changes to CDS extractor behavior. Adds the `cds-compilation-for-actions.test.sh` script as well as a note in the CDS extractor's `README.md` documentation in order to encourage improved synchronization between actual CDS extractor behavior versus what the unit tests actions workflow actually compiles.
Updates the `run-codeql-unit-tests-javascript.yml` actions workflow file in order to set the `cache-dependency-path` option as an attempt to resolve a runtime error for that actions workflow.
This commit replaces many separate string literals for `model.cds.json` with an exported constant of the same string in an effort to improve the readability and maintainability of CDS extractor code.
Fixes a regression affecting CodeQL queries for CAP / CDS by ensuring the `getImplementation` predicate of the `CdlService` uses `getAbsolutePath` instead of `getRelativePath`, where the `getRelativePath` approach is problematic in (unit test) cases where the source root directory and project directory are the same.
The changes from this PR should be ready for review. This PR contains changes to the CDS extractor code and the |
WARNING
This PR contains changes to the CDS extractor code and the
CDL.qll
library which are NOT backwards compatible with any CodeQL database created from a previous extractor version.Dependency Installation Changes:
installDependencies
withcacheInstallDependencies
incds-extractor.ts
to improve dependency installation performance and caching behavior. [1] [2]Compilation Process Improvements:
Retry Mechanism Enhancements:
orchestrateRetryAttempts
andidentifyTasksRequiringRetry
, to handle failed compilation tasks more robustly. [1] [2] [3]Centralized Status Updates:
Miscellaneous Updates:
build:tests
script to thepackage.json
file, ensuring TypeScript checks are run before tests.