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

Should "setuptools" be a runtime dependency of "opentelemetry-instrumentation"? #778

Closed
mattoberle opened this issue Oct 25, 2021 · 3 comments · Fixed by #781
Closed

Should "setuptools" be a runtime dependency of "opentelemetry-instrumentation"? #778

mattoberle opened this issue Oct 25, 2021 · 3 comments · Fixed by #781
Labels
bug Something isn't working good first issue Good for newcomers help wanted Extra attention is needed

Comments

@mattoberle
Copy link
Contributor

Describe your environment

I'm running Python 3.8.10 on Ubuntu 20.04 (but it shouldn't make a difference here).

Additional Context

I believe that setuptools recently moved from a buildtime dependency to a runtime dependency in #738.

Direct references to pkg_resources were added outside of the build context here:

In most cases individuals are unlikely to encounter a problem because they had to have setuptools present to pip install in the first place.

There are cases where this could be a problem, for example:

  • A multi-stage Docker build where pip-installed packages are copied to a new image that does not include packaging tools.
  • Using a build system, eg. pants where artifacts are designed to be isolated from the system python environment.

Essentially, any case that involves creating an artifact with pip install -t is open to an import error:

ModuleNotFoundError: No module named 'pkg_resources'

I'm opening this issue to discuss whether setuptools should be added to install_requires here:

There are workarounds for the cases that don't support the current setup.cfg, but I've encountered this with relatively few packages and figured it might be worth a look. I can open the PR if it is.

@mattoberle mattoberle added the bug Something isn't working label Oct 25, 2021
@owais
Copy link
Contributor

owais commented Oct 25, 2021

Sounds reasonable. We probably don't want to pin it though and keep it as open as possible. Is there a minimum version that contains pkg_resources or is it available in all versions?

Q: Are there any downsides/concerns to adding setuptools deps?

//cc @open-telemetry/opentelemetry-python-contrib-approvers @open-telemetry/python-maintainers

@mattoberle
Copy link
Contributor Author

100% agree with the pin sentiment.
I know PEP 365 was opened in 2007 with the proposal to add pkg_resources to the stdlib, but was rejected.

It's been around for quite a while but I'll see if I can track down the earliest version that supports the current imports.

@mattoberle
Copy link
Contributor Author

I cloned setuptools and ran git bisect run {script} where {script} was a shell script containing:

#!/bin/bash
set -e -o pipefail

[[ $(
    grep --include=*.py --exclude-dir=.git -rE \
        '^class (Distribution|DistrubtionNotFound|RequirementParseError|VersionConflict)|^def get_distribution\(' \
        pkg_resources \
    | wc -l
) -ne 5 ]]

The earliest compatible setuptools is from 2015 here (this commit introduced RequirementParseError):
pypa/setuptools@cd79379

The minimum pin we'd need is 16.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working good first issue Good for newcomers help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants