Skip to content
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

Add a fix for python 3.12 #1928

Merged
merged 4 commits into from
Oct 6, 2023
Merged

Add a fix for python 3.12 #1928

merged 4 commits into from
Oct 6, 2023

Conversation

aeisenberg
Copy link
Contributor

@aeisenberg aeisenberg commented Oct 6, 2023

The python extractor does not yet support 3.12. Check for this and instead make sure we run python 3.11. Only need to check on windows since we are extremely unlikely to be running 3.12 on linux or macos.

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.

The python extractor does not yet support 3.12. Check for this and
instead make sure we run python 3.11. Only need to check on windows
since we are extremely unlikely to be running 3.12 on linux or macos.
@aeisenberg aeisenberg requested review from a team as code owners October 6, 2023 20:07
Copy link
Contributor

@adityasharad adityasharad left a comment

Choose a reason for hiding this comment

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

Clever. Looks reasonable.
Can we have an integration test on Windows that calls setup-python with 3.12 to exercise this path?

Write-Host "Python 3.11 detected, using this version instead of 3.12+."
Write-Output "PY_PYTHON3=3.11" | Out-File -FilePath $Env:GITHUB_ENV -Encoding utf8 -Append
} else {
Write-Host "FAILURE: Python 3.12+ is not supported. Please install Python 3.11."
Copy link
Contributor

Choose a reason for hiding this comment

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

Optional.

Suggested change
Write-Host "FAILURE: Python 3.12+ is not supported. Please install Python 3.11."
Write-Host "FAILURE: Python 3.12+ is not supported, and Python 3.11 could not be detected on the system. Please install Python 3.11."

#! /usr/bin/pwsh

# If we are running greater than or equal to python 3.12, change py to run version 3.11
Write-Host "Checking python version"
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Write-Host "Checking python version"
Write-Host "Checking Python version"

Super-nit

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, I think all Python should be python.

@aeisenberg
Copy link
Contributor Author

Should be simple enough to create a test.

Also:

- Update changelog
- Fix comments in check_python script
@criemen
Copy link
Contributor

criemen commented Oct 6, 2023

I think we should query a feature flag from the CLI so that we can turn off this workaround from within the CLI once we've a release out that fixes the python extractor.

@aeisenberg
Copy link
Contributor Author

Good call. I can check for less than 2.15.0.

@criemen
Copy link
Contributor

criemen commented Oct 6, 2023

I think the new tool-features feature (see https://github.com/github/codeql-action/pull/1909/files#diff-a2b4f46aa09bd648db18893b59331e2e037dfb60954c80383d820703b09d2519) would be more appropriate to use.

@aeisenberg
Copy link
Contributor Author

That requires adding the feature to the CLI directly, no?

@criemen
Copy link
Contributor

criemen commented Oct 6, 2023

As far as I understand it yes, but only once we're actually shipping new python-3.12 support - which should be doable, as we release the python extractor together with the CLI.

@aeisenberg
Copy link
Contributor Author

Ah...I see what you're saying. I choose a feature name now and that will be the feature name in the version of codeql that has the fix. Good idea.

@github-advanced-security
Copy link

This pull request sets up GitHub code scanning for this repository. Once the scans have completed and the checks have passed, the analysis results for this pull request branch will appear on this overview. Once you merge this pull request, the 'Security' tab will show more code scanning analysis results (for example, for the default branch). Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results. For more information about GitHub code scanning, check out the documentation.

CHANGELOG.md Outdated Show resolved Hide resolved
angelapwen
angelapwen previously approved these changes Oct 6, 2023
Copy link
Contributor

@angelapwen angelapwen left a comment

Choose a reason for hiding this comment

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

Looks good modulo question on the changelog file!

@@ -4,7 +4,7 @@ See the [releases page](https://github.com/github/codeql-action/releases) for th

## [UNRELEASED]

No user facing changes.
- Add a workaround python 3.12, which is not supported in CodeQL CLI version 2.14.6 or earlier. If you are running an analysis on Windows and using python 3.12 or later, the CodeQL Action will switch to running python 3.11. If python 3.11 is not found, then the workflow will fail.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- Add a workaround python 3.12, which is not supported in CodeQL CLI version 2.14.6 or earlier. If you are running an analysis on Windows and using python 3.12 or later, the CodeQL Action will switch to running python 3.11. If python 3.11 is not found, then the workflow will fail.
- Add a workaround for python 3.12, which is not supported in CodeQL CLI version 2.14.6 or earlier. If you are running an analysis on Windows and using python 3.12 or later, the CodeQL Action will switch to running python 3.11. If python 3.11 is not found in this case, then the workflow will fail.

Just realized it's missing a word..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks I'll put this in a follow-up.

@aeisenberg aeisenberg merged commit 0d5c2e0 into main Oct 6, 2023
391 checks passed
@aeisenberg aeisenberg deleted the aeisenberg/fix-python312 branch October 6, 2023 23:42
@aeisenberg aeisenberg mentioned this pull request Oct 7, 2023
@github-actions github-actions bot mentioned this pull request Oct 9, 2023
6 tasks
@aeisenberg aeisenberg mentioned this pull request Nov 10, 2023
3 tasks
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.

4 participants