C++: Handle codeql_action_cpp_build_mode_none feature flag#2568
C++: Handle codeql_action_cpp_build_mode_none feature flag#2568calumgrant merged 3 commits intomainfrom
Conversation
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
|
|
||
| // 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.
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.