Rust: remove shipped feature flag #2960
Conversation
src/init-action.ts
Outdated
| if ( | ||
| config.languages.includes(Language.rust) && | ||
| !(await codeql.resolveLanguages()).rust | ||
| !(await codeql.resolveLanguages()).rust // means codeql already supports rust without any intervention |
There was a problem hiding this comment.
Minor: I'd prefer this comment to be part of the comment above the if rather than at the end of this line, if you think that it is needed at all.
src/init-action.ts
Outdated
| @@ -588,32 +587,25 @@ async function run() { | |||
| core.exportVariable(bmnVar, value); | |||
| } | |||
|
|
|||
| // For rust: set CODEQL_ENABLE_EXPERIMENTAL_FEATURES, unless codeql already supports rust without it | |||
| // For rust, if running a version prior to public preview (2.22.1) set CODEQL_ENABLE_EXPERIMENTAL_FEATURES | |||
There was a problem hiding this comment.
I am not sure that this comment accurately describes what is currently implemented. As far as I can tell you check whether the actual CLI version is less than minVer and if so throw a ConfigurationError. That means we never get to the CODEQL_ENABLE_EXPERIMENTAL_FEATURES check.
If that is the case, and that behaviour is unintentional, then you should obviously fix it, but I'd also like to perhaps see this refactored with a test to make sure it does the right thing.
There was a problem hiding this comment.
there are 2 versions involved:
- prior to 2.19.3, there is no rust support whatsoever, which means that if we are requesting rust we indeed need to throw a configuration error
- between 2.19.3 and 2.22.1, rust support is experimental, which means the variable needs to be set explicitly. What I now see, is that if
!(await codeql.resolveLanguages()).rust, that actually entails already thatCODEQL_ENABLE_EXPERIMENTAL_FEATURESis not set, otherwisecodeql resolve languageswould provide an entry forrust, which makes theprocess.env[envVar] !== "true"check redundant. - after 2.22.1,
(await codeql.resolveLanguages()).rustwill always be set
If we forego the "Experimental Rust analysis enabled" log line entirely (there's probably no burning need for it), we can simplify by getting rid of the second if. And the comment needs a rewrite to be clearer
There was a problem hiding this comment.
I'm considering to just not mention the 2.19.3 range, and go for a simple Rust analysis is supported from version 2.22.1. No need for the general public to know about the experimental phase I guess
| // * either we run `codeql` prior to 2.19.3, which means no rust support is possible | ||
| // * or we run a version between 2.19.3 and 2.22.1 (excluded), when rust support was | ||
| // experimental and required explicitly setting CODEQL_ENABLE_EXPERIMENTAL_FEATURES | ||
| // However, there is not need to let users know about the experimental feature flag at |
There was a problem hiding this comment.
Typo: this should read "no need", rather than "not need"
| // this point, if they want rust support they should chose a version which supports it | ||
| // publicly. |
There was a problem hiding this comment.
This might break Rust analysis for users who have pinned v3 (or similar) of the Action as well as a CodeQL CLI >= 2.19.3 && < 2.22.1 and who used CODEQL_ENABLE_EXPERIMENTAL_FEATURES. I agree that it's not unreasonable to ask users to upgrade to a newer CLI version in that case, but perhaps we should give them some warning first before breaking their workflows over night when the next Action release happens? That said, it's not clear to me that a warning in the output would necessarily be noticed. I don't know if we have any data to know how many people would be affected.
Merge / deployment checklist