Rust: remove shipped feature flag #2960
Conversation
src/init-action.ts
Outdated
| // 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.
There was a problem hiding this comment.
in that situation (version 2.19.3..2.22.1 and CODEQL_ENABLE_EXPERIMENTAL_FEATURES=true) codeql resolve languages does give rust, which means that (await codeql.resolveLanguages()).rust is not undefined. The only change then is that users lose the Experimental Rust analysis enabled log line, but no functional change happens.
There was a problem hiding this comment.
uh, actually, now I see that that log line was dead code anyway!
Clarify comments regarding rust support in codeql versions
|
#3014 should ease the merge conflict in compiled code pain here. |
|
(Though we'll have to do the merge first, to get the updated workflow in the branch) |
|
I've noticed that #2914 duplicated the rust specific bock before |
There was a problem hiding this comment.
As I wrote elsewhere, I am a bit concerned that we have gone through a few iterations of the logic here, approved it, and then changed our minds about how this should work.
That signals to me that we:
- Don't necessarily know what this should do and, while these changes make sense in isolation, we could still be missing something. It might be good to outline internally in an issue what different scenarios we need to consider for this (e.g. permutations of: Advanced/Default Setup, GH-hosted runner vs self-hosted, different CLI versions, Rust with and without other languages, ...). I think having a matrix of scenarios to consider like that might be more broadly useful a well.
- Don't have any test coverage that would tell us if the logic here doesn't do the right thing. I believe that we can pin specific CLI versions in the PR checks. So perhaps it would make sense to have a PR check that tests this with different permutations of relevant CLI versions (
< 2.19.3,>= 2.19.3 && < 2.22.1, and>= 2.22.1)? Or perhaps a combination of unit tests (to check the general logic of this) and PR check (to check that the analysis for Rust works when it should).
| // Set CODEQL_ENABLE_EXPERIMENTAL_FEATURES for Rust. We need to set this environment | ||
| // variable before initializing the config, otherwise Rust analysis will not be | ||
| // enabled. | ||
| // Set CODEQL_ENABLE_EXPERIMENTAL_FEATURES for Rust if between 2.19.3 and 2.22.1 (excluded) |
There was a problem hiding this comment.
This is slightly ambiguous: does the "(excluded)" refer to both 2.19.3 and 2.22.1 or only the latter?
| // We need to set this environment variable before initializing the config, otherwise Rust | ||
| // analysis will not be enabled. |
There was a problem hiding this comment.
Could you add a small bit to this comment to remind us why it will not be enabled otherwise? I.e. is the logic for that elsewhere in the Action, the CLI, the extractor, ...?
| // Set CODEQL_ENABLE_EXPERIMENTAL_FEATURES for Rust if between 2.19.3 and 2.22.1 (excluded) | ||
| // We need to set this environment variable before initializing the config, otherwise Rust | ||
| // analysis will not be enabled. | ||
| // Initially this was driven by a feature flag which has been rolled out already. |
There was a problem hiding this comment.
Minor: This can probably be removed. I don't see much value in documenting what used to be the case here.
Merge / deployment checklist