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

Fix missing setuptools dep #4799

Merged
merged 2 commits into from
Aug 9, 2024
Merged

Conversation

joeyorlando
Copy link
Contributor

@joeyorlando joeyorlando commented Aug 9, 2024

What this PR does

tldr; I think we should install setuptools into our engine Dockerfile + in our CI env because Python 3.12 no longer installs distutils by default. This should unblock us from being able to merge #4656 and #4555.

More details

I would like to be able to merge #4656 and #4555. However, in both of these PRs setuptools is being removed from requirements-dev.txt (here and here). This leads to things breaking (CI job #1 and #2) because of:

File "/opt/hostedtoolcache/Python/3.12.3/x64/lib/python3.12/site-packages/polymorphic/__init__.py", line 9, in <module>
    import pkg_resources
ModuleNotFoundError: No module named 'pkg_resources'

(☝️ from the pkg_resources docs.. "The pkg_resources module distributed with setuptools provides an API for Python libraries")

Python 3.12 made a change to no longer pre-install distutils (relevant release notes):

PEP 632: Remove the distutils package. See the migration guide for advice replacing the APIs it provided. The third-party Setuptools package continues to provide distutils, if you still require it in Python 3.12 and beyond.

gh-95299: Do not pre-install setuptools in virtual environments created with venv. This means that distutils, setuptools, pkg_resources, and easy_install will no longer available by default; to access these run pip install setuptools in the activated virtual environment.

Additionally, setuptools is in pip-tools UNSAFE_PACKAGES list (related GitHub issue), hence why I think Dependabot is removing it in #4656 and #4555.

Checklist

  • Unit, integration, and e2e (if applicable) tests updated
  • Documentation added (or pr:no public docs PR label added if not required)
  • Added the relevant release notes label (see labels prefixed w/ release:). These labels dictate how your PR will
    show up in the autogenerated release notes.

@joeyorlando joeyorlando added pr:no public docs Added to a PR that does not require public documentation updates release:ignore PR will not be added to release notes labels Aug 9, 2024
@joeyorlando joeyorlando self-assigned this Aug 9, 2024
@joeyorlando joeyorlando requested a review from a team August 9, 2024 18:39
@@ -53,7 +53,7 @@ python-telegram-bot==13.13
recurring-ical-events==2.1.0
redis==5.0.1
regex==2021.11.2
requests==2.32.0
requests==2.32.3
Copy link
Contributor Author

@joeyorlando joeyorlando Aug 9, 2024

Choose a reason for hiding this comment

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

also, bump requests because of this message I was seeing when running uv pip compile:

warning: `requests==2.32.0` is yanked (reason: "Yanked due to conflicts with CVE-2024-35195 mitigation").

https://nvd.nist.gov/vuln/detail/CVE-2024-35195

@@ -34,7 +34,7 @@ cachetools==4.2.2
# via
# google-auth
# python-telegram-bot
celery[redis]==5.3.1
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 don't quite understand why it's dropping the [redis] portion here 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

FYI, astral-sh/uv#1595 (I don't have a strong opinion to preserve the extra, though)

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 for the reference 👍 (seems like including it/excluding it doesn't make a big difference)

@@ -157,7 +157,7 @@ firebase-admin==5.4.0
# via fcm-django
flask==3.0.2
# via slack-export-viewer
google-api-core[grpc]==2.17.0
Copy link
Contributor Author

Choose a reason for hiding this comment

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

similar to this comment

@joeyorlando joeyorlando requested a review from matiasb August 9, 2024 18:41
Comment on lines +419 to +422
setuptools==72.1.0
# via
# apscheduler
# opentelemetry-instrumentation
Copy link
Contributor Author

@joeyorlando joeyorlando Aug 9, 2024

Choose a reason for hiding this comment

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

@@ -34,7 +34,7 @@ cachetools==4.2.2
# via
# google-auth
# python-telegram-bot
celery[redis]==5.3.1
Copy link
Contributor

Choose a reason for hiding this comment

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

FYI, astral-sh/uv#1595 (I don't have a strong opinion to preserve the extra, though)

@@ -27,7 +27,7 @@ RUN if [ "$TARGETPLATFORM" = "linux/arm64" ]; then \
&& rm grpcio-1.64.1-cp312-cp312-linux_aarch64.whl; \
fi

RUN pip install uv
RUN pip install uv setuptools
Copy link
Contributor

Choose a reason for hiding this comment

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

if setuptools is being added through requirements, does it need to be here too? (same above)

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 guess technically no, but I think setuptools is only being included in requirements.txt/requirements-dev.txt because of two transitive dependencies. If those dependencies drop their requirement on setuptools we'll need this anyways 🤔

Python 3.12's release notes mention:

python/cpython#95299: Do not pre-install setuptools in virtual environments created with venv. This means that distutils, setuptools, pkg_resources, and easy_install will no longer available by default; to access these run pip install setuptools in the activated virtual environment.

@joeyorlando joeyorlando enabled auto-merge August 9, 2024 20:01
@joeyorlando joeyorlando added this pull request to the merge queue Aug 9, 2024
Merged via the queue into dev with commit 535baf7 Aug 9, 2024
21 checks passed
@joeyorlando joeyorlando deleted the jorlando/fix-missing-setuptools-dep branch August 9, 2024 20:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr:no public docs Added to a PR that does not require public documentation updates release:ignore PR will not be added to release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants