C++: Handle codeql_action_cpp_build_mode_none feature flag #2568
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
calumgrant
force-pushed
the
calumgrant/bmn-ff2
branch
from
efbdf00
to
0d2a78f
Compare
src/init-action.ts
Outdated
| logger.info("Enabling C++ build-mode: none"); | ||
| core.exportVariable(bmn_var, "true"); | ||
| } else { | ||
| logger.info("Disabling C++ build-mode: none"); | ||
| core.exportVariable(bmn_var, "false"); |
There was a problem hiding this comment.
I assume the reason why you are doing this is because the extractor requires this same env var to be set?
There was a problem hiding this comment.
It's not required, just setting it for completeness.
src/init-action.ts
Outdated
| @@ -546,6 +546,20 @@ async function run() { | |||
| } | |||
| } | |||
|
|
|||
| // Set CODEQL_EXTRACTOR_CPP_BUILD_MODE_NONE | |||
| if (config.languages.includes(Language.cpp)) { | |||
| const bmn_var = "CODEQL_EXTRACTOR_CPP_BUILD_MODE_NONE"; | |||
There was a problem hiding this comment.
This is probably a linter error. Here's a more canonical name.
| const bmn_var = "CODEQL_EXTRACTOR_CPP_BUILD_MODE_NONE"; | |
| const bmnVar = "CODEQL_EXTRACTOR_CPP_BUILD_MODE_NONE"; |
There was a problem hiding this comment.
Is this simpler? features.getValue is guaranteed to return a boolean and if the env var is already set, it will use that as an override.
const bmnVar = await features.getValue(Feature.CppBuildModeNone, codeql);
core.exportVariable(bmnVar);
logger.info(`Setting C++ build-mode: none to ${bmnVar}`);There was a problem hiding this comment.
That didn't quite compile, but I managed to simplify things. The code in init-action.ts is not written in this idiomatic way, and the docs to exportVariable() don't mention the behaviour when the variable is already set. Most of the code in init-action.ts defensively checks the environment.
I would suggest refactoring this file as most people will just adapt code that's already there.
calumgrant
force-pushed
the
calumgrant/bmn-ff2
branch
from
26d552d
to
f88a648
Compare
| // Set CODEQL_EXTRACTOR_CPP_BUILD_MODE_NONE | ||
| if (config.languages.includes(Language.cpp)) { | ||
| const bmnVar = "CODEQL_EXTRACTOR_CPP_BUILD_MODE_NONE"; |
There was a problem hiding this comment.
If you think we might want users to interact with this option, we should make this an extractor option by changing the name to CODEQL_EXTRACTOR_CPP_OPTION_BUILD_MODE_NONE and including an entry in codeql-extractor.yml. That gives customers more options for configuring it. If this is only going to be used by the Action, then we can leave it as is.
There was a problem hiding this comment.
This is internal and short-lived, so I don't think we should give customers the option of configuring it.
Merge / deployment checklist
Continues #2565, tidying up the feature flag options and setting the environment variables.