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 direct/explicit dependencies on yarl #1821

Closed
wants to merge 2 commits into from

Conversation

musicinmybrain
Copy link
Contributor

Description

This makes the actual dependencies explicit and avoids relying on indirect or implicit dependencies.

In some environments, with some combinations of packages, this can prevent an ImportError when yarl is imported but was never required.

No issue was filed.

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

  • Test A: Run the normal tests and observe no regressions. (I did this by applying this PR as a patch to the Fedora Linux package.)
  • Test B: Compare the added dependencies against the actual imports (see the bottom of this PR text).

Does This PR Require a Core Repo Change?

  • Yes. - Link to PR:
  • No.

Checklist:

See contributing.md for styleguide, changelog guidelines, and more.

  • Followed the style guidelines of this project
  • Changelogs have been updated
  • Unit tests have been added
  • Documentation has been updated

The minimum version ensures the expected URL string in the tests is
consistent; see
open-telemetry#1772.

The yarl dependency is needed both for the package and for the tests.
@musicinmybrain
Copy link
Contributor Author

Hmm. I am not sure about the >= 1.9.1. In my testing of Fedora Linux Rawhide with yarl 1.9.2, I’m having to revert #1772 to get the URL string to match. I can’t easily get the test suite to run properly outside the RPM build environment, so it’s hard for me to try, say, yarl 1.9.1 for comparison.

@srikanthccv
Copy link
Member

We don't add any explicit dependency for any instrumentation package. For automatic instrumentation, the instrumentation package won't be installed with opentelemetry-bootstrap if the dependent package is not installed and won't be instrumented if the version criteria don't match https://github.com/open-telemetry/opentelemetry-python-contrib/blob/main/opentelemetry-instrumentation/src/opentelemetry/instrumentation/bootstrap_gen.py.

@musicinmybrain
Copy link
Contributor Author

We don't add any explicit dependency for any instrumentation package. […]

Ok, so the idea is that indirect dependencies via known versions should suffice.

It turns out that I’m able to build this without the direct dependencies in Fedora as well (I was encountering several overlapping difficulties when I opened this PR), so I’ll just close the PR and move on.

Thanks for your response!

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.

2 participants