Rust: remove shipped feature flag by redsun82 · Pull Request #2960 · github/codeql-action · GitHub

Merge / deployment checklist

Confirm this change is backwards compatible with existing workflows.
Confirm the readme has been updated if necessary.
Confirm the changelog has been updated if necessary.

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

I think this is OK to merge if you are happy to break Rust analyses for those older versions of the CLI. See my thoughts in the comment I added. There's also one typo in the 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.

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.

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 initConfig. That got me thinking, that actually since the feature flag has been rolled out, the effective version from which we must support Rust to be backward compatible is 2.19.3, which means that we must set the env variable in versions between 2.19.3 and 2.22.1. I've changed the code accordingly in the if block before initConfig, and removed the later if block which becomes unneeded.

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).

This is slightly ambiguous: does the "(excluded)" refer to both 2.19.3 and 2.22.1 or only the latter?

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, ...?

Minor: This can probably be removed. I don't see much value in documenting what used to be the case here.

Also run the `rust` checks on "milestone" CLI releases, to ensure we remain backward compatible with those versions. This was prompted by #2960 (review) Running this on current `main` and then on that PR should improve our confidence we remain backward compatible. It also turns out a probable `ruamel.yaml` update was changing a lot of generated workflows, so I've: * fixed the `ruamel.yaml` version to the latest in `sync.sh` * added `yaml.width = 120` in `sync.py` to minimize (but not entirely remove) the number of changes * checked in the workflows whose formatting was changed by the new `ruamel.yaml` version

ha, well, I should have double checked what copilot was writing here. Good thing someone suggested to put in more testing 😉

(this was causing a failure in the 2.19.3 case, which is now fixed)

LGTM with the extra test cases added in #3023. One minor point, but it's not blocking this PR.

Minor: Should "rust" be capitalised here?

Makes sense

Rust: remove shipped feature flag

3478646

redsun82 force-pushed the redsun82/rust branch from 95b072e to 3478646 Compare July 3, 2025 13:43

redsun82 marked this pull request as ready for review July 4, 2025 14:41

Copilot AI review requested due to automatic review settings July 4, 2025 14:41

redsun82 requested a review from a team as a code owner July 4, 2025 14:41

Copilot AI reviewed Jul 4, 2025

mbg requested changes Jul 30, 2025

View reviewed changes

Comment thread src/init-action.ts Outdated

Comment thread src/init-action.ts Outdated

redsun82 added 2 commits August 4, 2025 17:24

Merge branch 'main' into redsun82/rust

662cec8

Simplify rust check

a58e7d8

mbg mentioned this pull request Aug 5, 2025

Allow using new CodeQL languages without updating the CodeQL Action #2914

Merged

3 tasks

mbg previously approved these changes Aug 5, 2025

View reviewed changes

Fix typo

3e4d856

redsun82 dismissed mbg’s stale review via 3e4d856 August 6, 2025 04:33

Update comments on rust support in init-action

6802597

Clarify comments regarding rust support in codeql versions

redsun82 requested a review from mbg August 6, 2025 04:36

Transpile

8d19b24

mbg previously approved these changes Aug 6, 2025

View reviewed changes

redsun82 added 2 commits August 8, 2025 11:49

Merge branch 'main' into redsun82/rust

068f150

Rework rust code

67812dd

redsun82 dismissed mbg’s stale review via 67812dd August 8, 2025 10:10

redsun82 requested a review from mbg August 8, 2025 10:14

mbg reviewed Aug 11, 2025

View reviewed changes

redsun82 added 2 commits August 11, 2025 14:34

Merge branch 'main' into redsun82/rust

68da2c5

Address review

bfa52a8

redsun82 mentioned this pull request Aug 11, 2025

Improve Rust analysis PR check #3023

Merged

3 tasks

Merge branch 'main' into redsun82/rust

aa456a5

redsun82 commented Aug 12, 2025

View reviewed changes

Fix EXPERIMENTAL_FEATURES environment variable

a9c4652

redsun82 requested a review from mbg August 12, 2025 10:33

mbg previously approved these changes Aug 12, 2025

View reviewed changes

redsun82 dismissed mbg’s stale review via 162e9ba August 12, 2025 11:29

redsun82 force-pushed the redsun82/rust branch from 162e9ba to 939a13f Compare August 12, 2025 11:32

redsun82 added 2 commits August 12, 2025 13:33

Capitalize Rust in log

486a50d

Merge branch 'main' into redsun82/rust

df1ceac

redsun82 force-pushed the redsun82/rust branch from 939a13f to df1ceac Compare August 12, 2025 11:33

redsun82 requested a review from mbg August 12, 2025 11:34

mbg approved these changes Aug 12, 2025

View reviewed changes

redsun82 enabled auto-merge August 12, 2025 11:39

redsun82 merged commit 60aa58a into main Aug 12, 2025
284 checks passed

redsun82 deleted the redsun82/rust branch August 12, 2025 11:47

github-actions Bot mentioned this pull request Aug 18, 2025

Merge main into releases/v3 #3042

Merged

8 tasks

-                  // 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)

+                  // 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.

+                /**
+                 * Whether to enable experimental extractors for CodeQL.
+                 */
+                EXPERIMENTAL_FEATURES = "CODEQL_ACTION_EXPERIMENTAL_FEATURES",

-                    logger.info("Experimental rust analysis enabled");
+                    if (semver.lt(actualVer, publicPreview)) {
+                      core.exportVariable(EnvVar.EXPERIMENTAL_FEATURES, "true");
+                      logger.info("Experimental rust analysis enabled");

		// this point, if they want rust support they should chose a version which supports it
		// publicly.

		// We need to set this environment variable before initializing the config, otherwise Rust
		// analysis will not be enabled.

Rust: remove shipped feature flag by redsun82 · Pull Request #2960 · github/codeql-action · GitHub

Uh oh!

Rust: remove shipped feature flag - #2960

Rust: remove shipped feature flag#2960redsun82 merged 14 commits intomaingithub/codeql-action:mainfrom redsun82/rustgithub/codeql-action:redsun82/rustCopy head branch name to clipboard

Conversation

redsun82 commented Jul 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Merge / deployment checklist

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

mbg left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

henrymercer commented Aug 7, 2025

Uh oh!

henrymercer commented Aug 8, 2025

Uh oh!

redsun82 commented Aug 8, 2025

Uh oh!

mbg left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mbg left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Rust: remove shipped feature flag#2960
redsun82 merged 14 commits into
mainfrom
redsun82/rust

redsun82 commented Jul 3, 2025 •
edited
Loading