Skip to content

Rust: remove shipped feature flag #2960

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Rust: remove shipped feature flag #2960

wants to merge 6 commits into from

Conversation

redsun82
Copy link
Contributor

@redsun82 redsun82 commented Jul 3, 2025

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.

@redsun82 redsun82 marked this pull request as ready for review July 4, 2025 14:41
@Copilot Copilot AI review requested due to automatic review settings July 4, 2025 14:41
@redsun82 redsun82 requested a review from a team as a code owner July 4, 2025 14:41
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

mbg
mbg previously approved these changes Aug 5, 2025
Copy link
Member

@mbg mbg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +637 to +638
// this point, if they want rust support they should chose a version which supports it
// publicly.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

uh, actually, now I see that that log line was dead code anyway!

Clarify comments regarding rust support in codeql versions
@redsun82 redsun82 requested a review from mbg August 6, 2025 04:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants