Skip to content

Commit

Permalink
Resolve issues with SARIF and GitHub (#12)
Browse files Browse the repository at this point in the history 
Combines SARIF into a single run and improve `ignoreIncludePaths` to better filter results.
  • Loading branch information
@d-winsor
d-winsor authored Oct 29, 2021
1 parent cbc0b02 commit 47ecec9
Show file tree
Hide file tree
Showing 5 changed files with 83 additions and 73 deletions.
10 changes: 5 additions & 5 deletions .github/workflows/run-tests.yml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -42,19 +42,19 @@ jobs:
- test: Regular
outcome: success
ruleset: NativeRecommendedRules.ruleset
ignoredTargetPaths: test/sample/failure
ignoredPaths: test/sample/failure
- test: Custom ruleset
outcome: success
ruleset: test/sample/Custom.ruleset
ignoredTargetPaths: test/sample/failure
ignoredPaths: test/sample/failure
- test: Invalid ruleset
outcome: failure
ruleset: test/sample/Invalid.ruleset
ignoredTargetPaths: test/sample/failure
ignoredPaths: test/sample/failure
- test: Compilation Error
outcome: failure
ruleset: NativeRecommendedRules.ruleset
ignoredTargetPaths: ''
ignoredPaths: ''

steps:
- name: Checkout action
Expand All @@ -73,7 +73,7 @@ jobs:
resultsPath: ${{ env.result }}
ruleset: ${{ matrix.ruleset }}
ignoreSystemHeaders: true
ignoredTargetPaths: ${{ matrix.ignoredTargetPaths }}
ignoredPaths: ${{ matrix.ignoredPaths }}

- name: Validate expected action outcome
if: steps.run_action.outcome != matrix.outcome
Expand Down
2 changes: 2 additions & 0 deletions README.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,8 @@ jobs:
buildConfiguration: ${{ env.config }}
# Ruleset file that will determine what checks will be run
ruleset: NativeRecommendedRules.ruleset
# Paths to ignore analysis of CMake targets and includes
# ignoredPaths: ${{ github.workspace }}/dependencies;${{ github.workspace }}/test

# Upload SARIF file to GitHub Code Scanning Alerts
- name: Upload SARIF to GitHub
Expand Down
18 changes: 12 additions & 6 deletions action.yml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,20 +11,26 @@ inputs:
ruleset:
description: 'Ruleset file used to determine what checks are run.'
default: 'NativeRecommendedRules.ruleset'
ignoredPaths:
description: 'Identical to setting "ignoredTargetPaths" and "ignoreSystemHeaders" for the given path. This
is recommended over either option seperately.'
ignoredTargetPaths:
description: 'Any CMake targets defined inside these paths will be excluded from analysis. This is useful for excluding
code such as unit tests. List is ";" seperated and can be absolute or relative to "github.workspace"'
description: 'Any CMake targets defined inside these paths will be excluded from analysis. This is useful
for excluding tests or locally built dependencies. List is ";" seperated, requires complete
directory paths and can be absolute or relative to "github.workspace"'
required: false
ignoredIncludePaths:
description: 'Any paths in this list will be treated as SYSTEM includes and will be excluded from analysis. This requires
"ignoreSystemHeaders == true". List is ";" seperated and can be absolute or relative to "github.workspace"'
description: 'Any includes contained inside these path will be excluded from analysis. This will only filter
existing paths add not add any additional includes to the compiler. This is useful for excluding
target includes or other custom includes added to CMake. List is ";" seperated, requires complete
directory paths and can be absolute or relative to "github.workspace"'
required: false
ignoreSystemHeaders:
description: 'Uses /external arguments to ignore warnings from any headers marked as SYSTEM in CMake.'
default: true
resultsPath:
description: 'Optional path to generate the SARIF file to. If not supplied "results.sarif" will be created in the CMake
build directory. Path can be absolute or relative to "github.workspace".'
description: 'Optional path to generate the SARIF file to. If not supplied "results.sarif" will be created in
the CMake build directory. Path can be absolute or relative to "github.workspace".'
required: false
loadImplicitCompilerEnv:
description: 'Load implicit includes/libs for the given MSVC toolset using Visual Studio Command Prompt. Set to
Expand Down
63 changes: 32 additions & 31 deletions dist/index.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6412,13 +6412,14 @@ function isDirectoryEmpty(targetDir) {
}

/**
* Validate if the targetDir is either equal or a sub-directory of the parentDir
* @param {string} parentDir parent directory
* Validate if the targetDir is either equal or a sub-directory of any path in parentDirs
* @param {string[]} parentDirs parent directories
* @param {string} targetDir directory to test
* @returns {boolean} true if sub-directory
* @returns {boolean} true if a sub-directory is found
*/
function isSubdirectory(parentDir, targetDir) {
return path.normalize(targetDir).startsWith(path.normalize(parentDir));
function containsSubdirectory(parentDirs, targetDir) {
const normalizedTarget = path.normalize(targetDir);
return parentDirs.some((parentDir) => normalizedTarget.startsWith(path.normalize(parentDir)));
}

/**
Expand Down Expand Up @@ -6746,7 +6747,7 @@ function loadCompileCommands(replyIndexInfo, buildConfiguration, excludedTargetP
const codemodelInfo = configurations[0];
for (const targetInfo of codemodelInfo.targets) {
const targetDir = path.join(sourceRoot, codemodelInfo.directories[targetInfo.directoryIndex].source);
if (excludedTargetPaths.some((excludePath) => isSubdirectory(excludePath, targetDir))) {
if (containsSubdirectory(excludedTargetPaths, targetDir)) {
continue;
}

Expand Down Expand Up @@ -6827,14 +6828,12 @@ function CompilerCommandOptions() {
this.ignoreSystemHeaders = core.getInput("ignoreSystemHeaders");
// Toggle whether implicit includes/libs are loaded from Visual Studio Command Prompt
this.loadImplicitCompilerEnv = core.getInput("loadImplicitCompilerEnv");
// Ignore analysis on any CMake targets defined in these paths
this.ignoredTargetPaths = resolveInputPaths("ignoredTargetPaths");
// Additional include paths to exclude from analysis
this.ignoredIncludePaths = resolveInputPaths("ignoredIncludePaths")
.map((include) => new IncludePath(include, true));
if (this.ignoredIncludePaths && !this.ignoreSystemHeaders) {
throw new Error("Use of 'ignoredIncludePaths' requires 'ignoreSystemHeaders == true'");
}
// Ignore analysis on any CMake targets or includes.
this.ignoredPaths = resolveInputPaths("ignoredPaths");
this.ignoredTargetPaths = this.ignoredPaths || [];
this.ignoredTargetPaths = this.ignoredTargetPaths.concat(resolveInputPaths("ignoredTargetPaths"));
this.ignoredIncludePaths = this.ignoredPaths || [];
this.ignoredIncludePaths = this.ignoredIncludePaths.concat(resolveInputPaths("ignoredIncludePaths"));
// Additional arguments to add the command-line of every analysis instance
this.additionalArgs = core.getInput("additionalArgs");
// TODO: add support to build precompiled headers before running analysis.
Expand Down Expand Up @@ -6982,11 +6981,11 @@ async function createAnalysisCommands(buildRoot, options) {
const toolchain = toolchainMap[command.language];
if (toolchain) {
let args = toolrunner.argStringToArray(command.args);
const allIncludes = toolchain.includes.concat(
command.includes, options.ignoredIncludePaths);
const allIncludes = toolchain.includes.concat(command.includes);
for (const include of allIncludes) {
if (options.ignoreSystemHeaders && include.isSystem) {
// TODO: filter compilers that don't support /external.
if ((options.ignoreSystemHeaders && include.isSystem) ||
containsSubdirectory(options.ignoredIncludePaths, include.path)) {
// TODO: filter compiler versions that don't support /external.
args.push(`/external:I${include.path}`);
} else {
args.push(`/I${include.path}`);
Expand Down Expand Up @@ -7064,28 +7063,30 @@ function ResultCache() {
};
};

function filterRun(run, resultCache) {
// remove any artifacts that don't contain results to reduce log size
run.artifacts = run.artifacts.filter(artifact => artifact.roles &&
artifact.roles.some(r => r == "resultFile"));

// remove any duplicate results from other sarif runs
run.results = run.results.filter(result => resultCache.addIfUnique(result));

return run;
}

function combineSarif(resultPath, sarifFiles) {
const resultCache = new ResultCache();
const combinedSarif = {
"version": "2.1.0",
"$schema": "https://schemastore.azurewebsites.net/schemas/json/sarif-2.1.0-rtm.5.json",
"runs": []
"runs": [{
"tool": null,
"results": []
}]
};

for (const sarifFile of sarifFiles) {
const sarifLog = parseReplyFile(sarifFile);
combinedSarif.runs.push(filterRun(sarifLog.runs[0], resultCache));
for (const run of sarifLog.runs) {
if (!combinedSarif.runs[0].tool) {
combinedSarif.runs[0].tool = run.tool;
}

for (const result of run.results) {
if (resultCache.addIfUnique(result)) {
combinedSarif.runs[0].results.push(result);
}
}
}
}

try {
Expand Down
63 changes: 32 additions & 31 deletions index.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,13 +24,14 @@ function isDirectoryEmpty(targetDir) {
}

/**
* Validate if the targetDir is either equal or a sub-directory of the parentDir
* @param {string} parentDir parent directory
* Validate if the targetDir is either equal or a sub-directory of any path in parentDirs
* @param {string[]} parentDirs parent directories
* @param {string} targetDir directory to test
* @returns {boolean} true if sub-directory
* @returns {boolean} true if a sub-directory is found
*/
function isSubdirectory(parentDir, targetDir) {
return path.normalize(targetDir).startsWith(path.normalize(parentDir));
function containsSubdirectory(parentDirs, targetDir) {
const normalizedTarget = path.normalize(targetDir);
return parentDirs.some((parentDir) => normalizedTarget.startsWith(path.normalize(parentDir)));
}

/**
Expand Down Expand Up @@ -358,7 +359,7 @@ function loadCompileCommands(replyIndexInfo, buildConfiguration, excludedTargetP
const codemodelInfo = configurations[0];
for (const targetInfo of codemodelInfo.targets) {
const targetDir = path.join(sourceRoot, codemodelInfo.directories[targetInfo.directoryIndex].source);
if (excludedTargetPaths.some((excludePath) => isSubdirectory(excludePath, targetDir))) {
if (containsSubdirectory(excludedTargetPaths, targetDir)) {
continue;
}

Expand Down Expand Up @@ -439,14 +440,12 @@ function CompilerCommandOptions() {
this.ignoreSystemHeaders = core.getInput("ignoreSystemHeaders");
// Toggle whether implicit includes/libs are loaded from Visual Studio Command Prompt
this.loadImplicitCompilerEnv = core.getInput("loadImplicitCompilerEnv");
// Ignore analysis on any CMake targets defined in these paths
this.ignoredTargetPaths = resolveInputPaths("ignoredTargetPaths");
// Additional include paths to exclude from analysis
this.ignoredIncludePaths = resolveInputPaths("ignoredIncludePaths")
.map((include) => new IncludePath(include, true));
if (this.ignoredIncludePaths && !this.ignoreSystemHeaders) {
throw new Error("Use of 'ignoredIncludePaths' requires 'ignoreSystemHeaders == true'");
}
// Ignore analysis on any CMake targets or includes.
this.ignoredPaths = resolveInputPaths("ignoredPaths");
this.ignoredTargetPaths = this.ignoredPaths || [];
this.ignoredTargetPaths = this.ignoredTargetPaths.concat(resolveInputPaths("ignoredTargetPaths"));
this.ignoredIncludePaths = this.ignoredPaths || [];
this.ignoredIncludePaths = this.ignoredIncludePaths.concat(resolveInputPaths("ignoredIncludePaths"));
// Additional arguments to add the command-line of every analysis instance
this.additionalArgs = core.getInput("additionalArgs");
// TODO: add support to build precompiled headers before running analysis.
Expand Down Expand Up @@ -594,11 +593,11 @@ async function createAnalysisCommands(buildRoot, options) {
const toolchain = toolchainMap[command.language];
if (toolchain) {
let args = toolrunner.argStringToArray(command.args);
const allIncludes = toolchain.includes.concat(
command.includes, options.ignoredIncludePaths);
const allIncludes = toolchain.includes.concat(command.includes);
for (const include of allIncludes) {
if (options.ignoreSystemHeaders && include.isSystem) {
// TODO: filter compilers that don't support /external.
if ((options.ignoreSystemHeaders && include.isSystem) ||
containsSubdirectory(options.ignoredIncludePaths, include.path)) {
// TODO: filter compiler versions that don't support /external.
args.push(`/external:I${include.path}`);
} else {
args.push(`/I${include.path}`);
Expand Down Expand Up @@ -676,28 +675,30 @@ function ResultCache() {
};
};

function filterRun(run, resultCache) {
// remove any artifacts that don't contain results to reduce log size
run.artifacts = run.artifacts.filter(artifact => artifact.roles &&
artifact.roles.some(r => r == "resultFile"));

// remove any duplicate results from other sarif runs
run.results = run.results.filter(result => resultCache.addIfUnique(result));

return run;
}

function combineSarif(resultPath, sarifFiles) {
const resultCache = new ResultCache();
const combinedSarif = {
"version": "2.1.0",
"$schema": "https://schemastore.azurewebsites.net/schemas/json/sarif-2.1.0-rtm.5.json",
"runs": []
"runs": [{
"tool": null,
"results": []
}]
};

for (const sarifFile of sarifFiles) {
const sarifLog = parseReplyFile(sarifFile);
combinedSarif.runs.push(filterRun(sarifLog.runs[0], resultCache));
for (const run of sarifLog.runs) {
if (!combinedSarif.runs[0].tool) {
combinedSarif.runs[0].tool = run.tool;
}

for (const result of run.results) {
if (resultCache.addIfUnique(result)) {
combinedSarif.runs[0].results.push(result);
}
}
}
}

try {
Expand Down

0 comments on commit 47ecec9

Please sign in to comment.