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 Python 3.12 support #282

Merged
merged 2 commits into from
Apr 15, 2024
Merged

Add Python 3.12 support #282

merged 2 commits into from
Apr 15, 2024

Conversation

mrtmm
Copy link
Contributor

@mrtmm mrtmm commented Mar 28, 2024

  • Update test matrix:

    • xblock-sdk v0.9.0 includes a breaking change for XBlock versions before 2.0 but there is no upper constraint included in the requirements for this for all XBlock versions. Thus, all our tests have been failing since the release of xblock-sdk==0.9.0 on 2024-03-21.
    • Update our test matrix to include XBlock versions 1.7 / 1.8 / 1.9 and add an upper constraint for xblock-sdk.
    • The Open edX Quince release is using XBlock v1.8, which does pin the xblock-sdk to v0.7.0 so in this case we only need to add the constrait to our test runs.
  • Add support for Python 3.12

xblock-sdk v0.9.0 includes a breaking change for XBlock versions
before 2.0 but there is no upper constraint included in the
requirements for this for all XBlock versions. Thus, all our tests
have been failing since the release of xblock-sdk==0.9.0 on
2024-03-21.

Update our test matrix to include XBlock versions 1.7 / 1.8 / 1.9
and add an upper constraint for xblock-sdk.

The Open edX Quince release is using XBlock v1.8, which does pin
the xblock-sdk to v0.7.0 so in this case we only need to add the
constrait to our test runs.
Copy link

codecov bot commented Mar 28, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.43%. Comparing base (a180de8) to head (cc751ad).

❗ Current head cc751ad differs from pull request most recent head 22218f3. Consider uploading reports for the commit 22218f3 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #282      +/-   ##
==========================================
- Coverage   95.43%   95.43%   -0.01%     
==========================================
  Files          24       24              
  Lines        1973     1972       -1     
==========================================
- Hits         1883     1882       -1     
  Misses         90       90              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@mrtmm mrtmm force-pushed the python-3.12 branch 5 times, most recently from f346503 to 73b8006 Compare April 3, 2024 09:44
@fghaas
Copy link
Contributor

fghaas commented Apr 3, 2024

I believe that to unbreak the tests on Python 3.12, you'll still need to add setuptools to the requirements/tests.txt list. But maybe you can achieve the same effect by dropping the <6 version specifier on setuptools-scm in requirements/setup.txt — the reason why we had that constraint in the requirements (e2dc395) is long obsolete.

@mrtmm mrtmm force-pushed the python-3.12 branch 8 times, most recently from a6a642d to 70c216d Compare April 3, 2024 14:49
Copy link
Contributor

@fghaas fghaas left a comment

Choose a reason for hiding this comment

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

Just a few thoughts here.

requirements/test.txt Outdated Show resolved Hide resolved
@@ -27,7 +29,7 @@ jobs:
python-version: ${{ matrix.python-version }}
- name: Install dependencies
run: |
pip install "tox<4" tox-gh-actions tox-pip-version
pip install "tox<4" tox-gh-actions tox-pip-version setuptools-scm
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this addition is also not necessary, since we do pull in setuptools-scm via setup.py and base.txt now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I tried that and I got "Module not found" errors, but I will try again now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried again, and even with setuptools-scm in base.txt, gh-actions was missing this and all tests failed. So, I think we need this addition here.

Copy link
Contributor

Choose a reason for hiding this comment

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

I completely understand that I'm nitpicking here, but I think mrtmm#9 has a more correct fix for this. :) What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, looks good. I merged and squashed it, not sure why the PR didn't close, I think I should have pushed the branch before squashing it.

.github/workflows/tox.yml Show resolved Hide resolved
@mrtmm mrtmm force-pushed the python-3.12 branch 5 times, most recently from 3ed3dd1 to 2a5d7a6 Compare April 11, 2024 13:55
@mrtmm mrtmm marked this pull request as ready for review April 11, 2024 14:13
@mrtmm mrtmm requested a review from fghaas April 15, 2024 09:55
* Add support for Python 3.12
* Also, update the GitHub Actions workflow matrix so that we test
  all supported pip versions with Python 3.8–3.11, but only pip 23
  on Python 3.12.
Copy link
Contributor

@fghaas fghaas 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 to me now, thanks!

@mrtmm mrtmm merged commit 22218f3 into hastexo:master Apr 15, 2024
14 checks passed
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.

None yet

2 participants