Skip to content

Feat/add support for python 3.12+ reloaded #1950

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 16 commits into
base: main
Choose a base branch
from

Conversation

instinct-vfx
Copy link
Contributor

Fixes #1949 builds on #1943 as many of the issues are due to outdated vendored dependencies

@instinct-vfx instinct-vfx requested a review from a team as a code owner April 16, 2025 11:26
@instinct-vfx instinct-vfx force-pushed the feat/add_support_for_python-3.12+_reloaded branch from c455cba to 7edd950 Compare April 16, 2025 11:28
Copy link

codecov bot commented Apr 16, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 59.28%. Comparing base (5c15568) to head (1e3e1ec).
Report is 6 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1950   +/-   ##
=======================================
  Coverage   59.28%   59.28%           
=======================================
  Files         126      126           
  Lines       17218    17218           
  Branches     3017     3017           
=======================================
  Hits        10208    10208           
  Misses       6325     6325           
  Partials      685      685           

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

maxnbk and others added 16 commits April 16, 2025 13:51
Signed-off-by: Stephen Mackenzie <[email protected]>
Signed-off-by: Thorsten Kaufmann <[email protected]>
Signed-off-by: Stephen Mackenzie <[email protected]>
Signed-off-by: Thorsten Kaufmann <[email protected]>
Signed-off-by: Stephen Mackenzie <[email protected]>
Signed-off-by: Thorsten Kaufmann <[email protected]>
Signed-off-by: Stephen Mackenzie <[email protected]>
Signed-off-by: Thorsten Kaufmann <[email protected]>
Signed-off-by: Stephen Mackenzie <[email protected]>
Signed-off-by: Thorsten Kaufmann <[email protected]>
Signed-off-by: Stephen Mackenzie <[email protected]>
Signed-off-by: Thorsten Kaufmann <[email protected]>
Signed-off-by: Stephen Mackenzie <[email protected]>
Signed-off-by: Thorsten Kaufmann <[email protected]>
Signed-off-by: Stephen Mackenzie <[email protected]>
Signed-off-by: Thorsten Kaufmann <[email protected]>
Signed-off-by: Stephen Mackenzie <[email protected]>
Signed-off-by: Thorsten Kaufmann <[email protected]>
@instinct-vfx instinct-vfx force-pushed the feat/add_support_for_python-3.12+_reloaded branch from f443e2b to 1e3e1ec Compare April 16, 2025 11:52
@@ -68,6 +68,7 @@ def find_files(pattern, path=None, root="rez"):
packages=find_packages('src', exclude=["build_utils",
"build_utils.*",
"tests"]),
install_requires=['setuptools'],
Copy link
Contributor

Choose a reason for hiding this comment

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

With this new PR, I think this is my only issue. I'll take the branch, erase setuptools on my local host, and see if I'm still able to install rez. If I can't, I do think we need to call it out on the Installation page. It's not exactly an onerous requirement, but a requirement nonetheless.

Copy link
Contributor

Choose a reason for hiding this comment

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

Interesting - I erased setuptools on my local box, and it still installed... How the heck?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For visibility:

Up to Python 3.11 when creating a virtualenv setuptools is automatically installed into the virtualenv. Starting with 3.12 this is not the case anymore. By listing setuptools as an explicit dependency installing rez with pip automatlically installs the missing setuptools.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the corresponding github issue in python: python/cpython#95299
And it is also listed in the What's new in the docs: https://docs.python.org/3.12/whatsnew/3.12.html under "Important deprecations, removals or restrictions:"

Choose a reason for hiding this comment

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

Interesting - I erased setuptools on my local box, and it still installed... How the heck?

@maxnbk install_requires means "dependencies at runtime", not "dependencies that are needed to install the project".

But now I'm curious, why do we need setuptools at runtime?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To be brutally honest i am not entirely sure we do, but i think we do. Here is an example of a failed run:

https://github.com/instinct-vfx/rez/actions/runs/14479919556/job/40614454516#step:6:35

Note that it failed on the "install rez with rez-pip" step not on the actual install. Does that not mean we need setuptools in rez's venv for good?

Copy link
Member

@JeanChristopheMorinPerso JeanChristopheMorinPerso left a comment

Choose a reason for hiding this comment

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

Thanks @instinct-vfx, I left some comments.

@@ -38,7 +38,8 @@

Paths should use the path separator appropriate for the operating system
(based on Python's os.path.sep). So for Linux paths, / should be used. On
Windows \ (unescaped) should be used.
Windows \\ (unescaped! A change in python 3.12 does not allow single backslashes

Choose a reason for hiding this comment

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

We could add r do the string like we usually do with regexes. THis will allow us to have a single \.

@@ -68,6 +68,7 @@ def find_files(pattern, path=None, root="rez"):
packages=find_packages('src', exclude=["build_utils",
"build_utils.*",
"tests"]),
install_requires=['setuptools'],

Choose a reason for hiding this comment

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

Interesting - I erased setuptools on my local box, and it still installed... How the heck?

@maxnbk install_requires means "dependencies at runtime", not "dependencies that are needed to install the project".

But now I'm curious, why do we need setuptools at runtime?

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.

3 participants